Open r-xyz opened 2 months ago
Hi, you can look into my fork, i added a timestamp to each message and i use it to che k if the data is fresh 😁
Hi @r-xyz ,
Those improvements all seem very reasonable! However I would suggest a few small changes:
Item 2: It is possible that an existing user has defined a register named "modbus" and that would conflict with the proposed online/offline status message. I would suggest publishing these messages to <prefix>/modbus4mqtt/modbus_status
, or something similar. Also, the status message contents could provide a timestamp, as @pki791 has done, to ensure a stale online
message can be detected by a consumer of the message? Perhaps a message like {"status": "online", "timestamp": "2024-09-24T01:23:45Z"}
?
Item 3: Could you please explain the benefits of this change in more detail?
I'm assuming the will_set()
calls in the online/offline code snippets are just a typo? Should they be publish()
?
Please note that the best PRs tick the following boxes:
These help keep a FOSS project humming along smoothly.
Cheers, tjhowse.
Hi @tjhowse and @pki791 , Thanks for the feedback.
Following up: Item 1: OK Item 2:
will_set()
in this case was indeed a typo, apologies. I meant publish()
.Item 3:
connect_modbus()
after connect_mqtt()
in self.connect()
would provide better feedback via MQTT.Item 4:
modbus4mqtt
will become indeed a Last Will Topic, and modbus4mqtt/modbus_status
will also have similar aim. In my understanding, they should be preferably retained by default. Otherwise I could add an lwt_retain
parameter.Will hopefully send a PR in the upcoming days. Cheers, r-xyz
If you prefer, I can also change modbus4mqtt
message to JSON like that
{"status": "offline/online", "version": "v0.7.0","timestamp": "2024-09-24T01:23:45Z"}
Thanks for your patience on this one – I've been busy with other work. I should have some time to do the review soon.
The format of the "modbus4mqtt v{} connected" message will have to remain unchanged until we do a major version number increase, as there's a good chance of breaking something. Adding the "modbus4mqtt v{} disconnected" message has some risk, but low enough, I feel.
Cheers, tjhowse.
Thanks and no worries. :)
I could alternatively leave modbus4mqtt
as it is, and add LWT in JSON format in modbus4mqtt/status
.
It should not add much overhead to the MQTT server (modbus4mqtt
is only published on connection), and this way you can decide if deprecating the old format/topic in the next major release.
Dear @tjhowse , Thanks for this nice piece of software. :)
I would like to improve the information regarding modbus connection status. This might help to use it as availability topics, as well as enabling clients to detect failures directly from MQTT data. Before proceeding to a PR (if the repository is still active) I would like to check with you the best way to implement this.
Current status
<prefix>/modbus4mqtt
.Suggested solutions (feedback welcome)
connect()
on MQTT client. Content can actually be changed to just an empty string.connect_modbus
(successful connection), adding a conditional statement to check that MQTT is connected.poll()
failure:connect_modbus()
andconnect_mqtt()
at startup to avoid the extra conditional above. Not sure about implications.self.prefix+'modbus'
andself.prefix+'modbus4mqtt'
Looking forward for any feedback to contribute. Thanks in advance.