Use parameter matching instead of `args` for all lighting commands #1474

puddly · 2022-04-08T22:07:29Z

Depends on zigpy/zigpy#953 for fixing a command name typo.

Should supersede #1471 (since the extra , 0, 0 arguments depend on the firmware version) and also fixes a small bug introduced in #1472

This PR introduces unit tests that mandate the use of PARAMS instead of ARGS for all quirks that use level control or color changing commands. In the future, all of the command name string constants in quirks will be replaced with command schemas to ensure there is only place where commands and their parameters are defined.

MattWestb · 2022-04-09T06:42:39Z

Great work @puddly !!
Do you need testing all the quirks or do you feel comfortable with it ??

I have all IKEA devices plus the 2 tuya TS004F if needed but taking some time then must "rotating" little then some is only in production and other is in test setups (4 different systems).

One more time GREAT WORK !!

MattWestb · 2022-04-09T12:38:58Z

PARAMS = "params" is missing in zhaquirks/const.py then testing some of the updated quirks with HA 2022.4.1.

dmulcahey · 2022-04-09T12:45:54Z

PARAMS = "params" is missing in zhaquirks/const.py then testing some of the updated quirks with HA 2022.4.1.

Looks like it’s in the PR to me?

MattWestb · 2022-04-09T12:50:13Z

You is very true if looking more its updated in the const.py that i was missing.

Do you like have quirks testing with device before merging it the the test is fixed ?

dmulcahey · 2022-04-09T12:51:29Z

You is very true if looking more its updated in the const.py that i was missing.

Do you like have quirks testing with device before merging it the the test is fixed ?

If you want to test the ones you were checking yesterday we would definitely appreciate it.

MattWestb · 2022-04-09T12:56:35Z

I must patching my HA container for the missing parms and then i can do some but its taking time setting up automatons for all scenarios an testing them the TS004F is having nearly 15 and 20 DA in the 2 device classes.

Perhaps better doing some IKEA that is most used in our systems.

dmulcahey · 2022-04-09T12:59:25Z

I must patching my HA container for the missing parms and then i can do some but its taking time setting up automatons for all scenarios an testing them the TS004F is having nearly 15 and 20 DA in the 2 device classes.

Perhaps better doing some IKEA that is most used in our systems.

Completely up to you

MattWestb · 2022-04-09T13:53:33Z

