Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

Retain holding and parameter registers #154

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

lupine
Copy link
Contributor

@lupine lupine commented Mar 27, 2023

Builds on top of #143

Home Assistant is much more pleasant to use with these controls built on top of holding register data if the messages are retained. Otherwise, a restart of home assistant results in the fields being set to "unknown" until a restart of lxp-bridge, or use of the control.

@lupine lupine changed the title Retain holding and Param registers Retain holding and parameter registers Mar 27, 2023
@lupine lupine force-pushed the nt-retain-holding-regs branch 4 times, most recently from 6a86330 to a241473 Compare March 27, 2023 14:41
@celsworth
Copy link
Owner

Are we happy that this is always set to true, no configuration option for it?

Not a leading question :) It might well be that it makes sense to make this change permanent and never not retain these.

@lupine
Copy link
Contributor Author

lupine commented Mar 27, 2023

@celsworth perfectly fine question ^^.

Certainly for me, it either makes sense to retain these registers or it doesn't, and there isn't much value in making it configurable. The way it makes HA more usable is a good argument for them being retained, IMO. Probably the best time to consider adding configuration for it is the point where someone asks for that functionality?

I did have a minor crisis of confidence over registers 12, 13, and 14 - the current time - where the data quickly gets out of date. It feels like perhaps retaining those registers is a bad idea (and perhaps the same for some others?) - but that's a further argument against configuration, I think. If we start making decisions about what's best per-register, and also allow configuration overrides of that, it starts to get complicated quickly.

@lupine
Copy link
Contributor Author

lupine commented Mar 27, 2023

Assuming we want this, do we want it to be part of 0.10, since it forms part of the same logical block of functionality?

@lupine lupine marked this pull request as ready for review March 27, 2023 14:52
@celsworth
Copy link
Owner

Ok, lets just go with making them retained and if someone asks for the config we can add it then. Do we want to think about a way to cleanup retained topics when lxp-bridge exits? Just wondering if we should be good MQTT netizens and not leave topics lying around potentially forever.

Registers 12/13/14 are a bit odd, I think they're a special case in that they're the only ones that can change themselves. It seems pretty unlikely anyone would rely on them being up to date in MQTT so I'd be happy with a proviso in the documentation that those can't be used from the retained topics reliably.

And yes, lets get this into v0.10 as it seems like it rounds off the HA functionality nicely.

@lupine
Copy link
Contributor Author

lupine commented Mar 27, 2023

Do we want to think about a way to cleanup retained topics when lxp-bridge exits? Just wondering if we should be good MQTT netizens and not leave topics lying around potentially forever.

Do we know what that would look like? We do have the overall lxp/LWT topic which, e.g., home assistant will use to effectively ignore these retained messages... do you think that we should have a similar mechanism for each of these retained messages as well?

I'm very much an MQTT n00b so don't know what the correct etiquette is here 😅

@celsworth
Copy link
Owner

Do we know what that would look like? We do have the overall lxp/LWT topic which, e.g., home assistant will use to effectively ignore these retained messages... do you think that we should have a similar mechanism for each of these retained messages as well?

I think its a slightly different problem, the LWT is probably fine to be left hanging around because as you say thats what HA and other things can use to determine whether lxp-bridge is alive or not.

In fairness we probably can't guarantee that we remove the holding topics when exiting as we might be doing it in an uncontrolled manner anyway, panicing, lost connection to MQTT etc. So maybe this is just worrying too much, we can just leave them :)

@lupine
Copy link
Contributor Author

lupine commented Mar 27, 2023

MQTT 5 would allow us to have an expiry time on the messages, which would be the ideal low-effort solution... but I recall you mentioning that we're pinned to mqtt 3 at the moment.

@celsworth
Copy link
Owner

I'd not checked for a while, there's a mention of it in a recent rumqttc blog post - https://bytebeam.io/blog/rumqtt-r19-persistence-10k-load-and-public-infra/#🧭-mqtt-5-roadmap

But not sure of the latest progress. Once its available in that crate we could start using it..

@lupine lupine mentioned this pull request Mar 27, 2023
@celsworth celsworth merged commit dd9118c into celsworth:master Mar 28, 2023
@celsworth
Copy link
Owner

I've added a brief explanatory line into https://github.com/celsworth/lxp-bridge/wiki/MQTT-Publishes about the new retain behaviour. Can probably be fleshed out later, maybe it deserves a mention in the HomeAssistant page too. In fact the HA functionality has grown so much that page probably needs a revamp entirely.

@lupine lupine deleted the nt-retain-holding-regs branch March 29, 2023 10:52
@lupine
Copy link
Contributor Author

lupine commented Mar 29, 2023

Great, thanks for the merge and docs update!

I've added a short note about publish_individual_holdings and also opened up #158 , which might be worth getting into 0.10.0 as well?

It might be worth having a list of all controls, and the version they were added, on that page too; I'll see if I can put something together.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants