Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow setting of duration/transition #1321

Closed
marcelveldt opened this issue Jan 8, 2021 · 30 comments · Fixed by #2894
Closed

allow setting of duration/transition #1321

marcelveldt opened this issue Jan 8, 2021 · 30 comments · Fixed by #2894
Assignees
Labels
enhancement New feature or request

Comments

@marcelveldt
Copy link

marcelveldt commented Jan 8, 2021

Is your feature request related to a problem? Please describe.
Looks like setting the light transition duration is not implemented yet.

Describe the solution you'd like
Be able to set transition duration value.

Explaining your use-case will go a long way in helping us understand.
Home Assistant integration

Schermafbeelding 2021-01-08 om 23 17 02

Schermafbeelding 2021-01-08 om 23 17 49

Thanks!

@marcelveldt marcelveldt added the enhancement New feature or request label Jan 8, 2021
@zwave-js-bot zwave-js-bot added this to Needs triage in Triage Jan 8, 2021
@AlCalzone AlCalzone moved this from Needs triage to Feature request in Triage Jan 9, 2021
@AlCalzone AlCalzone self-assigned this Jan 9, 2021
@AlCalzone
Copy link
Member

What is the expected behavior here?

  1. Let's say, a user sets a duration, then changes the target value.
  2. The device later gets triggered physically and reports a different duration.
  3. The user changes the target value again.

Which duration (if any) gets used for the set command?

@robertsLando
Copy link
Member

robertsLando commented Jan 13, 2021

The device later gets triggered physically and reports a different duration

Why should this happen? Does a trigger also report a DUration?

@AlCalzone
Copy link
Member

Yes, some newer devices report current and target value as well as the remaining duration.

@robertsLando
Copy link
Member

I think that the device should always use the user selected duration

@AlCalzone
Copy link
Member

AlCalzone commented Jan 13, 2021

Currently, there's only one duration value ID, so that would get overwritten when we receive a report.
And looking at the confusion around current/target value I'm really hesitant to add another of these value pairs.

@marcelveldt
Copy link
Author

I think it would be nice to be able to retrieve the current configured transitionDuration and must-have to set a transitionDuration. maybe adjust it the same so you'll have currentDuration and targetDuration ?

@AlCalzone
Copy link
Member

I'm wondering if it is desirable not to have an extra value to set the transition duration and instead pass it somehow along with the set command?
I'm not very familiar with the MQTT API to determine if that is possible and makes sense.

@robertsLando
Copy link
Member

I'm wondering if it is desirable not to have an extra value to set the transition duration and instead pass it somehow along with the set command?

This could be good but also setting a default value should be possible

@AlCalzone
Copy link
Member

AlCalzone commented Jan 14, 2021

@marcelveldt we've been thinking - what's your take on adding another argument to setValue that specifies options for the set call? E.g.

setValue(/* targetValue ID */, 99, { duration: "2m" }); // transition to 99 over 2 minutes
setValue(/* thermostat setpoint ID */, 23, { scale: 1 }); // turn setpoint to 23 °C

These additional properties would be discoverable via setValueOptions: ["duration", ...] on the value metadata.

I'd prefer something like this, because it avoids littering applications with tons of single-purpose values. It would also allow you to define the transition duration, setpoint scale, etc. in a single location (e.g. once per node).

The remaining duration would still be reported as a value like now.

@marcelveldt
Copy link
Author

That sounds nice!
The code we have in HA is now as following:

  1. check current transitionDuration for the light
  2. if user gives a transition parameter and the value is different than currently set on the light, submit it.
  3. is user omits the transitionDuration, the light's default should be used (255)
  4. send transitionDuration value to light if it differs.
  5. send setValue to actually change brightness

So calls 4 and 5 could actually be combined and perhaps the check done at 3 too ?

@AlCalzone
Copy link
Member

send transitionDuration value to light if it differs

Z-Wave does not have a concept of saving transition duration on a device. It can either get sent as part of the set command or left out.

I'm guessing you're doing it that way because OZW had a settable duration parameter? In that case, the new process would be:

  1. send setValue to change brightness. Include transition duration if the user provided it.

If no duration was given, zwave-js automatically sends a command with the default duration.

@jcam
Copy link
Contributor

jcam commented Jan 14, 2021

Would changing the default duration be a separate setting, controllable through the API?

@marcelveldt
Copy link
Author

@AlCalzone that sounds like a very valid approach and even an improvement over what we have now.

@AlCalzone
Copy link
Member

