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

Change Lock entity to Unavailable when MQTT is disconnected #227

Closed
leranp opened this issue Sep 12, 2023 · 77 comments
Closed

Change Lock entity to Unavailable when MQTT is disconnected #227

leranp opened this issue Sep 12, 2023 · 77 comments

Comments

@leranp
Copy link

leranp commented Sep 12, 2023

Hi,
Love this component, but there is something that i think that needs to be changed, when the MQTT is disconnected for some reason, the lock entity still shows status that can be worng.
In all of my MQTT base devices, when the MQTT is not connected, the device goes to Unavailable State

@technyon
Copy link
Owner

It wouldn't make sense to change the lock state if MQTT is disconnected. The lock doesn't necessarily change state just because MQTT is disconnected.

What you want to monitor is the "maintenance/mqttConnectionState" node, which reflects wether or not the ESP is connected ... which reminds me I'll have to add this to the readme.

@mundschenk-at
Copy link
Collaborator

mundschenk-at commented Sep 12, 2023

I disagree, in HA that absolutely would make sense. When the ESP is disconnected from MQTT, the lock state is unavailable. Adding it to auto-discovery should be pretty straightforward, I can try to come up with something.

(You are not changing the lock state topic, just the way HA parses that into an entity.)

@technyon
Copy link
Owner

ok, understood. However @leranp didn't mention this is HA related.

@mundschenk-at
Copy link
Collaborator

Oh, you are right, I just assumed because of the terminology. @leranp, you are asking about Home Assistant, right?

@leranp
Copy link
Author

leranp commented Sep 13, 2023

Yes, i am talking about HA

@technyon
Copy link
Owner

@mundschenk-at Let's update the auto discovery then. We can release this together with the fix for the auto discovery warnings.

@mundschenk-at
Copy link
Collaborator

Sorry for the delay, I'll do the write-up this weekend.

@technyon
Copy link
Owner

@mundschenk-at ok

@mundschenk-at
Copy link
Collaborator

So in YAML you'd need to add this to auto-discovery:

      availability:
        - topic: <prefix>/maintenance/mqttConnectionState

No need to set the payloads, as they default to online and offline anyway. So for the JSON payload, it would be:

"avty": [{"t": "~/maintenance/mqttConnectionState"}]

I am using the availability syntax here because it would allow us to add several availability topics, e.g. not only for the MQTT connection but also for the BLE connection - the state of the lock is only accurate if there was recent-ish BLE connection and MQTT is connected. You could set such a topic to offline when a BLE restart is necessary. That would make such interruptions visible in the HA logbook.

@mundschenk-at
Copy link
Collaborator

mundschenk-at commented Sep 23, 2023

Anyway, it was just a thought, probably too niche a use-case. The MQTT availability topic should probably be added to all auto-discovery entities except the MQTT connected state.

@technyon
Copy link
Owner

I've added availability to the json as suggested. Please check and confirm that it works. So far only for the lock, I'd added it to the other entities if it's confirmed it works for the lock.

nuki_hub-8.27-pre-2.zip

@mundschenk-at
Copy link
Collaborator

I tried it, works fine for the lock, but It breaks the opener because the connection state is not available under the opener prefix. Not sure if you can separate them with inheritance?

I do have a suggestion how to handle this (with my long-promised write-up of a better auto-discovery device split), but that probably needs some discussion.

@technyon
Copy link
Owner

It should work the same way for the opener, shouldn't it? The easiest fix would be to duplicate the connection state topic under the opener node.

@mundschenk-at
Copy link
Collaborator

This would be the quick fix, but it is kind of dirty. Or you could simply reference the mqttConnectionState under the lock prefix.

