Repository navigation
Conversation
|
zigbee-window-treatment_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 5d5b792 |
Test Results 73 files 571 suites 0s ⏱️ Results for commit 5d5b792. ♻️ This comment has been updated with latest results. |
|
Invitation URL: |
| -- re-read whenever the shade moves while the device is still not initialized. | ||
| local function current_position_lift_percentage_handler(driver, device, value, zb_rx) | ||
| if is_not_init(device) then | ||
| device:send(cluster_base.read_manufacturer_specific_attribute(device, aqara_utils.PRIVATE_CLUSTER_ID, |
There was a problem hiding this comment.
This will result in a lot of reads if the controller is not yet initialized; a single move usually results in multiple current position percent reports from the device. It seems like there could be a situation where the start/stop limits of the controller are never set by the user, and so initialization never gets set, and this always issues unnecessary reads.
Currently the driver is only reading init state from the refresh handler. Is this read needed here, or can we remove it entirely? At the very least I think we need some mechanism to reduce these reads to a single read while the controller is moving the blind and it is generating reports.
There was a problem hiding this comment.
Due to the nature of the roller shade, initialization (setting the limits) is mandatory, not optional.
Even if a user attempts to operate the device without initialization, level control is not supported. In this uninitialized state, the device only sends a report after the motor stops completely, so the excessive read scenario you are concerned about will rarely occur. (Since the 0%-100% range is unknown without initialization, the device does not generate intermediate position reports while moving.)
This read logic was added as a safeguard to handle edge cases where the initialization state fails to update properly. Could you please take this context into consideration and review it again?
We need to consider whether prioritizing the accurate display of the current state outweighs the minimal risk of additional traffic.
There was a problem hiding this comment.
Can we instead do this read from the shade_state_report_handler handler to avoid the minimal risk of additional traffic.
|
|
cjswedes
left a comment
cjswedes
left a comment
There was a problem hiding this comment.
This is super close, just a few more code reproduction issues.
cjswedes
left a comment
cjswedes
left a comment
There was a problem hiding this comment.
Just waiting on @aleclorimer review prior to merging.
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests