Add step functionality to Zigbee curtains using Common Handler.#3108
Add step functionality to Zigbee curtains using Common Handler.#3108zhangshen001 wants to merge 4 commits into
Conversation
|
zs seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
| end | ||
|
|
||
| local function shade_level_report_handler(driver, device, value, zb_rx) | ||
| local latest_target_level = device:get_field(LATEST_TARGET_LEVEL) |
There was a problem hiding this comment.
Here what we can get is current position of window treatment, right?
There was a problem hiding this comment.
The common driver only clears the "LATEST_TARGET_LEVEL" state when the device reports its location and does not handle report-level events. The child driver is responsible for report-level processing and UI events. The report handlers of both drivers will be triggered and will not affect each other.
There was a problem hiding this comment.
Since the curtain position status has been updated, the target value is no longer used as a reference for position offset calculation, so LATEST_TARGET_LEVEL needs to be set to null.
| device:get_latest_state("main", capabilities.windowShadeLevel.ID, capabilities.windowShadeLevel.shadeLevel.NAME) or 0 | ||
|
|
||
| -- Calculate UI target_level (user's expected percentage) | ||
| local ui_target_level= utils.clamp_value(current_level + step, 0, 100) |
| { | ||
| name = "aqara", | ||
| mfr = "LUMI", | ||
| models = { "lumi.curtain", "lumi.curtain.v1", "lumi.curtain.aq2", "lumi.curtain.agl001", "lumi.curtain.acn002" }, |
| invert_level = true, | ||
| }, | ||
| { | ||
| name = "invert-lift-percentage", |
There was a problem hiding this comment.
If we must need these three parts, please consider to merge them
| -- Brand-specific configuration for window treatment devices | ||
| -- Supports statelessWindowShadeLevelStep capability | ||
|
|
||
| local BRAND_CONFIGS = { |
There was a problem hiding this comment.
Name file and variable using fingerprints
b89ff06 to
3573329
Compare
| -- Field keys for tracking target level and timeout timer | ||
| local LATEST_TARGET_LEVEL = "_latest_target_level" | ||
|
|
||
| -- Get brand configuration for a device |
There was a problem hiding this comment.
It's better to use 'fingerprint' into function name and comments
| -- Step shade level handler for statelessWindowShadeLevelStep capability | ||
| local function step_shade_level_handler(driver, device, command) | ||
| -- Get brand-specific configuration | ||
| local brand_config = get_brand_config(device) |
| end | ||
|
|
||
| local function shade_level_report_handler(driver, device, value, zb_rx) | ||
| local latest_target_level = device:get_field(LATEST_TARGET_LEVEL) |
There was a problem hiding this comment.
merge into one sentence and explain why it is required
ef2520e to
8acd43e
Compare
| local current_level = latest_target_level or | ||
| device:get_latest_state("main", capabilities.windowShadeLevel.ID, capabilities.windowShadeLevel.shadeLevel.NAME) or 0 |
There was a problem hiding this comment.
For example, if the curtain is initially at level 30 and receives a +10 command, setting the target_level to 40, and then receives a +2 command when the actual curtain position reaches 35, the current_level might still be 30. If the current_level were used as the baseline and the target_level were set to 32, the motor would reverse. Therefore, the target_level is calculated based on the saved LATEST_TARGET_LEVEL and is set to 42.
There was a problem hiding this comment.
Please consider the following situation:
LATEST_TARGET_LEVEL and is set to 42, in the way you described. Then, the user starts turning the knob the other direction before the target level reaches 42.
In the current layout, the user will have to scroll back far to actually have the device start moving in the opposite direction. I believe the device should begin moving the opposite direction immediately.
There was a problem hiding this comment.
When the user reverses the knob direction before level 42 is reached, the new target level is immediately recalculated and sent to the device for execution.
There was a problem hiding this comment.
I'm not sure that's true, and if it is, it seems like it is only because the LATEST TARGET LEVEL is made nil whenever a Current Window value is received, which seems like it would mess with the LATEST TARGET LEVEL incrementing.
| name = "invert-lift-percentage", | ||
| mfrs = { "IKEA of Sweden", "Smartwings", "Insta GmbH" }, | ||
| models = {}, | ||
| invert_level = true, |
There was a problem hiding this comment.
For the Ikea blind, 100% means fully open.
But based on your comment, it seems to be the opposite:
-- invert_level = true: Device reports inverted values (0 = open, 100 = closed)
Would you please double check?
There was a problem hiding this comment.
The comment description is inaccurate; we will change it to:
-- invert_level = true:The level values on the device side and the driver side need to be inverted.
| -- Standard devices (no special handling needed) | ||
| -- Use WindowCovering.GoToLiftPercentage with 0-100 percentage | ||
| { | ||
| name = "rooms-beautiful", |
There was a problem hiding this comment.
don't you need to add invert_level = true for this model?
There was a problem hiding this comment.
We have fixed this issue, please review.
| -- Window Covering cluster: handles standard curtain/blind devices | ||
| -- (e.g., Vimar: Window_Cov_v1.0/Window_Cov_Module_v1.0, SOMFY: Glydea Ultra Curtain/Sonesse series, | ||
| -- IKEA: KADRILJ/FYRTUR, Smartwings: WM25/L-Z, Insta GmbH: NEXENTRO Blinds Actuator, | ||
| -- Yookee: D10110, Rooms Beautiful: C001, Screen Innovations: WM25/L-Z) |
There was a problem hiding this comment.
below two test cases in test_zigbee_window_treatment.lua are failed.
Running test "State transnsition from opening to partially open" (1 of 9)
FAILED
Running test "State transnsition from opening to closing" (2 of 9)
FAILED
NodOn, REXENSE, Shared Revolution devices are using zigbee windowShade_default logic in default_current_lift_percentage_handler.lua
if You add the handler for CurrentPositionLiftPercentage here. the devices do nothing when zigbee message for CurrentPositionLiftPercentage comes
I wonder what your intention is?
There was a problem hiding this comment.
We have fixed this issue, please review.
| -- Brand-specific configuration for window treatment devices | ||
| -- Supports statelessWindowShadeLevelStep capability | ||
|
|
||
| local FINGERPRINTS = { |
There was a problem hiding this comment.
we don't need to support Sombra Shades. right?
Can you share the device list to support statelessWindowShadeLevelStep?
There was a problem hiding this comment.
Devices that support statelessWindowShadeLevelStep are all those listed in the following file, including Aqara, axis, feibit, hanssem, invert-lift-percentage, rooms-beautiful, screen-innovations, somfiy, vimar, and yoolax.
Hoposmart and VIVIDSTORM devices are not supported. (SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/fingerprints.yml)
There was a problem hiding this comment.
If so, why sombra shades aren't included in this FINGERPRINTS?
- id: "Sombra Shades/WM25/L-Z"
deviceLabel: Sombra Shades Window Treatment
manufacturer: Sombra Shades
model: WM25/L-Z
deviceProfileName: window-treatment-battery
- id: "Sombra Shades/SS25/L-Z"
deviceLabel: Sombra Automated Shades and Blinds
manufacturer: Sombra Shades
model: SS25/L-Z
deviceProfileName: window-treatment-battery
- id: "Sombra Shades/SOMBRA/Z-M"
deviceLabel: Sombra Automated Shades
manufacturer: Sombra Shades
model: SOMBRA/Z-M
deviceProfileName: window-treatment-powerSource
There was a problem hiding this comment.
Because the Sombra driver implementation wasn't available in SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src when we were trying to implement stateless_handler, we didn't add it.We will add it.
| { | ||
| name = "aqara", | ||
| mfr = "LUMI", | ||
| models = { "lumi.curtain", "lumi.curtain.v1", "lumi.curtain.aq2", "lumi.curtain.agl001" }, |
There was a problem hiding this comment.
lumi.curtain.agl001 is using window-treatment-aqara-curtain-driver-e1 profile.
but profile doesn't have statelessWindowShadeLevelStep
including lumi.curtain.agl001 model in here. right?
There was a problem hiding this comment.
Because the curtain-driver-e1 driver does not implement windowShadeLevel, we assume it also does not support statelessWindowShadeLevelStep, and therefore we are removing lumi.curtain.agl001 from the support list.
There was a problem hiding this comment.
We have fixed this issue, please review.
| local capabilities = require "st.capabilities" | ||
|
|
||
| return function(opts, driver, device) | ||
| local can_handle = device:supports_capability(capabilities.statelessWindowShadeLevelStep) |
There was a problem hiding this comment.
can you check compatibility with hubcore with not added statelessWindowShadeLevelStep?
I guess capabilities.statelessWindowShadeLevelStep statement would be error.
There was a problem hiding this comment.
We conducted tests. No impact.
|
|
||
| for _, config in ipairs(FINGERPRINTS) do | ||
| -- Check if this config matches by model only (match_by_model = true) | ||
| if config.match_by_model then |
There was a problem hiding this comment.
Why do you check only two model name via match_by_model?
TS0601 and WM25/L-Z.
As far as I know TS0601 is used for model name of other tuya devices (not only window shade). it is not unique name.
And there are the two devices in fingerprints.yml. same model name, different device.
- id: "Smartwings/WM25/L-Z"
deviceLabel: Smartwings Window Treatment
manufacturer: Smartwings
model: WM25/L-Z
deviceProfileName: window-treatment-battery
- id: "ScreenInnovations/WM25/L-Z"
deviceLabel: Screen Innovations Window Treatment
manufacturer: Screen Innovations
model: WM25/L-Z
deviceProfileName: window-treatment-powerSource
Perhaps, there might be some issues.
I wonder why you don't use model name and manufacturer to check those devices.
There was a problem hiding this comment.
We have fixed this issue, please review.
it seems that above devices are using below two sub-drivers. invert-lift-percentage screen-innovations can you check this? it is possible or bug. |
cefaa07 to
7e62881
Compare
3de945d to
4818175
Compare
| { | ||
| name = "sombra-shades", | ||
| mfr = "Sombra Shades", | ||
| models = { "SOMBRA/Z-M" }, |
There was a problem hiding this comment.
Because the Sombra driver's fingerprints only contain SOMBRA/Z-M, we only added this one.
|
I think the I have created a branch here that attempts to solve some of the issues the fingerprints file tries to solve, without the need for as much manual intervention. One thing I did was to change the stateless step handler to look more like this: which uses the The primary issue with this injection method, with the way the subdrivers are currently laid out, is that the "inversion" does not work naturally for both regular and inverted rotation devices. To get around this, I moved all of the inverted subdrivers under the general inverted subdriver, like this. This then allows all inverted devices to simply use this handler (with inverted set to true). Also, I do not see the benefit of this handler you've added: In its current form, it seems to only set the latest target to nil (which is undesirable since we may receive this value before the target is reached, or the user stops scrolling. As can be seen in my above stateless handler, I rather think that we should keep an internal timer for the target state, and set that to nil after a short period (aka how long we expect the user to be attempting one continuous scroll). This is effectively equivalent. Please check out the changes in my branch, which I have committed directly on top of this one, for more detail on how I imagine this may look. Thanks! |
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 73 files 538 suites 0s ⏱️ For more details on these errors, see this check. Results for commit 4818175. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 4818175 |
…dow-shade-step Handle Zigbee Window Stateless Step Commands
Hi @hcarter-775 Thank you very much for your detailed reivewing, we have updated the code & TCs according to your comments, please reivew, |
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests