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

[Feature] Expose a Unlatch button #284

Closed
coxtor opened this issue Feb 2, 2024 · 31 comments · Fixed by #285
Closed

[Feature] Expose a Unlatch button #284

coxtor opened this issue Feb 2, 2024 · 31 comments · Fixed by #285

Comments

@coxtor
Copy link

coxtor commented Feb 2, 2024

Hi, it would be cool to have a unlatch / open button exposed too.

@technyon
Copy link
Owner

technyon commented Feb 2, 2024

I'm not sure what exactly you mean. A hardware button? You can do that already by configuring GPIO accordingly. A software button? In the web interface of NUKI Hub, or are you coming from home assistant and you're asking about an additional control there?

@mundschenk-at
Copy link
Collaborator

I think @coxtor means a button entity in HA. However, I think this is a terrible idea because it is much to easy to accidentally unlatch the door with such a button. Furthermore, if someone wants the button nonetheless, they can always configure it themselves for their installation. Unlatch functionality itself is already provided by the lock entity.

@coxtor
Copy link
Author

coxtor commented Feb 2, 2024

exactly what @mundschenk-at said. Yes this is what I have done . I just think that having this entity in the device thatbasically calls the lock.open service feels more complete. For example One leaves the house and wants to open the door - simply unlocking would leave one standing in front of the door. @mundschenk-at for the sake of your argument one could say the same about not exposing the unlock entity too.
As I said its more or less just a sugesstion for "completeness".

@technyon
Copy link
Owner

technyon commented Feb 2, 2024

I think the argument here is that you usually can't undo an unlatch, because the door is open after unlatching and you have to manually close it. If you accidentally unlocked it, you can just lock it again. I'm thinking of a scenario when you're not at home and can't just close the door. In my interface for remote access, I intentionally left out an unlatch button for exactly that reason.

@mundschenk-at
Copy link
Collaborator

mundschenk-at commented Feb 2, 2024

Not sure what you mean by "unlock entity". The lock entity has specific semantics and UI and already allows all necessary operations in a way that by default does not open your front door with a single click. I think that is sensible for the vast majority of people.

If you absolutely do need one-click unlatching, you can either add a button entity to the device using YAML or just configure a clickable UI element that calls the service on the lock entity in one of your dashboard cards.

Addendum: Having a separate button just for unlatching also really goes against HA's abstraction layer. Why not one for unlocking as well then? Or for locking?

@coxtor
Copy link
Author

coxtor commented Feb 2, 2024

I gues thats just a specific thing to my door then. I can't open it without unlatching. I just thought that we are not exposing the full functionality of the device to HA. Fair enough if you don't want to add it though. For comparison of the case: In my mind its like only exposing on and off of a dimmable light and "hiding" the feature to dimm it.
Feel free to close.

@mundschenk-at
Copy link
Collaborator

Na, its not specific to your door, most European door locks are built this way. But what you seem to be missing is that unlatching is part of the lock entity, so the functionality is already exposed to HA. To stay with your dimmable light, you would not add a separate dim slider when the light already exposes the dimming feature.

@coxtor
Copy link
Author

coxtor commented Feb 2, 2024

I partitially disagree. While you are correct that its part of the lock entity, the frontend of ha in the devices section gives the possibility to controll all devices features, - (again the light feature: )including the slider and all associated light effects. However, this is really nothing really relevant, as the workarround is simple.

@mundschenk-at
Copy link
Collaborator

But unlatching is already in the UI: Once you unlock, you the get a button for re-locking and for opening (unlatching).
image

@iranl
Copy link
Collaborator

iranl commented Feb 2, 2024

I disagree with not exposing existing functionality to give a false sense of security.

If one is afraid to accidentally remotely open their door than maybe they should not

  1. Put a smart lock on your door in the first place
  2. Enable remote access

The functionality is there either way, wether you create a button for it or not.

At least when the button is visible users will be aware of the possibility to unlatch the door and easily implement the button in their HA UI if they want to or maybe consider disabling the ability to unlatch through the access level added in #279

Nuki also exposes these buttons through autodiscovery in Home Assistant in this way when using their own MQTT implementation.

See #285 for the pull request

@mundschenk-at
Copy link
Collaborator

The functionality is already there. Making it easier to accidentally shoot yourself in the foot is not a worthy design goal.

(I don't see why it should be relevant what Nuki does in their MQTT implementation. It's not like they know HA very well.)

@alexdelprete
Copy link
Collaborator

In my interface for remote access, I intentionally left out an unlatch button for exactly that reason.

Personally, I have a confirmation request when pressing "dangerous" buttons. This doesn't mean we shouldn't have an easy way for users who want to have those buttons. :)

I'm always against "deciding for users because users do stupid things", that's against the "local control" approach that we love. I don't want a Nuki dev deciding if I'm allowed to have a button or not. If users want to have an unlatch button, they will take the responsibility for stupid things, not the dev. Let them decide and use them as they want, also because they can already create an unlatch button, so having them auto-discovered it's just easier to configure things.