Would changing the default duration be a separate setting, controllable through the API?

Nothing defined in the specs. If there's an option to do that for a device, it is a device-specific configuration value.

@kpine
Copy link
Contributor

kpine commented Jan 15, 2021

If no duration was given, zwave-js automatically sends a command with the default duration.

What does "default duration" mean in this context?

@AlCalzone
Copy link
Member

It omits the duration field (so it sends a V1 command), which means a device should use the factory default duration - whatever that is.

Alternatively we could think about explicitly including the duration field and setting it to 255 (=default) if that is necessary.

@marcelveldt
Copy link
Author

Give me a ping once this is implemented so we can adjust our code. We will only send the duration if the user explicitly defines it and we'll not send it otherwise.

@kpine
Copy link
Contributor

kpine commented Jan 15, 2021

It omits the duration field (so it sends a V1 command), which means a device should use the factory default duration - whatever that is.

Interesting, I hadn't thought of doing it that way. I suppose that will work as long as there are not any new versions of the Set command. Thanks for the clarification.

@AlCalzone
Copy link
Member

AlCalzone commented Jan 15, 2021

There are. But the specs say (I know, who cares about specs - not the device manufacturers at least) to treat a V1 command (for Multilevel Switch at least) without duration the same as a V2+ command with a "default" duration.

@kpine
Copy link
Contributor

kpine commented Jan 15, 2021

I meant the Set command itself, no new version since V2. If a V5 comes along with a new field, you would be required to set duration in addition with the new field.

The specs say devices "should" (recommend) support V1. I wouldn't be surprised if there's some device that actually does take it as a "should" (or maybe the SDK handles it)! 😄

@jcam
Copy link
Contributor

jcam commented Jan 15, 2021

My Eaton dimmers are a great example of "should" taken literally. If no duration is set (i.e. they get a V1 set), then they use "0" instead of "default". Which mostly works (snaps instead of dims are a bit jarring), except when turning off. When told to turn off, they drop to about 8% and just stay there, instead of turning off.

@kpine
Copy link
Contributor

kpine commented Jan 15, 2021

And if it's a V2 set with duration 0xFF, they function as expected?

@jcam
Copy link
Contributor

jcam commented Jan 15, 2021

yup! 🤦

@kpine
Copy link
Contributor

kpine commented Jan 15, 2021

That seems like a good argument for generally using the V2 Set command for V2 devices. 😁

@matejdro
Copy link
Contributor

matejdro commented Feb 8, 2021

Is there a workaround to this issue at the moment (maybe settings some raw z-wave value through mqtt)?

@AlCalzone
Copy link
Member

https://zwave-js.github.io/zwavejs2mqtt/#/guide/mqtt?id=send-command
would work normally, but the set method expects a Duration class instance as the 2nd parameter - I don't think you can pass that via mqtt.

@matejdro
Copy link
Contributor

Yeah it does not appear so 😞

From what I can see, Duration class is just a class with one field, _value, so this should work:

{
    "args": [
        {
            "nodeId": 16,
            "commandClass": 38,
            "property": ""
        },
        "set",
        [
            50,
            {
                "_value": 5
            }
        ]
    ]
}

But second parameter is ignored (value is set to 50% with the default duration).

@dlashua
Copy link

dlashua commented Feb 13, 2021

If it helps any, even setting the "duration" via the ZWaveJS2MQTT Control Panel doesn't work.

image

It results in this error in the logs:

2021-02-13 11:42:31.211 INFO ZWAVE: Calling api writeValue with args: [
  { nodeId: 9, commandClass: 38, endpoint: 0, property: 'duration' },
  { value: 10, unit: 'seconds' },
  [length]: 2
]
2021-02-13 11:42:31.212 INFO ZWAVE: Writing { value: 10, unit: 'seconds' } to 9-38-0-duration
2021-02-13 11:42:31.213 INFO ZWAVE: Controller status: Driver: Multilevel Switch: "duration" is not a supported property
2021-02-13 11:42:31.214 ERROR ZWAVE: Unable to write { value: 10, unit: 'seconds' } on 9-38-0-duration

This is for Device ID: 634-40962-40960 (0x027a-0xa002-0xa000), which is able to handle transitions just fine with OZWd.

@AlCalzone
Copy link
Member

This is because the API expects a duration instance with the serializeSet method existing

@matejdro
Copy link
Contributor

Would you be okay with accepting PR with little if in the sendCommand that would create duration object? This would of course be temporary and removed until proper duration support is implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants