Add HomeAssistant time controls

[lupine]

This PR links up the existing ac_change / charge_priority / forced_discharge timeslot configuration topics with homeassistant autodiscovery. To do this, it uses the HA "text" type: https://www.home-assistant.io/integrations/text.mqtt/

The text type is supported in recent homeassistant versions - I'm running the 2023.2 version to test, but a full list is available from this commit: home-assistant/core@2785b2b

Here's a screenshot of it in action:

Relies on changes in #147 - that should be merged first. The commit is here too, so the branch works as a whole.

Closes #134

[lupine]

Trying to make use of the min and max attrs (setting both to 11) got me:

2023-02-13 23:54:16.933 ERROR (MainThread) [homeassistant.util.logging] Exception in async_discover when dispatching 'mqtt_discovery_new_text_mqtt': ({'name': 'AC Charge Timeslot 1', 'state_topic': 'lxp/BAXXXXXXXX/read/ac_charge/1', 'command_topic': 'lxp/BAXXXXXXXX/set/ac_charge/1', 'command_template': '{% set parts = value.split("-") %}{"start":"{{ parts[0] }}", "end":"{{ parts[1] }}"}', 'value_template': '{{ value_json["start"] }}-{{ value_json["end"] }}', 'unique_id': 'lxp_BAXXXXXXXX_text_ac_charge/1', 'device': {'manufacturer': 'LuxPower', 'name': 'lxp_BAXXXXXXXX', 'identifiers': ['lxp_BAXXXXXXXX']}, 'availability': {'topic': 'lxp/LWT'}, 'mode': 'text', 'min': 11, 'max': 11, 'pattern': '\\d{2}:\\d{2}-\\d{2}:\\d{2}', 'platform': 'mqtt'},)
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/voluptuous/schema_builder.py", line 818, in validate_callable
    return schema(data)
  File "/usr/src/homeassistant/homeassistant/components/mqtt/text.py", line 70, in valid_text_size_configuration
    raise ValueError("text length min must be >= max")
ValueError: text length min must be >= max

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/voluptuous/schema_builder.py", line 272, in __call__
    return self._compiled([], data)
  File "/usr/local/lib/python3.10/site-packages/voluptuous/schema_builder.py", line 820, in validate_callable
    raise er.ValueInvalid('not a valid value', path)
voluptuous.error.ValueInvalid: not a valid value

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/mqtt/mixins.py", line 334, in async_discover
    config: DiscoveryInfoType = discovery_schema(discovery_payload)
  File "/usr/local/lib/python3.10/site-packages/voluptuous/validators.py", line 232, in __call__
    return self._exec((Schema(val) for val in self.validators), v)
  File "/usr/local/lib/python3.10/site-packages/voluptuous/validators.py", line 355, in _exec
    raise e if self.msg is None else AllInvalid(self.msg, path=path)
  File "/usr/local/lib/python3.10/site-packages/voluptuous/validators.py", line 351, in _exec
    v = func(v)
  File "/usr/local/lib/python3.10/site-packages/voluptuous/schema_builder.py", line 276, in __call__
    raise er.MultipleInvalid([e])
voluptuous.error.MultipleInvalid: not a valid value

which seems... inaccurate. I'll remove them for the moment.

[lupine]

OK, removing min and max gets me a control:

The value unknown is invalid, but if I set it to 00:00-12:00 then I see a correct-looking MQTT message:

lxp/BAXXXXXXXX/set/ac_charge/1 {"start":"00:00", "end":"12:00"} 

I want to add tests and see if I can't get it to actually read the current state though, it's annoying that it doesn't, because the format to use in the field is then non-obvious. I'll see what I can do on that before taking it off draft.

[lupine]

Without this, the message for ac_charge/1 ends up nested a level deeper than HA expects. It's not obvious so I decided to centralise it.

[celsworth]

Probably just deserves a code comment but yeah seems fine.

(or better, a unit test with a supporting comment)

