Matter Switch: Improve battery profiling #2724
Conversation
This implements a few changes to the way that devices supporting batteries are profiled: * subscribe to PowerSource.AttributeList rather than reading this attribute, to help prevent failed reads from causing issues with device profiling * update the profile within match_profile rather than power_source_attribute_list_handler * use existing structure for handling waiting for profiling data before attempting a profile update
|
Invitation URL: |
| device:add_subscribed_attribute(clusters.PowerSource.attributes.AttributeList) | ||
| else | ||
| device:set_field(fields.profiling_data.BATTERY_SUPPORT, fields.battery_support.NO_BATTERY, {persist = true}) | ||
| end |
There was a problem hiding this comment.
This is needed because the device may fingerprint to a profile excluding battery, so PowerSource.AttributeList wouldn't be included in the initial subscription and the profile update would never occur. Let me know if you have thoughts on the placement of this block. I thought it made sense to put in added since it's only needed once.
Test Results 71 files 480 suites 0s ⏱️ Results for commit ac6e8e6. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against ac6e8e6 |
| )}) | ||
| expect_configure_buttons() | ||
| mock_device:expect_metadata_update({ profile = "button-battery" }) | ||
| end |
There was a problem hiding this comment.
This logic does not work very well within test_init, because the test framework does not seem to guarantee the timing of the PROVISIONED update unless the profile update occurs within doConfigure. Since it's being handled later in this case, it only seems to work in an external function like this, which is why I converted all the test cases to the register_coroutine_test format.
| end | ||
| if switch_utils.get_product_override_field(device, "is_climate_sensor_w100") then | ||
| profile_name = "3-button-battery-temperature-humidity" | ||
| end |
There was a problem hiding this comment.
It turns out that this is the only unique handling needed for this device (plus the existing logic in find_default_endpoint), so I was able to remove the extra block in power_source_attribute_list_handler
|
|
||
| -- For devices supporting BATTERY, add the PowerSource AttributeList to the list of subscribed | ||
| -- attributes in order to determine whether to use the battery or batteryLevel capability | ||
| if #device:get_endpoints(clusters.PowerSource.ID, {feature_bitmap = clusters.PowerSource.types.PowerSourceFeature.BATTERY}) > 0 then |
There was a problem hiding this comment.
| if #device:get_endpoints(clusters.PowerSource.ID, {feature_bitmap = clusters.PowerSource.types.PowerSourceFeature.BATTERY}) > 0 then | |
| if device:get_field(fields.profiling_data.BATTERY_SUPPORT) == nil then |
Does this make sense? Subscribe to the AttributeList if BATTERY_SUPPORT has not yet been handled?
There was a problem hiding this comment.
But what if the device doesn't support PowerSource?
There was a problem hiding this comment.
The current logic you have set up sets NO_BATTERY for all devices, whether or not they have power source?
There was a problem hiding this comment.
Oh my b I see what you meant. Yeah I think what you suggested should work fine
There was a problem hiding this comment.
Actually, I think it's better as-is because otherwise, existing devices that don't support batteries would have this field unset at driver init and therefore would subscribe to the AttributeList
There was a problem hiding this comment.
Ok, I think I still prefer this option because what it means is that we'll only actually subscribe to this attribute as long as we need to. As in, once the attribute is actually handled, this logic would have us no longer subscribe to that, keeping the subscription more lean as we've previously tried to maintain.
To keep this behavior, I have 2 thoughts.
- Should do that field setting in
init? - All that would mean is that these devices with unset fields will now subscribe once, and set the field, then go back to being unsubscribed. I'm not sure this is that bad?
drivers/SmartThings/matter-switch/src/switch_handlers/attribute_handlers.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/switch_handlers/attribute_handlers.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/switch_handlers/attribute_handlers.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/switch_utils/device_configuration.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/switch_utils/device_configuration.lua
Outdated
Show resolved
Hide resolved
|
|
||
| -- For devices supporting BATTERY, add the PowerSource AttributeList to the list of subscribed | ||
| -- attributes in order to determine whether to use the battery or batteryLevel capability | ||
| if #device:get_endpoints(clusters.PowerSource.ID, {feature_bitmap = clusters.PowerSource.types.PowerSourceFeature.BATTERY}) > 0 then |
There was a problem hiding this comment.
The current logic you have set up sets NO_BATTERY for all devices, whether or not they have power source?
drivers/SmartThings/matter-switch/src/switch_handlers/attribute_handlers.lua
Outdated
Show resolved
Hide resolved
nickolas-deboom
force-pushed
the
matter-switch-improve-battery-profiling
branch
from
48c5b71 to
ac6e8e6
Compare
This implements a few changes to the way that devices supporting batteries are profiled:
PowerSource.AttributeListrather than reading this attribute, to help prevent failed reads from causing issues with device profilingmatch_profilerather thanpower_source_attribute_list_handler