IMHO nicer would be to move all the Nuki Hub/ESP-specific topics to their own HA "hub" device (and maybe MQTT topic, but that would be a breaking change of course) and have the Lock and Opener be connected devices to that "hub". (I'd have to look into it whether we would then still need the separate availability topic in the Lock and Opener device definitions, maybe we do.)

@technyon
Copy link
Owner

All paths are relative right? Should be easy enough to point to the lock maintenance topic.

@technyon
Copy link
Owner

technyon commented Oct 9, 2023

Please check if it works like this.

nuki_hub-8.27-pre-3.zip

@leranp
Copy link
Author

leranp commented Oct 9, 2023

Please check if it works like this.

nuki_hub-8.27-pre-3.zip

Looks fine for me

@technyon
Copy link
Owner

technyon commented Oct 9, 2023

@mundschenk-at Does it fix the issue with the opener?

@mundschenk-at
Copy link
Collaborator

@mundschenk-at Does it fix the issue with the opener?

Unfortunately, no. I missed your question about paths being relative, to which the answer would be no (or rather: MQTT topics are not paths that can be navigated). So

"availability": {
  "topic": "nukiopener/../nuki/maintenance/mqttConnectionState"
}

is incorrect, it would need to be

"availability": {
  "topic": "nuki/maintenance/mqttConnectionState"
}

@technyon
Copy link
Owner

@mundschenk-at Give this a try

nuki_hub-8.27-pre-4.zip

@mundschenk-at
Copy link
Collaborator

Sorry I wasn't more explicit yesterday, the new version works fine.

@technyon
Copy link
Owner

Check release 8.27

@bcutter
Copy link
Contributor

bcutter commented Oct 20, 2023

OK I updated to 8.27 some days ago (same day it became available). Now that I restarted HA for the first time since then, all my locks are shown as unavailable, even they are reachable and at the same time the MQTT connected entities show on.

Not sure what screwed things up - I've NEVER ever seen the lock entities in unavailable state before the 8.27 update.
Will try to downgrade one Nuki hub to 8.26 for now and see if that changes things.

grafik

grafik

grafik

Edit:

  • Yes, immediately after downgrading 8.27 to 8.26 the lock entity became available again.
  • So something's not right with the 8.27 - or my Home Assistant. Downgraded the other one too now (same behaviour: 8.26 in place - lock entity immediately available) as I need both functional of course. So I'm out for now for further testing.
  • Summary: Restarting HA without any lock action (not sure if local button action would have triggered something to make the lock entities state switch from unavailable to locked or unlocked) leads to unavailable lock entities although MQTT is connected.

@technyon
Copy link
Owner

What path did you configure for the lock? And could you share the content of the HA discovery topic for the lock?

@bcutter
Copy link
Contributor

bcutter commented Oct 24, 2023

Could you please guide me precisely on how to get that information?

Afaik I never changed/customized things so everything should be default.

@technyon
Copy link
Owner

As a start, maybe post the content of the system information page.

Which MQTT broker are you using?

@mundschenk-at
Copy link
Collaborator

Could you please guide me precisely on how to get that information?

In HA: Settings > Devices & Services > MQTT > Your lock device > 3 dots > Download diagnostics

@bcutter
Copy link
Contributor

bcutter commented Oct 24, 2023

System information page (masked)

NUKI Hub version: 8.26
run: true
deviceId: XXXXXXXXXXX
deviceIdOp: XXXXXXXXXXX
mqttbroker: xxx.xxx.xxx.xxx
mqttport: 1883
mqttuser: ***
mqttpass: ***
mqttlog: false
lockena: true
mqttpath: nuki_doorone
openerena: false
mqttoppath: 
maxkpad: 5
opmaxkpad: 
mqttca: 
mqttcrt: 
mqttkey: 
hassdiscovery: homeassistant
dhcpena: true
ipaddr: xxx.xxx.xxx.xxx
ipsub: 255.255.255.0
ipgtw: xxx.xxx.xxx.xxx
dnssrv: xxx.xxx.xxx.xxx
nwhw: 1
rssipb: -1
hostname: ESP-Nuki-Hub-DoorOne
nettmout: -1
restdisc: true
rstbcn: 60
lockStInterval: 3600
configInterval: 3600
batInterval: 7200
kpInterval: 1800
kpEnabled: true
accLvl: 
regAsApp: false
nrRetry: 5
rtryDelay: 100
crdusr: ***
crdpass: ***
pubauth: true
pubdbg: false
prdtimeout: 60
hasmac: false
macb0: 
macb1: 
macb2: 
MQTT connected: Yes
Lock firmware version: 3.7.7
Lock hardware version: 5.11
Lock paired: Yes
Lock PIN set: Yes
Lock has door sensor: No
Lock has keypad: Yes
Network device: Built-in Wifi
Uptime: 1088 minutes
Heap: 85424
Stack watermarks: nw: 6104, nuki: 360, pd: 248
Restart reason FW: RestartOnDisconnectWatchdog
Restart reason ESP: ESP_RST_SW: Software reset via esp_restart.

As the MQTT -> lock -> burger menu -> diagnostics output is very very chatty, here's what I think you might have asked for:

        {
          "entity_id": "lock.schloss_doorone",
          "subscriptions": [
            {
              "topic": "nuki_doorone/lock/binaryState",
              "messages": [
               ...................................
                {
                  "payload": "locked",
                  "qos": 0,
                  "retain": 0,
                  "time": "2023-10-24T06:18:06.074445+00:00",
                  "topic": "nuki_doorone/lock/binaryState"
                },
                {
                  "payload": "unlocked",
                  "qos": 0,
                  "retain": 0,
                  "time": "2023-10-24T06:29:16.264524+00:00",
                  "topic": "nuki_doorone/lock/binaryState"
                }
              ]
            }
          ],

Here's the /homeassistant/lock/XXXXXXXXX/smartlock mqtt auto discovery topic:

{"dev":{"ids":["nuki_XXXXXXXX"],"mf":"Nuki","mdl":"SmartLock"},"~":"nuki_doortwo","name":"Door Two","unique_id":"XXXXXXXX_lock","cmd_t":"~/lock/action","pl_lock":"lock","pl_unlk":"unlock","pl_open":"unlatch","stat_t":"~/lock/binaryState","stat_locked":"locked","stat_unlocked":"unlocked","opt":"false"}

Hope that helps.

@bcutter
Copy link
Contributor

bcutter commented Nov 6, 2023

Going through the updates, but it takes some time, probably weeks.
2023.4 made no difference so far (and tbh I doubt it will be fixed with a HA update).

So annoying when updates break (core) things... 8.26 was running so smooth and stable. I get the "want have" factor for having lock entities unavailable depending on the MQTT connection state, but well... not working.

Maybe we can have this avty thing as config option? So being on as default this way everyone could simply decide on his/her own.

@technyon
Copy link
Owner

technyon commented Nov 6, 2023

I wonder if it could be somehow related to the broker. I think everyone else uses EMQX ... is this correct?

@mundschenk-at
Copy link
Collaborator

Well, if you mean @alexdelprete and me, yes, we are using EMQX. Not sure about everyone else. The broker should not make any difference here though, unless it is somehow possible to disable LWT in Mosquitto.

@bcutter Please look at your HA log to see if there is any issue with MQTT discovery messages not being parsed - the component is quite chatty. If not, try to enable debug logging for the mqtt component.

@alexdelprete
Copy link
Collaborator

alexdelprete commented Nov 7, 2023

I wonder if it could be somehow related to the broker. I think everyone else uses EMQX ... is this correct?

It's not broker related. It's an issue with @bcutter HA not parsing tha availability topic correctly for some unknown reason. Also, I would use availability_topic vs availability, since we only use one topic and not multiple topics to determine the availability status.

"avty_t": "~/maintenance/mqttConnectionState"

@bcutter can you try an older version of NH, pre 8.27, to exclude that the availability topic (introduced in 8.27) is an issue in your case? UPDATE: sorry, I read the whole thread, you already confirmed 8.26 works in your case.

@alexdelprete
Copy link
Collaborator

While I'm not 100 % sure what the detailed impact of this change on lock entities in HA is

That breaking change simply meanst that on HA startup, MQTT Locks have an unknown state, until the state is correctly determined by the relevant MQTT topic. Basically HA doesn't assume a default state, unless optimistic mode is configured.

@bcutter
Copy link
Contributor

bcutter commented Nov 7, 2023

Yes indeed, 8.26 works without any issues - because it does not have the avty set yet.

So many different approaches here (plenty of activity, appreciate that), not sure what to do / check next on my side. If you experts isolated the core issue please let me know what I can contribute.

I'm kinda sick and tired of updating NH to 8.27 and restarting HA just to see all my lock entities fail (unavailable) again, I've gone through that process several times lately. If I really need to do that one more time I want to know exactly what to check/do/provide you here.

@alexdelprete
Copy link
Collaborator

If you experts isolated the core issue please let me know what I can contribute.

The core issue seems to be specific to your setup, I haven't seen other reports after 8.27 with your same issue. Please correct me if I missed any. My installation works perfectly fine with 8.27 and my lock doesn't go unavailable.

If I really need to do that one more time I want to know exactly what to check/do/provide you here.

I suggested to change avty to avty_t, so when dev provides that test version you might want to test it. What version of HA are you running now? Can you also describe where HA and MQTT broker are running? RPi, VM, LXC, etc. Thanks.

@alexdelprete
Copy link
Collaborator

@bcutter can you tell me if the mqttConnectionState topic has the retain flag in the broker? I'm using MQTT Explorer to navigate the topics in the broker here.

image

@technyon
Copy link
Owner

technyon commented Nov 7, 2023

I suggested to change avty to avty_t, so when dev provides that test version you might want to test it. What version of HA are you running now? Can you also describe where HA and MQTT broker are running? RPi, VM, LXC, etc. Thanks.

I think you can test it without a new binary. Just start NUKI Hub and let it publish the discovery topic, then manually change avty to avty_t using some MQTT client. NUKI Hub will only overwrite the discovery topics on boot.

@mundschenk-at
Copy link
Collaborator

mundschenk-at commented Nov 12, 2023

OK, after sleeping over it, I now know why @bcutter's lock entities are unavailable with 8.27 - it's got nothing whatsoever to do with the availability topic, but is simply the same as #246: The new empty name in the auto-discovery topic is invalid before Home Assistant 2023.8.0.

@alexdelprete
Copy link
Collaborator

alexdelprete commented Nov 12, 2023

The new empty name in the auto-discovery topic is invalid before Home Assistant 2023.8.0.

So PR #225 causes the entities to be unavailable in HA < 2023.8.0 ?

If confirmed, we should put a note in readme of the repo, specifying HA 2023.8.0 is the minimum supported version. Maybe a pre-requirements paragraph.

Also, when these breaking changes are implemented, they should be highlighted as breaking in release notes so that users know they need to test it first.

@bcutter
Copy link
Contributor

bcutter commented Nov 12, 2023

Great finding, congratulations 👏🏼 👍🏼

Why is it empty at all? Why not setting it?

That way it would resolve any dependency of a HA version, no breaking change notice needed (which is now too late anyway for 8.27) etc.

@alexdelprete
Copy link
Collaborator

alexdelprete commented Nov 12, 2023

Why is it empty at all? Why not setting it?

It's an HA breaking change: read #217

image

That way it would resolve any dependency of a HA version, no breaking change notice needed

That's not true, if HA changes something components/integrations need to adapt to those changes, hence minimum supported versions of HA need to be specified.

which is now too late anyway for 8.27

not true either, there might be users with old HA versions that will install 8.27 and will read the instructions, so minimum requirements should always be mentioned since there are dependencies.

Then there are old users (like you) who simply upgrade, so the breaking change should also be mentioned in the release notes. So you would have read it.

@mundschenk-at
Copy link
Collaborator

@alexdelprete: Right, this should in the README but we didn't really know until now. @technyon doesn't use HA at all and I'm always on the latest version (since I find quickly adapting my configuration to any breaking changes means much fewer issues in the long run). Unfortunately, the Powers That Be at HA didn't highlight the fact that this auto-discovery change would prevent it from working with older versions of HA.

As a workaround, we could add name: Lock for the lock entities instead, which would result in the entity name lock.your_device_lock and a friendly name of Your Device Lock, but personally I think a simple note in the README should suffice at this point.

@bcutter:

Why is it empty at all? Why not setting it?

That way it would resolve any dependency of a HA version, no breaking change notice needed (which is now too late anyway for 8.27) etc.

Because that's the way it is intended: https://community.home-assistant.io/t/psa-mqtt-name-changes-in-2023-8/

If you want to stay on something older than 2023.8.0, you can always configure the lock entity yourself using YAML (even if you use auto-discovery for the other entities).

@alexdelprete
Copy link
Collaborator

Right, this should in the README but we didn't really know until now. @technyon doesn't use HA at all and I'm always on the latest version

we adopted a change due to HA recommendations, so we should mention that as a breaking change. we knew it when we implemented it. We were relying on the new 2023.8.0 way of working, to avoid warnings. Doesn't matter if @technyon uses HA or not, we knew the impact when we adopted those changes.

The general readme should always contain pre-requirements, and in the release notes breaking changes (such as a pre-requirement change) should be highlighted properly.

It's a general advice for the future. :)

@mundschenk-at
Copy link
Collaborator

mundschenk-at commented Nov 12, 2023

Well, at least I didn't know that the auto-discovery message would simply fail to parse on HA < 2023.8.0 (instead of using some sane default for the "missing" name) 🤷

@alexdelprete
Copy link
Collaborator

alexdelprete commented Nov 12, 2023

at least I didn't know that the auto-discovery message would simply fail to parse on HA < 2023.8.0

I understand. But in general, if you adopt a change based on HA version, you have to mention that as an important change and mention it. It's no big deal...but in the future we should not rely on intuitions or memory, we'd waste a lot of time for these issues.

Thank god you have a good memory and intuition. :)

@bcutter
Copy link
Contributor

bcutter commented Nov 12, 2023

So to sum up for my current stuck-on-NH-8.26-situation: I need to upgrade to HA Core 2023.8 to be able using NH 8.27. There will be no changes made on NH side except leaving a breaking change note in the release notes description.

Correct?

Not the best outcome (as I won't be able to upgrade HA Core to 2023.8 before mid of December at best) for me but at least something I can count with.

@mundschenk-at
Copy link
Collaborator

mundschenk-at commented Nov 12, 2023

You could also upgrade to 8.27 and configure the lock entity using YAML instead of relying on auto-discovery. But yes, staying on 8.26 until you can upgrade to HA >= 2023.8.x is probably the easier route. It's not that you are missing out on much.

@bcutter
Copy link
Contributor

bcutter commented Nov 12, 2023

I think I need to / will manage to look at this in my custom admin update dashboard for a while, sitting next to HA Core.

grafik
grafik

Really glad you found the root for this and I don't need to spend more time on testing etc. 👍

@technyon
Copy link
Owner

Hi. Thanks for figuring this out, I'll add a note to the readme.

@alexdelprete
Copy link
Collaborator

alexdelprete commented Nov 12, 2023

I need to upgrade to HA Core 2023.8 to be able using NH 8.27. There will be no changes made on NH side except leaving a breaking change note in the release notes description.

Why should NH make a change? It's in line with HA, your installation is not. 8.27 was made specifically for users who upgraded to latest HA versions and had that warning, and that's not you. So you should remain with version < 8.27.

It's not always possible to be background compatible when HA core changes something an integration uses.

@alexdelprete
Copy link
Collaborator

Hi. Thanks for figuring this out, I'll add a note to the readme.

Jan, could you also put a note in the release notes of 8.27? For users who will install it later...

@bcutter
Copy link
Contributor

bcutter commented Nov 12, 2023

It's in line with HA, your installation is not.

Well, expecting everyone uses latest versions of every component in the individual smart home eco system is a heavily dangerous state of mind imo.

Ideally software can technically avoid to install or even show updates when requirements are not met. Like in HACS where maintainers can precisely specify the requirements. Of course this is not possible for NH as it's a standalone project/firmware which makes it much more difficult to be as compatible as possible to the relevant smart home systems making use of the MQTT source. (sidenote: which is one reason for some wishing NH would be part of ESPHome so all these problems are semi-automatically taken care of, plus much smarter update management etc.)

So +1 for at least leaving notes in readme and release notes. I read them always, sometimes twice before upgrading.

Nothing more to add from my simple user / non-dev perspective. Thanks again for discovering this and taking care of.

Basically many of these posts here (starting with my first likely) belong to #217, not #227 as we know now.

@alexdelprete
Copy link
Collaborator

alexdelprete commented Nov 12, 2023

Well, expecting everyone uses latest versions of every component in the individual smart home eco system is a heavily dangerous state of mind imo.

We don't expect anything, we integrate with HA development cycle: if HA makes a breaking change, ALL integrations (not only NH) impacted will need to adapt.

It's pretty clear you are not a dev so you don't understand the lifecycle and the interdependencies of a component that relies on another software, and it's a very dangerous state of mind having such a strong opinion about something you clearly have no competencies or comprehension about.

You, as a user, can decide to stay on HA 0.19 of some years ago, but if you decide to do so, don't expect component developers to make sure your installation works.

Furthermore, there's no reason for you to upgrade to 8.27, since 8.26 works perfectly well for you, with no HA warnings. That is not the case for users on latest HA versions. So you're not impacted in any way functionality wise.

Our mistake was not clearly stating in docs and release notes about the new pre-requirements on HA version.

So +1 for at least leaving notes in readme and release notes. I read them always, sometimes twice before upgrading.

That's a very good practice, kudos to you. Unfortunately not many users do actually read them. They simply upgrade and expect everything to work, then start bragging about things that changed.

Thanks for your feedbacks, and next time before interpreting incorrectly why devs make some decisions and judging their state of mind, ask for more information, so you will understand the context and you will avoid potentially offending people only because you ignore important facts.

Cheers.

@technyon
Copy link
Owner

I've added a sentence or two to the readme and the release notes. I think there's little choice but to adopt these changes - although I have to say I'm not sure if it was really necessary to make this a breaking change from HA side. They could've just ignored the name and recommend not to put it for new integrations.

@alexdelprete
Copy link
Collaborator

although I have to say I'm not sure if it was really necessary to make this a breaking change from HA side. They could've just ignored the name and recommend not to put it for new integrations.

I agree. But HA devs are known to make these kind of decisions ignoring all the consequences to existing integrations / automations /etc. If you read the HA community, you'll find a lot of complaints about this. Unfortunately many integration/component devs take the blame for these decisions, while the root cause is bad decisions in HA.

But that's the way they see things, not much to do other than adapt, if you want to use HA, that I still believe is the best platform (for me obviously). :)

@technyon
Copy link
Owner

Well, they could act wiser, but it is as it is. I'll close this issue for now, since all problems are resolved (or at least understood).

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

No branches or pull requests

5 participants