Skip to content

Add step functionality to Zigbee curtains using Common Handler.#3108

Open
zhangshen001 wants to merge 4 commits into
SmartThingsCommunity:mainfrom
zhangshen001:zigbee_V2
Open

Add step functionality to Zigbee curtains using Common Handler.#3108
zhangshen001 wants to merge 4 commits into
SmartThingsCommunity:mainfrom
zhangshen001:zigbee_V2

Conversation

@zhangshen001

@zhangshen001 zhangshen001 commented Jul 9, 2026

Copy link
Copy Markdown

Check all that apply

Type of Change

  • WWST Certification Request
    • If this is your first time contributing code:
      • I have reviewed the README.md file
      • I have reviewed the CODE_OF_CONDUCT.md file
      • I have signed the CLA
    • I plan on entering a WWST Certification Request or have entered a request through the WWST Certification console at developer.smartthings.com
  • Bug fix
  • New feature
  • Refactor

Checklist

  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have verified my changes by testing with a device or have communicated a plan for testing
  • I am adding new behavior, such as adding a sub-driver, and have added and run new unit tests to cover the new behavior

Description of Change

Summary of Completed Tests

  • Co-Authored-By: Cline SR (50%)
  • CGAI & AI Review

@CLAassistant

CLAassistant commented Jul 9, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ zhangshen001
✅ hcarter-775
❌ zs


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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here what we can get is current position of window treatment, right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IS the variable name reasonable?