I'm in favor of PR #285.

@alexdelprete
Copy link
Collaborator

alexdelprete commented Feb 3, 2024

Making it easier to accidentally shoot yourself in the foot is not a worthy design goal.

So making things harder for users is a secure design approach? :)

Users want local control, it's not up to devs to decide HOW they want to use functionalities. That's why I always argued with Nuki and also HA devs. Devs should never take decisions for users because they think users are stupid. It's a paternalistic approach that should never happen when discussing local automation developement. Functionality is already there, let users use it as they prefer, "hiding" buttons is not "good security design" in any possible way.

It's like asking Linus to remove rm -rf from Linux. :)

@mundschenk-at
Copy link
Collaborator

This is not about security, but about safety. Related, but distinct.

Anyway, safety-by-design is only one argument, the other is that adding buttons for random things that are already exposed in other ways is working against the abstraction layer of Home Assistant. Just as you would not add a button for switching of a light (because the light platform already includes that function), we should not add an unlatch button because the lock platform already has that covered.

@technyon
Copy link
Owner

technyon commented Feb 4, 2024

Just for me to understand since I'm not using HA. Once you unlock, you'll get the unlatch button, correct?

In that case I'd agree that it would be adding redundant functionality. What's the benefit of adding another button to do the same thing?

@alexdelprete
Copy link
Collaborator

but about safety. Related, but distinct.

you don't decide safety for users. you offer functionalities, it's up to them deciding what to enable and what to use, not you. That's why we use Home Assistant: LOCAL CONTROL. Not up to an Amazon/Google/Microsoft dev to decide what's safe for me. You have the same mentality as them, from how you are approaching this, you think users are stupid and do stupid things. Well, let them do stupid things if they want, it's not up to you to decide what they are allowed or not to do.

the other is that adding buttons for random things that are already exposed in other ways is working against the abstraction layer of Home Assistant

A button that calls a service is not breaking anything, it's just making easier to configure a button for a user. HA is not an abstraction layer, it's an Automation Platform. And it has button entities for many use-cases, and this is the PERFECT use-case.

Why do you think HA devs made button entities available in autodiscovery? HA is wonderful because you can target a use-case in 50 different ways, abstraction doesn't mean there should be only one way (your way) to do things, quite the contrary: it allows you great flexibility and solve a problem in many different ways.

Just as you would not add a button for switching of a light (because the light platform already includes that function), we should not add an unlatch button because the lock platform already has that covered.

Buttons are used ALSO for lights (I do too). It's clear now to me that you don't have a clear idea of what a button entity is in HA:

image

Buttons are designed to be one-way interaction from Home Assistant to an integration (Nuki Hub in this case), or, abstracting: to have something happen on a device based on an action in Home Assistant (the button press). Buttons are STATELESS, a switch has a state, because they serve two whole different puroposes.

Creating a button for a service is not in any possible rational way duplicating the service: it's ENABLING the functionality, it's an entrypoint. And buttons were created exactly for this purpose.

@alexdelprete
Copy link
Collaborator

alexdelprete commented Feb 4, 2024

Just for me to understand since I'm not using HA. Once you unlock, you'll get the unlatch button, correct?

In that case I'd agree that it would be adding redundant functionality. What's the benefit of adding another button to do the same thing?

No, what he showed is a "mushroom lock card": it's not a default feature, mushroom is a sort of collection of UI cards and it has a lock card specific for lock devices.

I created my own mushroom card (using mushroom template card), specific for Nuki Hub:

image
image

BTW: I have an open issue with mushroom because (as you can see above) you can actually unlatch while the lock is locked, it's not necessary to unlock. I asked the dev to expose the open/unlatch button also while in locked state: piitaya/lovelace-mushroom#876

So, to answer your question: it's not duplicating anything, also because users have their own way of designing the UI (Lovelace) and they might want to use buttons or other things.

A button is just a momentary switch and it's stateless.

@iranl
Copy link
Collaborator

iranl commented Feb 4, 2024

Just for me to understand since I'm not using HA. Once you unlock, you'll get the unlatch button, correct?

In that case I'd agree that it would be adding redundant functionality. What's the benefit of adding another button to do the same thing?

I agree with @alexdelprete that a separate button for unlatching cannot be regarded as a redundant button in the way HA exposes unlatch/open door in the UI currently. This and earlier issues highlight the situation that unlatching is not intuitive in the HA UI, at least for some users.

As mentioned earlier NUKI exposes these buttons in the exact same way through auto discovery in the official MQTT implementation themselves (hopefully also taking safety/security/usability into account?)

Feature parity with the official implementation is a reasonable goal imo, while also adding additional options and choices that NUKI does not make available.

As such I will extend the PR to cover the following:
-Add a checkbox to add these buttons to the discovery topic (disabled by default)
-If this option is selected, still disable them in HA by default, as suggested by @mundschenk-at

That should hopefully be a compromise that everyone can be comfortable with.

@technyon
Copy link
Owner

technyon commented Feb 4, 2024

Sounds reasonable to me, thanks for the PR @iranl