[lupine]

Test is a very good idea 👍 - will add before I take it off draft.

[lupine]

I had a go at the test and struggled a bit 😬. It can't go in in tests/test_home_assistant.rs because it's a private method, and I didn't like the idea of pulling that site's common code for setup into src/home_assistant.rs.

So, I bailed out and just added the comment, plus another of the HA discovery message tests that implicitly exercises this code 😅

[lupine]

https://github.com/home-assistant/core/blob/master/homeassistant/components/mqtt/discovery.py#L46 shows us all the available controls; just to confirm that there isn't a time, much less a "time range with minute resolution".

[lupine]

@celsworth I wonder what you think of the approach to getting holding register data into home assistant that I've taken here in 64019634d205fb4a86215d6843cf0e8514e27b98 ? Obviously, the sleep() can't stay, but I'd love to get your thoughts on it before putting more effort in.

The idea is that we publish the mqtt messages mentioned in the HA state_topics once on startup, so HA has something to display. The controls then look a lot better:

(I still need to get display of the time fields working, but the others would previously say "unknown" too).

I also wonder if, since holding registers have configuration data, these could also be made into retained topics? Since HA stores state in its database, we probably don't need this for the common case (HA restarts after lxp-bridge has run in its presence at least once), but for the initial setup case (lxp-bridge starts, then HA starts for the first time) it'd make a difference.

[celsworth]

@celsworth I wonder what you think of the approach to getting holding register data into home assistant that I've taken here in 6401963 ? Obviously, the sleep() can't stay, but I'd love to get your thoughts on it before putting more effort in.

The idea is that we publish the mqtt messages mentioned in the HA state_topics once on startup, so HA has something to display. The controls then look a lot better:

I think this approach is fine and it's something that has been considered before so would be happy to have this go in.

Obviously though I think it should be a PR of its own and also should be behind a configuration switch. Probably best to default it to off for now?

The sleep is a result of you hitting the limitations behind my task control :) Basically channels are setup between all the tasks. Remember all this code is single threaded, but async. As each task gets its turn on the CPU and runs, it subscribes to a channel to listen for "things to do" essentially. If you publish to the other end of that channel before the task has subscribed, you get this "channel closed" error.

Until now everything works because nothing publishes before all the tasks are subscribed, but now you've added a publish that could run at any point, even the first thing (not sure if its deterministic) before the MQTT sender has subscribed.

The fix is probably to make this all a bit smarter, unfortunately that also means making it a fair bit messier, probably via yet more channels that are essentially used for signalling that a particular task is ready for messages. Then your new method here could wait to receive that message before doing its own publishes. If we start a new PR with this change in, that would be the place to experiment with that probably. I'd be happy to have a go helping with it.

I also wonder if, since holding registers have configuration data, these could also be made into retained topics? Since HA stores state in its database, we probably don't need this for the common case (HA restarts after lxp-bridge has run in its presence at least once), but for the initial setup case (lxp-bridge starts, then HA starts for the first time) it'd make a difference.

