Add battery level attribute support to matter drivers #1796
Conversation
nickolas-deboom
requested review from
tpmanley,
ctowns,
z-michel and
hcarter-775
|
Duplicate profile check: Warning - duplicate profiles detected. |
|
matter-lock_coverage.xml
matter-sensor_coverage.xml
matter-switch_coverage.xml
matter-thermostat_coverage.xml
matter-window-covering_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against afbeba3 |
|
Invitation URL: |
Test Results 64 files 405 suites 0s ⏱️ Results for commit afbeba3. ♻️ This comment has been updated with latest results. |
| profile_name = profile_name .. "-nobattery" | ||
| elseif #battery_feature_eps == 0 then | ||
| elseif not device:get_field(SUPPORT_BATTERY_PERCENTAGE) then |
There was a problem hiding this comment.
I would not expect this value to have been set at the time of doConfigure since the doConfigure event itself sends the read request for the AttributeList below.
If #battery feature eps is 0, we could move forward with the metadata update to change the profile to the no-battery variety here, but if the #battery eps is > 0, then we could hold off on the metadata update so that the proper profile can be determined and then set in the AttributeList handler. That way, we could avoid setting the profile if we know it is likely to change when we receive an AttributeList report. What do you think?
There was a problem hiding this comment.
Good call! I changed it so that it would only update the profile if it finds that the battery feature is not available. The AttributeList handler can update it again once it runs.
| profile_name = profile_name .. "-nobattery" | ||
| elseif #battery_feature_eps == 0 then | ||
| elseif not device:get_field(SUPPORT_BATTERY_PERCENTAGE) then |
There was a problem hiding this comment.
Similar to above, since this is being handled in the added handler, I wouldn't expect this value to have been updated yet since it relies on a read request that is sent during the initial onboarding. This will always be false then, and the device would always join to a -batteryLevel profile. I would suggest again that we count on the AttributeList handler to determine what battery support needs to be added to the profile, as mentioned above. I think this would have the added benefit of centralizing this logic in once place too!
There was a problem hiding this comment.
Like in the other comment, I changed this to only update the profile if the battery feature is not available!
| profile_name = profile_name .. "-battery" | ||
| local battery_feature_eps = device:get_endpoints(clusters.PowerSource.ID, {feature_bitmap = clusters.PowerSource.types.PowerSourceFeature.BATTERY}) | ||
| if #battery_feature_eps > 0 then | ||
| if device:get_field(SUPPORT_BATTERY_PERCENTAGE) then |
There was a problem hiding this comment.
Same issue as mentioned above, give the order in which these lifecycle events occur, I wouldn't expect this field to have been populated at this point since we'd still need to rely on a read response from the device. Even if the read request is sent and received before this function is called via doConfigure, there is still no guarantee of this order of events which could lead to a race condition. Therefore, I would rely on distinguishing between battery vs battery-level support in the AttributeList handler itself.
There was a problem hiding this comment.
In 061a1d0, I moved the read request to do_configure after match_profile runs in order to avoid the race condition. Do you agree with this approach?
| @@ -436,10 +441,10 @@ local function create_thermostat_modes_profile(device) | |||
| return thermostat_modes | |||
| end | |||
|
|
|||
| local function do_configure(driver, device) | |||
| local function match_profile(driver, device) | |||
There was a problem hiding this comment.
As a blanket statement, the same concerns I mentioned in regards to the timing and order of the lifecycle events on the other two drivers apply to this driver as well.
There was a problem hiding this comment.
I made the same change here as I did for matter-sensor (moved the read request from device_added to do_configure to ensure that it occurs after match_profile runs for the first time.
…hingsCommunity/SmartThingsEdgeDrivers into add-batterLevel-to-matter-drivers
nickolas-deboom
force-pushed
the
add-batterLevel-to-matter-drivers
branch
from
c30b955 to
b058cde
Compare
Type of Change
Checklist
Description of Change
CHAD-12161
matter-sensor, matter-thermostat, matter-lock, matter-switch, and matter-window-covering currently assume that
BatPercentRemainingis available if theBATfeature is supported, but this attribute is only optionally required if this feature is present. The changes in this PR improve the profile selection logic in these drivers by reading theAttributeList, checking ifBatPercentRemainingis available, and then re-profiling the device to use a profile with thebatterycapability rather thanbatteryLevel.Summary of Completed Tests
See new test cases.