TS004F tested and little more work for our Puddly ;-(((

ROK
(LONG_PRESS, BUTTON)
{
    "event_type": "zha_event",
    "data": {
        "device_ieee": "00:3c:84:ff:fe:b1:5b:b3",
        "unique_id": "00:3c:84:ff:fe:b1:5b:b3:1:0x0300",
        "device_id": "cf4d64a8db61e2dd336a30f6ae769553",
        "endpoint_id": 1,
        "cluster_id": 768,
        "command": "move_saturation",
        "args": [
            1,
            200
        ],
        "params": {
            "move_mode": 1,
            "rate": 200,
            "options_mask": null,
            "options_override": null
        }
    },

DA:
        (LONG_PRESS, BUTTON): {
            COMMAND: COMMAND_MOVE_SATURATION,
            ENDPOINT_ID: 1,
            CLUSTER_ID: 768,
-            ARGS: [1, 200],
+            PARAMS: {"saturation": 1, "transition_time": 200},
        },

(ROTATED_SLOW, RIGHT) OK
(ROTATED_SLOW, LEFT) OK
(ROTATED_FAST, RIGHT) OK
(ROTATED_FAST, LEFT) OK


DMS
(SHORT_PRESS, DIM_UP) OK
(LONG_PRESS, DIM_UP) OK
(SHORT_PRESS, DIM_DOWN) OK
(LONG_PRESS, DIM_DOWN) OK

zhaquirks/tuya/ts004f.py

dmulcahey · 2022-04-09T14:19:41Z

just a note to kick

zha-device-handlers/setup.py

Line 23 in d660948

install_requires=["zigpy>=0.44.1"],

in this PR once the Zigpy release happens (more so I don't forget to check)

MattWestb · 2022-04-09T14:37:48Z

IkeaTradfriRemote3: Somthing strange and i have tipple checking i have made the automatons OK

(SHORT_PRESS, DIM_UP) OK
(LONG_PRESS, DIM_UP) OK
(SHORT_PRESS, DIM_DOWN)
{
    "event_type": "zha_event",
    "data": {
        "device_ieee": "00:0d:6f:ff:fe:51:43:5c",
        "unique_id": "00:0d:6f:ff:fe:51:43:5c:1:0x0008",
        "device_id": "ff32ddeaf5d0c3d2041d3614dd6da3e1",
        "endpoint_id": 1,
        "cluster_id": 8,
        "command": "step",
        "args": [
            1,
            43,
            5,
            0,
            0
        ],
        "params": {
            "step_mode": 1,
            "step_size": 43,
            "transition_time": 5,
            "options_mask": 0,
            "options_override": 0
        }
    },
(LONG_PRESS, DIM_DOWN)
{
    "event_type": "zha_event",
    "data": {
        "device_ieee": "00:0d:6f:ff:fe:51:43:5c",
        "unique_id": "00:0d:6f:ff:fe:51:43:5c:1:0x0008",
        "device_id": "ff32ddeaf5d0c3d2041d3614dd6da3e1",
        "endpoint_id": 1,
        "cluster_id": 8,
        "command": "move",
        "args": [
            1,
            84,
            0,
            0
        ],
        "params": {
            "move_mode": 1,
            "rate": 84,
            "options_mask": 0,
            "options_override": 0
        }
    },

Is 0 not the same as None ?

zigpy#1474 (comment)

puddly · 2022-04-09T14:40:57Z

Is 0 not the same as None ?

My mistake, those should have been deleted. I pushed a few commits that should resolve both of the issues you found. Thank you for testing out the changes!

~~Or maybe not, failing unit tests...~~

You may need to apply the setp_mode -> step_mode patch from zigpy if that doesn't work: https://github.com/zigpy/zigpy/pull/953/files

MattWestb · 2022-04-09T14:44:10Z

Thanks P and the software tests is not my problem ( ;-)) )i trying little more "hardware" testing later after doing dinner !!

MattWestb · 2022-04-09T14:58:14Z

IkeaTradfriRemote3: (5Bt latest firmware)

(SHORT_PRESS, DIM_UP) OK
(LONG_PRESS, DIM_UP) OK
(SHORT_PRESS, DIM_DOWN) OK
(LONG_PRESS, DIM_DOWN) OK

Is very Puddly !!

MattWestb · 2022-04-09T15:10:28Z

fourbtnremote:

(LONG_PRESS, DIM_UP) OK
(LONG_PRESS, DIM_DOWN) OK
(LONG_RELEASE, DIM_DOWN)
{
    "event_type": "zha_event",
    "data": {
        "device_ieee": "84:2e:14:ff:fe:5f:89:0e",
        "unique_id": "84:2e:14:ff:fe:5f:89:0e:1:0x0008",
        "device_id": "14c00f9f7f350057a29c75268ff7ad88",
        "endpoint_id": 1,
        "cluster_id": 8,
        "command": "stop_with_on_off",
        "args": [],
        "params": {}
    },
    "origin": "LOCAL",
    "time_fired": "2022-04-09T15:08:37.656212+00:00",
    "context": {
        "id": "2cb5a02bf527cf0ec2ede892d1287e82",
        "parent_id": null,
        "user_id": null
    }
}

puddly · 2022-04-09T15:16:39Z

Hmm, that's unexpected. So (LONG_RELEASE, DIM_UP) and (LONG_RELEASE, DIM_DOWN) have the same trigger?

MattWestb · 2022-04-09T15:24:35Z

Its only one command for stop after move. The Open/Close is having one code for "remember" the direction and tagging the stop with the last direction.

MattWestb · 2022-04-09T15:31:03Z

fourbtnremote:

(LONG_PRESS, DIM_UP) OK
(LONG_PRESS, DIM_DOWN) OK
(LONG_RELEASE, DIM_DOWN) OK

Is the setp patch needed for the TS004F then i cant getting working OK ?

puddly · 2022-04-09T15:35:00Z

Yes. Any event that uses step_mode will fail because zigpy has a typo and instead sends setp_mode. You should be able to install zigpy@dev without any issues, it has only minor fixes so far.

MattWestb · 2022-04-09T15:56:47Z

Was patching the HA only with the setp and trying the latest TS004F for the ROK and i still getting and no DA triggered:

{
    "event_type": "zha_event",
    "data": {
        "device_ieee": "00:3c:84:ff:fe:b1:5b:b3",
        "unique_id": "00:3c:84:ff:fe:b1:5b:b3:1:0x0300",
        "device_id": "cf4d64a8db61e2dd336a30f6ae769553",
        "endpoint_id": 1,
        "cluster_id": 768,
        "command": "move_saturation",
        "args": [
            1,
            200
        ],
        "params": {
            "move_mode": 1,
            "rate": 200,
            "options_mask": null,
            "options_override": null
        }
    },

And the DA is looking like:

        (LONG_PRESS, BUTTON): {
            COMMAND: COMMAND_MOVE_SATURATION,
            ENDPOINT_ID: 1,
            CLUSTER_ID: 768,
            PARAMS: {"move_mode": 1, "rate": 200},
        },

And the automation:

alias: 4F (LONG_PRESS, BUTTON)
description: ''
trigger:
  - device_id: cf4d64a8db61e2dd336a30f6ae769553
    domain: zha
    platform: device
    type: remote_button_long_press
    subtype: button
condition: []
action:
  - event: zha_event
    event_data:
      BUTTON: LONG_PRESSed
mode: single

MattWestb · 2022-04-09T16:05:48Z

SYMFONISK:

(ROTATED, RIGHT) OK
(ROTATED, LEFT) OK
(DOUBLE_PRESS, TURN_ON) OK
(TRIPLE_PRESS, TURN_ON) OK

MattWestb · 2022-04-09T16:18:40Z

SHORTCUT:

(LONG_PRESS, DIM_UP) OK

Wireless Dimmer:

(ROTATED, RIGHT)
{
    "event_type": "zha_event",
    "data": {
        "device_ieee": "00:0b:57:ff:fe:8a:68:e9",
        "unique_id": "00:0b:57:ff:fe:8a:68:e9:1:0x0008",
        "device_id": "cdabcfbefe0e732306adf293fe2c6c59",
        "endpoint_id": 1,
        "cluster_id": 8,
        "command": "move_with_on_off",
        "args": [
            0,
            195
        ],
        "params": {
            "move_mode": 0,
            "rate": 195
        }
    },

DA:
        (ROTATED, RIGHT): {
            COMMAND: COMMAND_MOVE,
            CLUSTER_ID: 8,
            ENDPOINT_ID: 1,
            ARGS: [0, 195],
            PARAMS: {"move_mode": 0, "rate": 195},
        },
(ROTATED, LEFT) OK

Right is with OnOff ;-((

MattWestb · 2022-04-09T16:54:17Z

Last device i is having 2 Button (only in production so its also patched).

(LONG_PRESS, DIM_UP) OK
(LONG_PRESS, DIM_DOWN) OK

All locks good !!

I have only tested the updated DA then all other was looking OK yesterday and i think its no need testing it with DA made Automatons and its pretty time consuming doing it for all devices.
I have not testing the zhaquirks/ikea/fivebtnremote.py then its for 5 Button remotes with very old ZLL firmware that i dont have but can flashing with SWD but its better user is updating there devices to production version and getting ZB3 functionality.

One more thanks to our P for ZCLR7/8 and what is bringing in the future (long wanted and very much work done) !!

puddly · 2022-04-09T17:03:34Z

Thanks you for all the testing, I appreciate it!

Regarding #1474 (comment):

I recently renamed COMMAND_MOVE_SATURATION from "move_to_saturation" to "move_saturation" so it looks the constant may be wrong in your installation, since the event otherwise matches identically.

Are you manually patching quirks or are you installing it straight from this PR?

pip install git+https://github.com/puddly/zha-device-handlers.git@puddly/hotfix-triggers-args-to-params

MattWestb · 2022-04-09T17:10:32Z

The test HA with most IKEA controllers is contained HA 2022.4.1 under window and i was copy all quirks from your GIT by downloading one Zip to custom quirk folder and then updating them then you was updating them.

If the renaming of the COMMAND_MOVE_SATURATION in the quirk or is it in one other file ?
If i knowing where it is i can patching it.

MattWestb · 2022-04-09T17:18:21Z

WD updated:

{
    "event_type": "zha_event",
    "data": {
        "ROTATED": "LEFT"
    },
    "origin": "LOCAL",
    "time_fired": "2022-04-09T17:16:12.731646+00:00",
    "context": {
        "id": "e821d0409e53c4ce068a00f27120c53b",
        "parent_id": "60e2b2125d2de9477b626dfc03f573bc",
        "user_id": null
    }
}


{
    "event_type": "zha_event",
    "data": {
        "ROTATED": "RIGHT"
    },
    "origin": "LOCAL",
    "time_fired": "2022-04-09T17:16:09.022434+00:00",
    "context": {
        "id": "0ad41ad0943cdffdd225791256645cce",
        "parent_id": "8ec5c2d7edcaa3b0c37672f23f0418e0",
        "user_id": null
    }
}

And both DA rotating is working OK !!

MattWestb · 2022-04-09T17:28:56Z

Patched COMMAND_MOVE_SATURATION and now the DA for TS004F ROK is working :-)))

{
    "event_type": "zha_event",
    "data": {
        "BUTTON": "LONG_PRESSed"
    },
    "origin": "LOCAL",
    "time_fired": "2022-04-09T17:26:01.904280+00:00",
    "context": {
        "id": "e5b2d51880cb2954a92e38980525a349",
        "parent_id": "d1b5e755c912354b3fe69c364dd716c9",
        "user_id": null
    }
}

But im missing the error in the log from my IKEA remotes that was sending extra information that is gone ;-)

zigpy#1474 (comment)

This version of zigpy fixes typos that prevent quirks from functioning

puddly · 2022-04-11T16:14:21Z

@dmulcahey I've bumped the minimum required zigpy version to 0.42.2, matching the latest zigpy release. @MattWestb tested just about every affected device so I think this is good to go!

MattWestb · 2022-04-11T16:32:28Z

If some other device is not tested and not working or i have missing some i think its better to going with it so user can start using the devices with automatons and fixing if some is coming after that was missed.

Very sad user is using light controllers for HA automatons ans not as light controllers but IKEA is easy to getting and easy to using for normal users.

Great work @puddly and also @dmulcahey for support for making it working !!!

And now our system is running the ZCLR7.5 :-)))

coveralls · 2022-04-11T17:18:39Z

Pull Request Test Coverage Report for Build 2149729312

6 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.006%) to 80.37%

Totals
Change from base Build 2145199523:	0.006%
Covered Lines:	5388
Relevant Lines:	6704

💛 - Coveralls

…igpy#1474)

dmulcahey reviewed Apr 9, 2022

View reviewed changes

zhaquirks/tuya/ts004f.py Outdated Show resolved Hide resolved

puddly added a commit to puddly/zha-device-handlers that referenced this pull request Apr 9, 2022

Remove "options_mask": None, that was accidentally left in

d4e7330

zigpy#1474 (comment)

MattWestb mentioned this pull request Apr 10, 2022

ikea two button dim down longpress not working home-assistant/core#69765

Closed

Migrate ARGS to PARAMS for all lighting commands

3e670a8

puddly added 9 commits April 11, 2022 12:05

Revert incorrect stop -> stop_with_on_off

a75b0b3

Add unit tests to ensure all lighting commands are valid

e788c30

Rename COMMAND_MOVE_SATURATION to COMMAND_MOVE_TO_SATURATION

183519f

Fix wrong _saturation command being used

92e0f6e

Remove "options_mask": None, that was accidentally left in

8dddfb0

zigpy#1474 (comment)

Fix incorrect version of STOP used in IKEA four button remote quirk

054cd1b

Move OCCUPANCY_* constants into const

8d7fa17

Fix IKEA dimmer direction trigger

9586451

Bump minimum required zigpy version to 0.44.2

b532a61

This version of zigpy fixes typos that prevent quirks from functioning

puddly force-pushed the puddly/hotfix-triggers-args-to-params branch from 03b73cb to b532a61 Compare April 11, 2022 16:05

dmulcahey merged commit ce932b7 into zigpy:dev Apr 11, 2022

MattWestb mentioned this pull request Apr 12, 2022

ZHA ikea tradfri button up/down buttons service calls are not recognized anymore home-assistant/core#69375

Closed

Emrvb added a commit to Emrvb/zha-device-handlers that referenced this pull request Aug 31, 2022

Updated device handlers to use parameter matching instead of args (z…

e67f6ce

…igpy#1474)

Emrvb added a commit to Emrvb/zha-device-handlers that referenced this pull request Oct 31, 2023

Updated device handlers to use parameter matching instead of args (z…

364d968

…igpy#1474)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use parameter matching instead of `args` for all lighting commands #1474

Use parameter matching instead of `args` for all lighting commands #1474

puddly commented Apr 8, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

dmulcahey commented Apr 9, 2022 •

edited

Loading

MattWestb commented Apr 9, 2022

dmulcahey commented Apr 9, 2022

MattWestb commented Apr 9, 2022

dmulcahey commented Apr 9, 2022

MattWestb commented Apr 9, 2022

dmulcahey commented Apr 9, 2022

MattWestb commented Apr 9, 2022

puddly commented Apr 9, 2022 •

edited

Loading

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022 •

edited

Loading

MattWestb commented Apr 9, 2022

puddly commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

puddly commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

puddly commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

puddly commented Apr 11, 2022

MattWestb commented Apr 11, 2022

coveralls commented Apr 11, 2022

Use parameter matching instead of args for all lighting commands #1474

Use parameter matching instead of args for all lighting commands #1474

Conversation

puddly commented Apr 8, 2022

MattWestb commented Apr 9, 2022

One more time GREAT WORK !!

MattWestb commented Apr 9, 2022

dmulcahey commented Apr 9, 2022 • edited Loading

MattWestb commented Apr 9, 2022

dmulcahey commented Apr 9, 2022

MattWestb commented Apr 9, 2022

dmulcahey commented Apr 9, 2022

MattWestb commented Apr 9, 2022

dmulcahey commented Apr 9, 2022

MattWestb commented Apr 9, 2022

puddly commented Apr 9, 2022 • edited Loading

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022 • edited Loading

MattWestb commented Apr 9, 2022

puddly commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

puddly commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

puddly commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

MattWestb commented Apr 9, 2022

puddly commented Apr 11, 2022

MattWestb commented Apr 11, 2022

Great work @puddly and also @dmulcahey for support for making it working !!!

coveralls commented Apr 11, 2022

Pull Request Test Coverage Report for Build 2149729312

💛 - Coveralls

Use parameter matching instead of `args` for all lighting commands #1474

Use parameter matching instead of `args` for all lighting commands #1474

dmulcahey commented Apr 9, 2022 •

edited

Loading

puddly commented Apr 9, 2022 •

edited

Loading

MattWestb commented Apr 9, 2022 •

edited

Loading