@mundschenk-at
Copy link
Collaborator

mundschenk-at commented Feb 4, 2024

Just for me to understand since I'm not using HA. Once you unlock, you'll get the unlatch button, correct?
In that case I'd agree that it would be adding redundant functionality. What's the benefit of adding another button to do the same thing?

No, what he showed is a "mushroom lock card": it's not a default feature, mushroom is a sort of collection of UI cards and it has a lock card specific for lock devices.

Yeah, sorry, I did overlook that I was still using Mushroom Cards for this (I started using them before the official Tile card became a thing and I've since replaced most Mushroom cards with Tile cards, but apparently not for the locks).

Since the default Entity and Tile cards included with HA don't have any UI for triggering unlatching, I retract my argument that the button duplicates existing functionality. While the Tile card may eventually get feature parity with Mushroom Cards, that is not a given and thus currently out of the box you always need call the lock.open service to unlatch.

The safety argument is still valid (I know you disagree, @alexdelprete, but having safe defaults is an ethical requirement for software/system development - changing them should be an informed choice by the user), but that by itself is much easier to satisfy by having those entities disabled by default in HA. @iranl, I don't think a separate setting in the Nuki Hub web interface is necessary (it would mostly make the implementation more complex without a lot of benefits).

PS: @alexdelprete I'm not sure why you think HA does not have an abstraction layer - it obviously abstracts away a lot of device implementation details - but we've had such terminological discussions before and I don't think this is going anywhere productive, so let's leave it at that.

@technyon
Copy link
Owner

technyon commented Feb 4, 2024

Thanks everyone for the constructive discussion and your input on home assistant. It's hard for me to judge because I use iobroker, so I'm glad we've found a solution.

@technyon
Copy link
Owner

technyon commented Feb 4, 2024

I've merged the pull request. Please @coxtor @iranl @mundschenk-at @alexdelprete check the binary below if everything behaves as expected.

Note: This also includes the updated Arduino Core for ESP32 2.14

nuki_hub-8.32-pre-2.zip

@mundschenk-at
Copy link
Collaborator

mundschenk-at commented Feb 4, 2024

I just updated. I've noticed that this version also adds the buttons for the Nuki Opener, where IMHO only the unlatch button makes sense (not sure what the others would even do when applied to the Opener), and even for that I think the naming should be different (though "unlatch" is not totally wrong, it sounds weird to me in this context - right now I don't have an alternative name though).

Edit: The buttons themselves work fine (for the Lock).

@iranl
Copy link
Collaborator

iranl commented Feb 4, 2024

Buttons are discovered ok, are disabled by default and work fine.

Agree with @mundschenk-at about the buttons not being logical for the opener (naming and/or being there at all).

Must have missed it through a combination of not owning an opener and still getting to complete grips with the code structure.

The unlatch equivalent for the opener is Electronic Strike Actuation right?

Will create a new PR to address. Probably two separate functions that only get called from the appropriate Network(Opener/Lock).cpp

@technyon
Copy link
Owner

technyon commented Feb 4, 2024

Hi,

I've already made the changes. Not sure what how to call the unlatch button either. "Open" ?

nuki_hub-8.32-pre-3.zip

@mundschenk-at
Copy link
Collaborator

LGTM

@mundschenk-at
Copy link
Collaborator

mundschenk-at commented Feb 4, 2024

Yeah, "Open" sounds better than "Unlatch" for the Opener.

@alexdelprete
Copy link
Collaborator

Yeah, "Open" sounds better than "Unlatch" for the Opener.

Also for the lock, I'd call it Open, like HA does (the service is Open).

@alexdelprete
Copy link
Collaborator

I know you disagree, @alexdelprete, but having safe defaults is an ethical requirement for software/system development - changing them should be an informed choice by the user

I'm in disagreement with the fact you consider a button a safety issue, not with ethics/safety in sw developement. You still failed to convince me with a rational argument about why hiding a button represents a safety improvement.

I don't think a separate setting in the Nuki Hub web interface is necessary (it would mostly make the implementation more complex without a lot of benefits).

I agree with you here, but I think he implemented it because of our discussion, as a compromise. :)

I'm not sure why you think HA does not have an abstraction layer - it obviously abstracts away a lot of device implementation details - but we've had such terminological discussions before and I don't think this is going anywhere productive, so let's leave it at that.

You said HA is an abstraction layer, I said it's not only an abstraction layer but an Automation Platform, and EVERY Automation Platform provides abstraction intrinsically. So your argument was vague and doesn't convince me that a button in autodiscovery is redundant and dangerous. Yes, we have two different views of terminology and things, but that's good for a discussion, would be boring if we all had the same view about things. :)

@technyon
Copy link
Owner

technyon commented Feb 5, 2024

So everything works as expected, except for renaming unlatch to open? If so I'd do the renaming and then prepare a release.

@alexdelprete
Copy link
Collaborator

If so I'd do the renaming and then prepare a release.

Good. I'll test final release.

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 a pull request may close this issue.

5 participants