Repository navigation
Conversation
|
zigbee-window-treatment_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 3ce2a43 |
Test Results 73 files 546 suites 0s ⏱️ Results for commit 3ce2a43. ♻️ This comment has been updated with latest results. |
| local function init_state_handler(driver, device, value, zb_rx) | ||
| device:set_field(INIT_STATE_REPORTED, true, { persist = true }) | ||
| local state = value.value == 0x02 and initializedStateWithGuide.initializedStateWithGuide.initialized() or | ||
| initializedStateWithGuide.initializedStateWithGuide.notInitialized() |
There was a problem hiding this comment.
nit: indent to show it is a part of the line above
|
Invitation URL: |
| local INIT_STATE_ATTRIBUTE_ID = 0x0407 | ||
| local DIRECTION_ATTRIBUTE_ID = 0x0400 | ||
|
|
||
| local ROTATE_UP_VALUE = 0x0006 |
There was a problem hiding this comment.
This is 0x6 instead of 0x4 (what the base aqara roller shade driver has. Is that intentional, or should they match for a consistent experience?
There was a problem hiding this comment.
Yes, this is intentional. According to the manufacturer's technical guide, the T1S model requires the value to be 0x6. We have also verified this expected behavior using the physical test samples.
| local level = command.args.shadeLevel > 100 and 100 or utils.round(command.args.shadeLevel) | ||
| device:emit_event(capabilities.windowShadeLevel.shadeLevel(level)) | ||
| device:send_to_component(command.component, WindowCovering.server.commands.GoToLiftPercentage(device, level)) |
There was a problem hiding this comment.
This is the same as aqara_utils.shade_level_cmd, which should be used instead.
There was a problem hiding this comment.
I will update the code to use that function.
| local level = value.value > 100 and 100 or utils.round(value.value) | ||
| device:emit_event(capabilities.windowShadeLevel.shadeLevel(level)) |
There was a problem hiding this comment.
This is the same as aqara_utils.emit_shade_level_event which should be used to avoid duplicated code.
There was a problem hiding this comment.
I will replace it with the utility function.
| -- 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.
| local raw_value = reverseRollerShadeDirPrefValue | ||
| device:send(cluster_base.write_manufacturer_specific_attribute(device, aqara_utils.PRIVATE_CLUSTER_ID, | ||
| DIRECTION_ATTRIBUTE_ID, aqara_utils.MFG_CODE, data_types.Boolean, raw_value)) |
There was a problem hiding this comment.
| local raw_value = reverseRollerShadeDirPrefValue | |
| device:send(cluster_base.write_manufacturer_specific_attribute(device, aqara_utils.PRIVATE_CLUSTER_ID, | |
| DIRECTION_ATTRIBUTE_ID, aqara_utils.MFG_CODE, data_types.Boolean, raw_value)) | |
| device:send(cluster_base.write_manufacturer_specific_attribute(device, aqara_utils.PRIVATE_CLUSTER_ID, | |
| DIRECTION_ATTRIBUTE_ID, aqara_utils.MFG_CODE, data_types.Boolean, reverseRollerShadeDirPrefValue)) |
There was a problem hiding this comment.
Applied the suggested change.
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests