Matter Switch: remove ENERGY_MANAGEMENT_ENDPOINT field #2436
Conversation
|
Channel deleted. |
| local EVE_MANUFACTURER_ID, EVE_PRIVATE_CLUSTER_ID = 0x130A, 0x130AFC01 | ||
| local eve_private_energy_eps = device:get_endpoints(EVE_PRIVATE_CLUSTER_ID) | ||
| if device.manufacturer_info.vendor_id == EVE_MANUFACTURER_ID and #eve_private_energy_eps > 0 then | ||
| if device.network_type == device_lib.NETWORK_TYPE_MATTER and device.manufacturer_info.vendor_id == EVE_MANUFACTURER_ID and #eve_private_energy_eps > 0 then |
There was a problem hiding this comment.
This check may be required if a child device attempts to call this function for two reasons:
- a child device may not have
manufacturer_infofilled, which would cause an error. - The Eve subdriver only accepts non-child devices, so technically this is a more accurate check since we're trying to only gate devices that would be using the subdriver.
Both of these are kinda theoretical, but a unit test caught the issue and rather than updating the unit test, I figured the above reasons justified the inclusion anyway.
There was a problem hiding this comment.
Is this directly related to removing the ENERGY_MANAGEMENT_ENDPOINT? If not, I would make this a separate PR. If yes, I'm not sure I understand the connection, could you explain?
There was a problem hiding this comment.
It's semi-related. A child device may not have manufacturer info set, which would cause an issue. So in a unit test, that wasn't set. We also don't set the manufacturer info for child devices in our current logic.
Test Results 71 files 452 suites 0s ⏱️ Results for commit d52d763. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against d52d763 |
drivers/SmartThings/matter-switch/src/generic_handlers/attribute_handlers.lua
Outdated
Show resolved
Hide resolved
| -- [[ ELECTRICAL POWER MEASUREMENT CLUSTER ATTRIBUTES ]] -- | ||
|
|
||
| function AttributeHandlers.active_power_handler(driver, device, ib, response) | ||
| local component = device.profile.components["main"] |
There was a problem hiding this comment.
I don't think we should be hard coding in use of the main endpoint, because then this will break on a multi-component device. Do we have any multi-component power devices as far as we know?
Why does nevermind, I see we need to emit on the first switch endpointENERGY_MANAGEMENT_ENDPOINT need to be saved in the first place when we already know it is on endpoint 0 given the if ib.endpoint_id ~= 0 then statement?
There was a problem hiding this comment.
If we are going to hard code a component, why not just hardcode the endpoint for certain device types? If this is limited to a couple Aqara devices (or only 1?) I might perfer just adding special handling for them.
Is having the energy measurement cluster on the root endpoint typical, or is this the only device?
There was a problem hiding this comment.
We don't have any profiles where energy isn't on the main component. As I noted in the description, even if that comes up, I still think this is a better handling than the current.
There was a problem hiding this comment.
At the moment, this only shows up for Aqara because it is the only device that has energy on a child device. However, this will change, and we know these devices will often have the Electrical Sensor device type on multiple EPs. This would normally be where endpoint_to_component would come in. In this case, I've just circumvented that function and defined the relationship explicitly (since we already know it will be like this in all cases for the foreseeable future since plugs and switches are locked into single component profiles).
Perhaps endpoint_to_component could be a better option though, but tbh that is just punting this same logic to a helper function.
There was a problem hiding this comment.
I think this is all a matter of opinion, but to me we should avoid hard coding components in our function as a rule since we created the component_to_endpoint and endpoint_to_component apis explicitly for this purpose. I believe the cleanest solution is to handle this mapping in our available APIs, and leave component and endpoint mapping outside of attribute handlers as much as we can. If I am not mistaken, can we not just add the root node to the component to endpoint map and have it map to the main component, and then continue to leverage that function as expected?
There was a problem hiding this comment.
If we are going to hard code a component, why not just hardcode the endpoint for certain device types? If this is limited to a couple Aqara devices (or only 1?) I might perfer just adding special handling for them.
Is having the energy measurement cluster on the root endpoint typical, or is this the only device?
I might prefer specific handling for this device, because according to the spec it isn't supported to have clusters with the Application role implemented on the root node, which would include the Electrical Energy+Power Measurement clusters. So it's unlikely we would see this on another device
There was a problem hiding this comment.
But then again, it's already been implemented generically for a while, so I don't necessarily think we need to change it back to the original handling
There was a problem hiding this comment.
The issue is more generic than the fact that it's happening on the root node. It will occur for other devices if
- the device is a child device
- it supports Energy on an endpoint that is different from the endpoint OnOff is on.
From initial testing, I have seen this case show up twice (not counting aqara). It generically can and will occur for generic endpoints, not just root.
There was a problem hiding this comment.
For that find_child problem you mentioned where child device only use one endpoint, we can also update the find_child function specifically for our purposes here so that is could allow for multiple endpoints to map to the child device using a new device key that can have multiple endpoints. You could even update it so that we have a specific override of this function that is only set for devices like this where we need multiple endpoints mapped to a child, and other devices can continue to use the generic one.The find child function is device-scoped and not driver-scoped, so you can make a custom one specifically for devices like this and not affect others in the driver
There was a problem hiding this comment.
per this discussion, I have updated the logic to leave a single emit_event_for_endpoint call in each of these handlers in this PR: #2444
It cannot be easily broken out of the above PR anymore, since the new logic is inherently tied to the new PowerTopology handling.
|
Given this discussion's conclusion: #2436 (comment) I am closing this PR. |
Description of Change
This field is a workaround for the fact that in the Aqara Switch H2 device (and possibly others), the ElectricalEnergyMeasurement cluster is stored on the root endpoint, which is not associated with the main component in out current logic. Therefore, the current handling directly circumvents this issue by writing the event on ep 1 instead of ep 0 by means of this field.
However, there is a cleaner solution here which circumvents the entire
emit_event_for_endpointlogic, which I have implemented here, specifically by using theemit_component_event, which is whatemit_event_for_endpointbreaks down into anyway on the backend.For now,
maincan be directly written here as the component of choice (all devices currently havepowerConsumptionReportandenergyMeteron main) but if we expand this logic to become more inclusive of different configurations, this is still a more robust and more clear solution.Summary of Completed Tests
All unit tests pass.
Tested with Aqara H2 Switch (the only known device that functions like this) and
energyMeterandpowerConsumptionReportevents are emitted as expected. Tested with Meross plug and similarly functionality is retained.