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

Add workaround for duration support on sendCommand set #1159

Closed
wants to merge 2 commits into from

Conversation

matejdro
Copy link

@matejdro matejdro commented May 8, 2021

This is a workaround for zwave-js/node-zwave-js#1321 that allows setting MultiLevelSwitch with duration parameter via sendCommand mqtt topic by specifying second parameter in the set command as duration seconds.

This is not ideal, but it allows us to at least use dimming duration in some way until proper solution is developed (which appears to take a while).

lib/ZwaveClient.js Outdated Show resolved Hide resolved
@robertsLando
Copy link
Member

@AlCalzone Could this lead to other kind of issues? Or break old sendCommand?

@zwave-js-assistant
Copy link

🚧 It seems like this PR has lint errors 🚧

I should be able to fix them for you. If you want me to, just comment
@zwave-js-bot fix lint

@AlCalzone
Copy link
Member

AlCalzone commented May 10, 2021

Could this lead to other kind of issues?

Not every CC that has a set command accepts a value and duration.
Basic CC doesn't: https://zwave-js.github.io/node-zwave-js/#/api/CCs/Basic
Color Switch CC doesn't: https://zwave-js.github.io/node-zwave-js/#/api/CCs/ColorSwitch?id=set
Door Lock CC doesn't: https://zwave-js.github.io/node-zwave-js/#/api/CCs/DoorLock?id=set
Thermostat Setpoint CC will break if you do this: https://zwave-js.github.io/node-zwave-js/#/api/CCs/ThermostatSetpoint?id=set
Same with User Code CC: https://zwave-js.github.io/node-zwave-js/#/api/CCs/UserCode

Essentially, this is only okay for
https://zwave-js.github.io/node-zwave-js/#/api/CCs/BinarySwitch?id=set
https://zwave-js.github.io/node-zwave-js/#/api/CCs/MultilevelSwitch?id=set
https://zwave-js.github.io/node-zwave-js/#/api/CCs/SceneActivation?id=set

However, I'd prefer if this was done in those methods in the driver that do accept a duration. One way we could represent the duration would be a string like 2m5s or 55s to leave no doubt what the value means.

@matejdro
Copy link
Author

matejdro commented May 10, 2021

Should I add a check for the type of CC?

Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>
@matejdro
Copy link
Author

@zwave-js-bot fix lint

@zwave-js-bot
Copy link
Collaborator

🐌 Please wait for the lint check to complete, then try again.

@robertsLando
Copy link
Member

However, I'd prefer if this was done in those methods in the driver that do accept a duration. One way we could represent the duration would be a string like 2m5s or 55s to leave no doubt what the value means.

I'm +1 for this. If so this PR could be closed and this should be handled on zwavejs side

@zwave-js-assistant
Copy link

🚧 It seems like this PR has lint errors 🚧

I should be able to fix them for you. If you want me to, just comment
@zwave-js-bot fix lint

@matejdro
Copy link
Author

Well the idea was to add quick and dirty workaround until real duration support is ready without the need to change the core zwavejs side. But I guess I can take a stab at editing the zwavejs intead.

@AlCalzone
Copy link
Member

Sure but if that change is going to be incompatible with the desired solution we'll end up with a breaking change down the line.

@robertsLando
Copy link
Member

I see no reason to merge e temporary (breaking) dirty solution, I prefer to wait the support to be added on zwave-js side to allow back compatibility

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.

4 participants