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

Use a persistent MQTT connection #823

Closed
wants to merge 1 commit into from
Closed

Conversation

pb66
Copy link
Collaborator

@pb66 pb66 commented Apr 15, 2018

See the "A question over whether emonCMS is compatible with MQTT " OEM forum post.
Construct MQTT client to use a "persistent session" rather than a "clean session"

PLEASE DO NOT MERGE WITHOUT TESTING FIRST.

See the ["A question over whether emonCMS is compatible with MQTT "](https://community.openenergymonitor.org/t/a-question-over-whether-emoncms-is-compatible-with-mqtt/7180?u=pb66) OEM forum post.
Construct MQTT client to use a "persistent session" rather than a "clean session"
@glynhudson
Copy link
Member

What's the use case for this? Sorry, I've not been following the thread

@borpin
Copy link
Contributor

borpin commented May 10, 2018

@pb66 I've found a reason not to use the full apikey. The mosquitto log, logs it in plain text. If you are using a remote MQTT broker, that is probably undesirable! Use a portion of it perhaps? Other option would be to generate a key specifically for MQTT in the same way the API key is generated.

@pb66
Copy link
Collaborator Author

pb66 commented May 11, 2018

Yeah, using the apikey isn't perfect for a few reasons, I was just trying to avoid a hardcoded clientid. As discussed in the forum thread, I think a client id should be defined in the mqtt settings eg

$mqtt_enabled = false;          // Activate MQTT by changing to true
    $mqtt_server = array( 'host'     => 'localhost',
                          'port'     => 1883,
                          'user'     => '',
                          'password' => '',
                          'basetopic'=> 'emon',
                          'clientid`=> 'blah_de_blah`
                          );

This would separate the instances clientid from any one user whilst allowing multiple instances/clientids per machine (ie avoid using mac id), plus it allows for a new clientid to be defined with minimal fuss, if needed.

@borpin
Copy link
Contributor

borpin commented May 11, 2018

Yes, probably the best solution. If that change to the settings was going to be made, I think it would be worth creating the ability to use more than one topic at different QOS at the same time (changing the settings file is a pain!).

A JSON like array with 2 key value pairs of Topic & QOS I think would work. By default it would have `/emon/#' (make the subscription topic explicit), but it could be easily extended by users and the code to extract the topics to subscribe to should be reasonably straightforward.

@glynhudson
Copy link
Member

This sounds great, how is testing going for you? Shall I merge into master to open up the testing pool? All (or the majority) of emonPi/emonBase/emonSD users are running Stable.

@borpin
Copy link
Contributor

borpin commented May 17, 2018

It's working fine for me, but it should not be merged with the API key as the client ID as that is stored in plain text in any broker log.

@pb66
Copy link
Collaborator Author

pb66 commented May 17, 2018

Merging this will have an adverse effect on anyone using non-timestamped mqtt which is 99.99% of users (Brian uses timestamped MQTT). Can we keep the discussion in the forum, in one place please? At the very least we need to have a client id in the settings.

It's all in the forum thread why this must be in conjunction with either a change in the way we post MQTT (to use a timestamp) or until such time as we do that, change the QoS back to 0 to remove the chance of duplicated and significantly delayed data getting a current timestamp.

We currently lose the "late" data due any new connections being a clean slate, no buffered data. When you add a client id it will mean that all the missed/buffered data will get all posted as "now" no matter how old it is.

The golden rule here is that timestamped data can be or in fact should be QoS2, energy data without a timestamp is only valid for 5-10 secs, then it is stale, irrelevant and misleading. The fact we don't use clientids and we keep reconnecting almost compensated for using QoS2 with untimestamped data.

We need to review how we do MQTT or just accept it isn't right and leave it as is or make SOME corrections and roll back to QoS0 temporarily.

The issues causing the disconnection and reconnections are effectively clearing the buffer of stale (untimestamped) data because a new client id is issued and the 5s timeouts are ensuring the disconnections don't go too long without getting reconnected

@borpin
Copy link
Contributor

borpin commented May 17, 2018

I'd agree with Paul, I'm using it for either data that is time stamped, or where time does not really matter (temperature). The fact it works is only half the story!

@TrystanLea
Copy link
Member

Id be happy with a clientid set in settings.php, good idea.

@TrystanLea
Copy link
Member

We currently lose the "late" data due any new connections being a clean slate, no buffered data. When you add a client id it will mean that all the missed/buffered data will get all posted as "now" no matter how old it is.

That's interesting, so does that mean that our recent change to:

$mqtt_client = new Mosquitto\Client('emoncms',true);

should result in data being buffered in mosquitto if mqtt_input is stopped?

I've tried testing it here and it doesnt seem to be buffering?

@borpin
Copy link
Contributor

borpin commented Jul 15, 2018

should result in data being buffered in mosquitto if mqtt_input is stopped?

I think you are looking at it from the wrong end. If some data has been sent to the broker by the emonhub for instance, but has not been delivered to emoncms, as the connection has dropped and emoncms then reconnects, the broker should send the unsent data if a persistent connection is used and (I think) a QoS of greater than 0 is used for both the publish and the subscription. @pb66 has played with this more than me though 😄 .

These settings are all inter-related.

@borpin borpin mentioned this pull request Jan 17, 2019
@borpin
Copy link
Contributor

borpin commented Feb 7, 2019

@pb66 #1160 has been merged which I think should close this off. Do you concur?

@TrystanLea TrystanLea closed this Jul 3, 2019
@TrystanLea TrystanLea deleted the pb66-patch-phpmqtt_input branch October 1, 2019 15:28
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.

4 participants