Retaining the holding topics has also been mentioned actually. I do quite like the idea, as in ideal circumstances, the only time most of them should change is when written to (there are exceptions, the current time for instance) and usually we see the new value that has been written when the inverter replies. I say usually because again thats not always true; a WriteSingle packet reply from the inverter contains the new value, but a WriteMulti does not :(

If we did retain the topics here we should probably read all of them at startup to have some confidence they're correct, and maybe even read registers that we see have been written to with WriteMulti. So I think this has caveats. The first idea above is probably safer and easier.

[celsworth]

The fix is probably to make this all a bit smarter, unfortunately that also means making it a fair bit messier, probably via yet more channels that are essentially used for signalling that a particular task is ready for messages. Then your new method here could wait to receive that message before doing its own publishes. If we start a new PR with this change in, that would be the place to experiment with that probably. I'd be happy to have a go helping with it.

Thinking about this further, even this might not be a great fix because we'd probably run into the opposite problem; your new method might not have subscribed to the signal channel before the MQTT sender tries to tell it its ready! This is awkward :)

Maybe we need a shared state object that is basically a key/value store to say when various components of the app are ready. The getter would probably just end up being an async poller with a sleep; just a glorified more responsive version of the sleep you've already got really. Not sure if its worth it..

Edit: having a play with the above idea using condition variables https://docs.rs/tokio/latest/tokio/sync/struct.Notify.html# atm, will report back :)

[celsworth]

Something like #144 (WIP) might do the job to avoid the janky sleep. Haven't tested yet and I don't think there's anywhere else its actually needed yet. I'll finish it up later, fix tests and pass it into all the subsystems so at least its ready for use.

[celsworth]

Actually, maybe Sender#receiver_count is a simpler solution. We could just wait for it to go above zero with a short wait and timeout system before aborting. No need for an extra impl/struct at all then..

[lupine]

Thanks for looking!

Hmm, yeah, receiver_count is nice and simple. I'll put up a separate PR with the non-time-field holding registers and we can take it from there. It makes for a sensible change all by itself without these time range additions.

[lupine]

Hmm. Actually, it's toosimple - we can wait patiently for the to_inverter channel to get its subscribers, but won't we have exactly the same problem when we try to send a message out via MQTT?

Looking at the code a bit more, the inverters don't subscribe to from_inverter or to_inverter until after it's established a TCP connection to the remote. The coordinator subscribes to from_inverter almost immediately, so this runtime error probably doesn't happen in real life, but there's no guarantee that it wouldn't happen in the existing code.

Similarly, even if inverter and coordinator subscribe as expected, the MQTT server doesn't subscribe until after it's made its TCP connection, so we could get the same runtime error there as well - and indeed, if the MQTT server is slow, there's nothing in the existing code stopping us from getting a broadcast from the inverter before that happens, leading to the same runtime error.

I'll have a play and see what I can come up with.

[lupine]

On the topic of configuration - I did wonder if it might make sense to use the scheduler for this. We could have an @reboot line, just like real cron, to express "emit the holding registers when lxp-bridge starts", but people would still be free to configure hourly or minutely broadcasts if those would be useful to them. I know the scheduler's got some timezone problems which might need looking at, but I wonder how you feel about this approach assuming those could be solved?

If that's not the way to go, I was thinking simple yaml like:

mqtt:
  homeassistant: {...} # not in this block because it's not actually HA-specific
  # ...
  publish_holding_registers_on_startup: true

WDYT?

I also had the thought that maybe it should be implemented per-inverter instead:

inverters:
  - enabled: true
    # ...
    heartbeats: true
    publish_holding_registers_on_startup: true

? Not sure offhand how that would look with the scheduler approach, but I'm sure it's possible.

[celsworth]

As things stand, I do favour publish_holding_registers_on_startup I think.

If the scheduler problems can be sorted that could be pretty novel, but I wonder if its getting a bit complex for what 99% of people will need this to do. I'm in two minds about removing the scheduler, the timezone issues are pretty annoying. That said, I do have the timesync running on my inverter and it works well most of the time, just breaks and crashes lxp-bridge in DST transition times :(

[lupine]

This change publishes a lxp/XX/ac_charge/1 {"start":"00:00","end":"23:59"} message for us when the command is run. Without it, Home Assistant assumes the attempt was a failure and resets the text field back to the old value after ?some time? ?a restart of lxp-bridge?

The changes to the underlying register are already published, but of course, we don't use them in state_topic.

[lupine]

I had a couple of little bugs with topic names, but the command_template is working seamlessly. Quite surprised!

I don't think we can avoid a dependency on a single MQTT message for both registers here, or at least not without being a lot cleverer than I am with HA.

[celsworth]

Nice, this looks good. I think we're reaching a natural breaking point to make a release, so unless any showstoppers crop up soon then I'll get 0.10 out as the changelog is getting quite big ;)