{
name = "aqara",
mfr = "LUMI",
models = { "lumi.curtain", "lumi.curtain.v1", "lumi.curtain.aq2", "lumi.curtain.agl001", "lumi.curtain.acn002" },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is 'lumi.curtain.acn002' from?

invert_level = true,
},
{
name = "invert-lift-percentage",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name file and variable using fingerprints

@zhangshen001 zhangshen001 force-pushed the zigbee_V2 branch 2 times, most recently from b89ff06 to 3573329 Compare July 10, 2026 04:44
-- Field keys for tracking target level and timeout timer
local LATEST_TARGET_LEVEL = "_latest_target_level"

-- Get brand configuration for a device

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

end

local function shade_level_report_handler(driver, device, value, zb_rx)
local latest_target_level = device:get_field(LATEST_TARGET_LEVEL)

@vangoran vangoran Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge into one sentence and explain why it is required

@zhangshen001 zhangshen001 force-pushed the zigbee_V2 branch 2 times, most recently from ef2520e to 8acd43e Compare July 10, 2026 07:31
Comment on lines +95 to +96
local current_level = latest_target_level or
device:get_latest_state("main", capabilities.windowShadeLevel.ID, capabilities.windowShadeLevel.shadeLevel.NAME) or 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @partner-hub-chenxuelong ,

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hcarter-775 hcarter-775 Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@zhangshen001 zhangshen001 marked this pull request as ready for review July 10, 2026 08:29
@tpmanley tpmanley requested review from hcarter-775 and inasail July 10, 2026 21:44
name = "invert-lift-percentage",
mfrs = { "IKEA of Sweden", "Smartwings", "Insta GmbH" },
models = {},
invert_level = true,

@inasail inasail Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@partner-hub-chenxuelong partner-hub-chenxuelong Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you need to add invert_level = true for this model?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we will add it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@kdbang kdbang Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have fixed this issue, please review.

-- Brand-specific configuration for window treatment devices
-- Supports statelessWindowShadeLevelStep capability

local FINGERPRINTS = {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need to support Sombra Shades. right?
Can you share the device list to support statelessWindowShadeLevelStep?

@partner-hub-chenxuelong partner-hub-chenxuelong Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" },

@kdbang kdbang Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@kdbang kdbang Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you check compatibility with hubcore with not added statelessWindowShadeLevelStep?
I guess capabilities.statelessWindowShadeLevelStep statement would be error.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kdbang kdbang Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have fixed this issue, please review.

@kdbang

kdbang commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator
  - id: "Smartwings/WM25/L-Z"
    deviceLabel: Smartwings Window Treatment
    manufacturer: Smartwings
    model: WM25/L-Z
    deviceProfileName: window-treatment-battery

it seems that above devices are using below two sub-drivers.

invert-lift-percentage

local function invert_lift_percentage_can_handle(opts, driver, device, ...)
  if device:get_manufacturer() == "IKEA of Sweden" or
    device:get_manufacturer() == "Smartwings" or
    device:get_manufacturer() == "Insta GmbH"
  then
      return true, require("invert-lift-percentage")
  end
  return false
end

screen-innovations

local function screen_innovations_can_handle(opts, driver, device, ...)
  if device:get_model() == "WM25/L-Z" then
    return true, require("screen-innovations")
  end
  return false
end

can you check this? it is possible or bug.

@zhangshen001 zhangshen001 force-pushed the zigbee_V2 branch 2 times, most recently from cefaa07 to 7e62881 Compare July 13, 2026 10:19
@zhangshen001 zhangshen001 force-pushed the zigbee_V2 branch 3 times, most recently from 3de945d to 4818175 Compare July 13, 2026 11:48
{
name = "sombra-shades",
mfr = "Sombra Shades",
models = { "SOMBRA/Z-M" },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the Sombra driver's fingerprints only contain SOMBRA/Z-M, we only added this one.

@hcarter-775

Copy link
Copy Markdown
Contributor

I think the fingerprints.lua structure you have here has some issues as far as maintainability and ensuring that all devices are properly handled.

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:

-- Step shade level handler for statelessWindowShadeLevelStep capability
utils.step_shade_level_handler = function(invert_shade_level)
  return function(driver, device, command)
    local step = command.args.stepSize or 0
    if step == 0 then return
    elseif invert_shade_level then step = -step end -- invert step's direction if invert_shade_level is true

    -- Step from the latest target level if it exists, or from the current shade level
    local latest_target_level = device:get_field(LATEST_TARGET_LEVEL) or
      device:get_latest_state("main", capabilities.windowShadeLevel.ID, capabilities.windowShadeLevel.shadeLevel.NAME, 0)
    local new_target_level = st_utils.clamp_value(latest_target_level + step, 0, 100)

    -- Cancel any previous timer and set a 3 second timeout timer to ensure target_level is cleared
    -- This is to prevent the stateless step from accumulating indefinitely, while ensuring that a single
    -- "scroll" action can accumulate multiple steps within a short time frame
    if device:get_field(TARGET_LEVEL_TIME_OUT) then
      device.thread:cancel_timer(device:get_field(TARGET_LEVEL_TIME_OUT))
    end
    local timer = device.thread:call_with_delay(TARGET_LEVEL_TIME_OUT_SECONDS, function(d)
      device:set_field(LATEST_TARGET_LEVEL, nil)
    end)
    device:set_field(TARGET_LEVEL_TIME_OUT, timer)
    device:set_field(LATEST_TARGET_LEVEL, new_target_level)

    driver:inject_capability_command(device, {
      capability = capabilities.windowShadeLevel.ID,
      component = command.component,
      command = capabilities.windowShadeLevel.commands.setShadeLevel.NAME,
      named_args = { shadeLevel = new_target_level }
    })
  end
end

which uses the inject_capability_command helper. This will allow us to explicitly use each subdriver's logic without having to re-write it all from scratch, which is error-prone and non maintainable.

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:

local function shade_level_report_handler(driver, device, value, zb_rx)
...
end

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!

@github-actions

Copy link
Copy Markdown

Duplicate profile check: Passed - no duplicate profiles detected.

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

Test Results

   73 files    538 suites   0s ⏱️
3 186 tests 3 177 ✅ 0 💤 0 ❌ 9 🔥
5 187 runs  5 178 ✅ 0 💤 0 ❌ 9 🔥

For more details on these errors, see this check.

Results for commit 4818175.

@github-actions

Copy link
Copy Markdown

File Coverage
All files 93%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/aqara/version/can_handle.lua 83%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/VIVIDSTORM/init.lua 93%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/aqara/aqara_utils.lua 93%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/aqara/init.lua 90%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/hanssem/init.lua 91%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/yoolax/init.lua 97%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/axis/axis_version/init.lua 97%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/window_shade_utils.lua 88%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/lazy_load_subdriver.lua 57%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/stateless_handler/can_handle.lua 83%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/stateless_handler/init.lua 20%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/sombra/init.lua 95%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-window-treatment/src/aqara/curtain-driver-e1/init.lua 92%

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 4818175

@hongye-samsung

Copy link
Copy Markdown

I think the fingerprints.lua structure you have here has some issues as far as maintainability and ensuring that all devices are properly handled.

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:

-- Step shade level handler for statelessWindowShadeLevelStep capability
utils.step_shade_level_handler = function(invert_shade_level)
  return function(driver, device, command)
    local step = command.args.stepSize or 0
    if step == 0 then return
    elseif invert_shade_level then step = -step end -- invert step's direction if invert_shade_level is true

    -- Step from the latest target level if it exists, or from the current shade level
    local latest_target_level = device:get_field(LATEST_TARGET_LEVEL) or
      device:get_latest_state("main", capabilities.windowShadeLevel.ID, capabilities.windowShadeLevel.shadeLevel.NAME, 0)
    local new_target_level = st_utils.clamp_value(latest_target_level + step, 0, 100)

    -- Cancel any previous timer and set a 3 second timeout timer to ensure target_level is cleared
    -- This is to prevent the stateless step from accumulating indefinitely, while ensuring that a single
    -- "scroll" action can accumulate multiple steps within a short time frame
    if device:get_field(TARGET_LEVEL_TIME_OUT) then
      device.thread:cancel_timer(device:get_field(TARGET_LEVEL_TIME_OUT))
    end
    local timer = device.thread:call_with_delay(TARGET_LEVEL_TIME_OUT_SECONDS, function(d)
      device:set_field(LATEST_TARGET_LEVEL, nil)
    end)
    device:set_field(TARGET_LEVEL_TIME_OUT, timer)
    device:set_field(LATEST_TARGET_LEVEL, new_target_level)

    driver:inject_capability_command(device, {
      capability = capabilities.windowShadeLevel.ID,
      component = command.component,
      command = capabilities.windowShadeLevel.commands.setShadeLevel.NAME,
      named_args = { shadeLevel = new_target_level }
    })
  end
end

which uses the inject_capability_command helper. This will allow us to explicitly use each subdriver's logic without having to re-write it all from scratch, which is error-prone and non maintainable.

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:

local function shade_level_report_handler(driver, device, value, zb_rx)
...
end

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!

Hi @hcarter-775 Thank you very much for your detailed reivewing, we have updated the code & TCs according to your comments, please reivew,
Besides, we have some questions, could you please help us to check? Many thanks!
04af865

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants