-
-
Notifications
You must be signed in to change notification settings - Fork 603
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
fix: auto-fallback to no supervision for start/stopLevelChange #1178
Conversation
@robertsLando @ODINServ this might fix the issue in zwave-js/zwave-js-ui#40 (review) - wanna give it a try? |
@ODINServ You should use the zwavejs2mqtt master with this branch of zwavejs |
Alright I don't have more time now, I'll try tomorrow |
@@ -98,6 +99,14 @@ function getCurrentValueValueID(endpoint: number): ValueID { | |||
}; | |||
} | |||
|
|||
/** Returns the ValueID used to remember whether a node supports supervision on the start/stop level change commands*/ | |||
export function getSuperviseStartStopLevelChangeValueId(): ValueID { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlCalzone Is this internal right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlCalzone I mean the valueId will not be crated right? It's an internal valueid? Does it need @internal ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the weirder parts of value IDs. The function won't be exported now. The valueID is marked internal in the CC constructor (registerValue
, boolean parameter). One of the many aspects to be considered in #552
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% clear to me but ok I trust you 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ok I'll wait for @ODINServ's test and then merge if it works. |
Hey, Here is what i did:
@node-zwave-js
(( No errors ))
Any clues ? i can revert to a snapshot of my vm and get it working but what am i doing wrong ? |
Try to run : zwave-js is using workspaces that is only supported by npm@7, but I saw that npm link seems is't working well so the lerna way should work (it's like running |
$npm --version
So is did But then:
|
@ODINServ Zwavejs:
Zwavejs2mqtt:
|
@robertsLando doesn't @ODINServ also have to |
@AlCalzone Yes I have edited my comment |
That was it indeed :) but now my break is over, doing the actual test at lunch time. |
@AlCalzone IMO |
I'll need to do some more testing with that. Judging from the command description, it should indeed. |
Well, that failed. It still works! 💪 😄 |
Ok I think we can merge this now @AlCalzone :) |
perfect! |
In #648 we changed start/stopLevelChange to use Supervision if it is supported. However this seems to cause problems with some devices.
With this PR we try a supervised command first. If that yields "No support", we remember that and fall back to an unsupervised command.
Side note: it might make sense to generalize this approach.