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

MQTT: refactor messages for better home assistant integration #134

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jbaudoux
Copy link
Contributor

@jbaudoux jbaudoux commented Jun 8, 2020

I wonder if we should not change the way the messages are published to be more close to the several integrations existing in home assistant
https://www.home-assistant.io/integrations/mqtt/

This would allow in the future to also listen to "..../set" topics to receive commands

I know that json can be parsed in home assistant, but as the MQTT feature has not yet been released, it's still the time to ask our self if we should not do this small change to ease the integration.

Relates to initial issue #103 and my comment here #130 (comment)

Not tested

Before

  • system start
    topic: opensprinkler/system
    payload: {"state":"started"}
  • station start
    topic: opensprinkler/station/<id starting at 0>
    payload: {"state":1}
  • station stop
    topic: opensprinkler/station/<id starting at 0>
    payload:
    if flow sensor configured: {"state":0,"duration":[integer],"flow":[float]}
    else: {"state":0,"duration":}
  • measured flow
    topic: opensprinkler/sensor/flow
    payload: {"count":[integer],"volume":[float]}
  • rain detection
    topic: opensprinkler/sensor/rain
    payload: {"state":0/1}

After

  • system start
    topic: opensprinkler/system/power_state
    payload: on
  • station start
    topic: opensprinkler/station/<id starting at 0>/state
    payload: {"state": "on"}
  • station stop
    topic: opensprinkler/station/<id starting at 0>/state
    payload:
    if flow sensor configured: {"state": "off", "duration":[integer],"flow":[float]}
    else: {"state": "off", "duration":[integer]}
  • measured flow
    topic: opensprinkler/sensor/flow/state
    payload: {"count":[integer],"volume":[float]}
  • rain detection
    topic: opensprinkler/sensor/rain/state
    payload: "on"|"off"

@jbaudoux
Copy link
Contributor Author

jbaudoux commented Jun 8, 2020

@jbaudoux jbaudoux mentioned this pull request Jun 8, 2020
@zachfi
Copy link

zachfi commented Jun 9, 2020

Oh rad. I'll upgrade for this change.

@rayshobby
Copy link
Member

I am not sufficiently familiar with Home Assistant to tell. From the firmware point of view, it would make sense to maximally reuse what the HTTP API already has. For example, to trigger a manual program run, the client can publish a message to topic /mp, with payload the same format as the HTTP API command (or to make it conform with json format, the payload could be something like {"cmd":"xxxxx"}. Reusing existing code paths can help minimize the amount of work and also make it easy to adapt when we introduce additional API commands in the future.

@zachfi
Copy link

zachfi commented Jun 10, 2020

This is where I've been reading to implement some code that reads from MQTT to do useful things, like export metrics to a prometheus exporter.

https://www.home-assistant.io/docs/mqtt/discovery/

I don't use home assistant, but the structure there seems to be where lots of IOT development is rallying around.

@jbaudoux
Copy link
Contributor Author

Thanks for the feedback. I feel it's going in the right direction. I have updated the PR description with a new proposal 2:

  • system start
    topic: opensprinkler/system/power_state
    payload: on
  • station start
    topic: opensprinkler/station/<id starting at 0>/state
    payload: {"state": "on"}
  • station stop
    topic: opensprinkler/station/<id starting at 0>/state
    payload:
    if flow sensor configured: {"state": "off", "duration":[integer],"flow":[float]}
    else: {"state": "off", "duration":[integer]}
  • measured flow
    topic: opensprinkler/sensor/flow/state
    payload: {"count":[integer],"volume":[float]}
  • rain detection
    topic: opensprinkler/sensor/rain/state
    payload: "on"|"off"

I haven't updated the PR yet, I'm waiting for your comments

@Nick-Adams-AU
Copy link

Should we also insert a unique id or MAC in the topic so we can support multiple units?

opensprinkler/00:11:22:33:44:55/system/power_state

@jbaudoux
Copy link
Contributor Author

The right approach for this would be to allow to configure the node name through the interface when MQTT is enabled. The node name is the first element in the topic and it is currently hardcoded to opensprinkler.

Should we also insert a unique id or MAC in the topic so we can support multiple units?

opensprinkler/00:11:22:33:44:55/system/power_state

@zachfi
Copy link

zachfi commented Jun 12, 2020

In terms of an ID for the device, I like this approach pretty well:

https://github.com/AnaviTechnology/anavi-thermometer-sw/blob/master/anavi-thermometer-sw/anavi-thermometer-sw.ino#L1722

You have to keep inventory either way, and I do still want to know the MAC somehow, but perhaps reading from ESP.getChipId() makes sense as well.

The rest of what you have there I think is a great start.

That Anavi code does something else interesting which is to publish a "config" about the object when a new MQTT client connects to the broker. I've not dug in completely on how that is being achieved, but its easy to simulate with mosquitto_sub. Perhaps the MAC could be part of that payload.

@Nick-Adams-AU
Copy link

In terms of an ID for the device, I like this approach pretty well

We might be covered already by the work done in #124.

@jbaudoux
Copy link
Contributor Author

I have updated the PR with my proposal 2

@nagyrobi
Copy link

Please push after boot not only availability, but also the state of the variables like current water level, rain delay, operation status.

@PeteBa
Copy link
Contributor

PeteBa commented Jun 27, 2020

Some thoughts on the messages below:

system start
topic: opensprinkler/system/power_state
payload: on

I think the availability topic handles the "powered on" scenario so we could slightly re-purpose this topic to notify of "system restart" given this can occur not just on a power cycle but also via watchdog timer and as soft-restart via the UI. Also, it would be useful to add the restart reason, i.e. based on os.last_reboot_cause, to the payload to allow for an informative notification.

station start
topic: opensprinkler/station/<id starting at 0>/state
payload: {"state": "on"}

Should we add intended duration to the payload given that it can vary based on watering level.

station stop
topic: opensprinkler/station/<id starting at 0>/state
payload: if flow sensor configured: {"state": "off", "duration":[integer],"flow":[float]}
else: {"state": "off", "duration":[integer]}

I think it would be better to add volume and make flow the average during the run. Unfortunately, the flow and volume are not accurate if multiple stations are running at the same time but that is a limitation in the existing notifications as well.

measured flow
topic: opensprinkler/sensor/flow/state
payload: {"count":[integer],"volume":[float]}

I know this matches IFTTT message but not sure of the use case for sending count given already sending volume.

This message is only generated when the programme queue is emptied and I'm not sure how people use this to be honest. I think we should move to something more like the App where the current flow rate is published say every 30 seconds. That would allow users to setup alarms if the flow rate is below a certain value when a particular station is running or flow is present when system is idle.

rain detection
topic: opensprinkler/sensor1/state
payload: "on"|"off"

Should this use the more generic opensprinkler/sensor/1/state format similar to stations ?. I notice that you use 0|1 in the code rather than "on"|"off". Is there a reason for that ? Also, do we need a principle that all state information should be a json object e,g. { "state":"on", "type": "soil", ...} to allow for additional information to be added in future ?

@ruimarinho
Copy link

@jbaudoux do you plan on getting these changes merged? The MQTT version has been working great for me -- would be really interesting to start accept /set commands and getting more information vis-a-vis the REST API.

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 this pull request may close these issues.

7 participants