-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
ZWaveJS: Support for Door Lock in Expert UI #22775
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@AlCalzone do you have a lock to test this? |
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.
I don't see why it happens, but if the operation mode is set to Timed, lockTimeoutConfiguration
is not actually set in the configuration object sent to Z-Wave JS.
...ons/integration-panels/zwave_js/capability-controls/zwave_js-capability-control-door-lock.ts
Outdated
Show resolved
Hide resolved
…ability-controls/zwave_js-capability-control-door-lock.ts Co-authored-by: AlCalzone <[email protected]>
The call now goes through, but it looks like the same change is needed for the toggles as well. |
...ons/integration-panels/zwave_js/capability-controls/zwave_js-capability-control-door-lock.ts
Outdated
Show resolved
Hide resolved
With my last suggestion, the first half of the certification test passes. The second half simulates an older device and runs into an error because the unsupported command |
…ability-controls/zwave_js-capability-control-door-lock.ts Co-authored-by: AlCalzone <[email protected]>
const capabilities = await invokeZWaveCCApi<DoorLockCapabilities | null>( | ||
this.hass, | ||
this.device.id, | ||
this.command_class, | ||
this.endpoint, | ||
"getCapabilities", | ||
[], | ||
true | ||
); |
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 call can throw with an error code ZWaveErrorCodes.CC_NotSupported
if the getCapabilities
command isn't supported by the device (i.e. if it's an older lock).
We need to catch that case and assume that autoRelock, holdAndRelease, twistAssist and blockToBlock are not supported.
The cleaner way would be to ask the driver for its cached information, but this isn't exposed yet: zwave-js/node-zwave-js#7388
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.
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.
Do we need to support get/set mode in the expert UI? I left it out because it's not part of getConfiguration/setConfiguration. I can add it with extra requests for the mode
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.
No, just setting is enough for certification. Getting wouldn't hurt, but you do that anyways on load if I'm not mistaken?
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.
Added a select for the mode with validation for the mode.
For get
there is currentMode
and targetMode
. I used currentMode
but please correct me if this is wrong.
It defaults to Unsecured 0x00
when the driver doesn't return the mode on get
. Maybe it should be Unknown 0xfe
but this value is not in the above list so I set it to 0 for now.
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.
Test passed ✅
Oh sorry, I think there was a misunderstanding. We don't have to control the lock mode in the expert UI, we already have proper lock and select entities for that. I thought you meant getting the current configuration, since the expert UI only lets users set it. |
Well we have it now. And I saw that we are required to not allow timed modes for non timed operation, so I think it's good to have them together like this and validate |
Proposed change
The validation doesn't give much feedback to the user but I think this is enough for the purpose of this form.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: