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

Implement missing functionality for Door Locks #21

Assignees
Labels
backend This bug or request (primary) involves the Home Assistant backend/integration (and its dependencies) expert-ui This is something that should go in HA's new Z-Wave Expert UI new feature New feature or request zwave-certification Required for Z-Wave certification

Comments

@marcelveldt
Copy link
Collaborator

From core created by AlCalzone: home-assistant/core#86955

The problem

grafik
(the mentioned operation types mean constant 0x01 and timed 0x02, V4+ adds additional ones)

I could not find any way to configure the operation type of a lock, so this will need to be added before certification.


Also setting the lock mode (Locked, Unlocked, and several timed modes) must be available to the user if supported by the lock:
grafik

What version of Home Assistant Core has the issue?

2023.1.7

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

Z-Wave JS

Link to integration documentation on our website

No response

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

@marcelveldt marcelveldt added integration: zwave_js zwave-certification Required for Z-Wave certification labels Jul 13, 2023
@marcelveldt marcelveldt changed the title Z-Wave JS: Certification requirements for Door Lock CC not fulfilled Implement missing functionality for Door Locks Jul 13, 2023
@marcelveldt marcelveldt added new feature New feature or request backend This bug or request (primary) involves the Home Assistant backend/integration (and its dependencies) labels Jul 13, 2023
@marcelveldt
Copy link
Collaborator Author

See the original issue in core for some discussion about this, the gist is that there are no configuration options missing but actual controls and sensors for door locks.

@raman325
Copy link
Collaborator

@AlCalzone I started looking at this again. Can you review my assumptions and tell me which, if any are wrong?

  1. We need to set all DoorLock CC parameters at the same time which we will do using the setConfiguration CC command.
  2. outsideHandlesCanOpenDoor and insideHandlesCanOpenDoor are not required to be controllable by users.
  3. outsideHandlesCanOpenDoor and insideHandlesCanOpenDoor must be sent as part of the setConfiguration CC command, so presumably we can just retrieve the current value of these parameters and resend them.
  4. If the CC version is v4, only then will the following parameters be available: blockToBlock, holdAndReleaseTime, twistAssist, and blockToBlock
  5. If the CC version is v4, all four parameters listed in 4 need to be sent as part of the setConfiguration CC command.
  6. If the CC version < v3, we CANNOT include any of the parameters listed in 4 in the command.

@AlCalzone
Copy link
Member

  1. correct
  2. correct
  3. correct, but I think this should be done in the driver. The serialize function already has some fallbacks if those are not arrays, so they should be just made optional and default to 0xF as recommended if no value has been set before.
  4. correct, but not all of them have to be supported by a lock. Z-Wave JS will only create the corresponding values if they are supported.
  5. No, they are still optional. They will default to 0/false though if not provided. Might make sense to read the cached values in the setConfiguration command though.
  6. You can pass them, but Z-Wave JS will not send them to the device.

@raman325
Copy link
Collaborator

Might make sense to read the cached values in the setConfiguration command though.

Are you saying that if the v4 properties aren't included, the library should read the cached values or are you thinking about adding this to the driver?

@AlCalzone
Copy link
Member

I'm not sure this is solved yet:

  1. I have no idea what to fix here:

Image

  1. Not sure how to do this:

Setup Door Lock Mode to 'UnsecuredWithTimeout'

Image

@AlCalzone AlCalzone reopened this Nov 20, 2023
@AlCalzone AlCalzone moved this from Done to Triage in Z-Wave JS: Road to Certification Nov 20, 2023
@AlCalzone
Copy link
Member

Related to 2. in the above comment, other states than locked and unlocked can also not be displayed:

Please check if current mode is set to 'UnsecuredWithTimeout'. Is the mode displayed correctly?

@AlCalzone
Copy link
Member

AlCalzone commented Apr 25, 2024

So, I got further this time, but setting the lock configuration still fails. This is because lockTimeoutConfiguration is missing from the configuration passed to the driver, even though the checkbox is set:

Image

This field is required when the operation type is timed.


Also how is this supposed to be formatted? I can't get it to pass validation:

Image

@AlCalzone AlCalzone reopened this Apr 25, 2024
@AlCalzone AlCalzone moved this from Done to Triage in Z-Wave JS: Road to Certification Apr 25, 2024
@marcelveldt marcelveldt moved this from Triage to Later in Z-Wave JS: Road to Certification May 23, 2024
@marcelveldt
Copy link
Collaborator Author

Shouldnt this be in the expert UI ?

@AlCalzone AlCalzone added the expert-ui This is something that should go in HA's new Z-Wave Expert UI label Jul 4, 2024
@AlCalzone
Copy link
Member

AlCalzone commented Sep 2, 2024

Very, very short term fix:

  • In the service call zwave_js.set_lock_configuration, pass missing lockTimeoutConfiguration to the node-zwave-js API

Better:

  • Move the service call functionality to the expert UI. This needs one block where:

    • User can select operation type among supported (Timed, Constant)
    • User can check/uncheck inside/outside door handles (4, amount can not be detected)
    • If operation type is timed, set the timeout min:sec
    • If supported, set the auto relock time in seconds
    • If supported, set the hold-and-release time in seconds
    • If supported, enable/disable block-to-block functionality
    • If supported, enable/disable twist assist
    • One button to apply the changes
  • Afterwards, verify if this requirement is fulfilled for the door lock mode control on the device page:

Timed Operation modes MUST NOT be selectable by the end user if the
door lock is not configured in Timed Operation

@AlCalzone AlCalzone added needs-diagnostics Needs device diagnostics to work on this and removed needs-tasks labels Sep 2, 2024
@AlCalzone
Copy link
Member

@AlCalzone AlCalzone removed the needs-diagnostics Needs device diagnostics to work on this label Sep 4, 2024
@MartinHjelmare
Copy link
Collaborator

MartinHjelmare commented Sep 4, 2024

I think I've found the bug for missing lock timeout configuration. We don't include that attribute when building the dict from the dataclass that represents the lock configuration in the client library. So even though we pass the parameter from the integration it's not included in the parameters sent from the client to the server.

https://github.com/home-assistant-libs/zwave-js-server-python/blob/78c0ed02c5ffd0f57d991058eb099b82394d2a2f/zwave_js_server/const/command_class/lock.py#L177-L197

I'm working on a fix.

@MartinHjelmare
Copy link
Collaborator

Let's move the expert panel tasks to a new issue. I'll close this issue when merging the fix.

@AlCalzone
Copy link
Member

Followup issue: #48

@AlCalzone
Copy link
Member

Still not fixed I'm afraid:
Image

@AlCalzone AlCalzone reopened this Oct 30, 2024
@MartinHjelmare
Copy link
Collaborator

Please paste the YAML from the YAML mode instead.

@MartinHjelmare
Copy link
Collaborator

MartinHjelmare commented Oct 30, 2024

We decided to remove those parameters (inside/outside handles) since they weren't required for certification as we understood it at that time. We forgot to remove it from the service description.

home-assistant/core#103595 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend This bug or request (primary) involves the Home Assistant backend/integration (and its dependencies) expert-ui This is something that should go in HA's new Z-Wave Expert UI new feature New feature or request zwave-certification Required for Z-Wave certification
Projects
4 participants