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

Feature/issue 840 migrate to mosquitto #853

Merged

Conversation

emrysr
Copy link
Contributor

@emrysr emrysr commented Jun 5, 2018

@TrystanLea TrystanLea merged commit 50c22a1 into emoncms:master Jun 6, 2018
Copy link
Contributor

@borpin borpin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these comments are correect 😄

*/
//log errors connecting to mqtt
if ($responseCode > 0) $log->info($msg.$message);
$this->mqtt->publish($topic, $value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the publish be inside an else i.e. only publish if there is a successful connection? I suspect it will raise an exception if you try a publish with no connection.

Equally, if there is no connection, you should not disconnect.

I think, that if the connection fails the first time, the data will be lost as it will not retry.

Copy link
Contributor Author

@emrysr emrysr Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Borpin, thanks for looking over this.
We've moved to the Mosquitto MQTT client to make use of the callbacks and event-driven operations used in the library to avoid the issues you raised. Installed via PECL.

pecl install Mosquitto-alpha

I've called the Mosquitto::publish() method within the onConnect() callback. This will only attempt the publish once a successful connection is established.

The Mosquitto::loopForever() function will attempt re-connection if disconnected for whatever reason. And the onConnect() callback will disconnect once the messages have been published.

If there is an unsuccessful connection an error is sent to the error log.

I hope this helps. Let me know if you think I've missed anything.

Thanks again for your help - I had to double check that it was doing what is was supposed to be doing :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @emrysr,

We've moved to the Mosquitto MQTT

Yes, I suggested we did so.

I've called the Mosquitto::publish() method within the onConnect() callback. This will only attempt the publish once a successful connection is established.

The onConnect() callback is called even if there is an unsuccessful connection (hence values other than 0). The Mosquitto::publish() should only therefore be called if a CONNACK of 0 is received.

If this happens then as you say the Mosquitto::loopForever() function should reattempt the connection (does this need to time out?).

I suggest that if you want to disconnect on a sucessful publish, use the onPublish callback.

What is way beyond me, in PHP terms, is, once a connection has been made, does there need to be a disconnect() or could it simply be left to run. Making multiple connections is daft if you do not need to but I really do not know if the asynchronous nature of this actually work across separate procedural calls.

I've got #822 open to modify the mqttinput script as there was a similar issue in that the subscribe was happening before there was a connection and causing an exception. However, I have subsequently found (but not had time to do anything with) that a simpler mechanism (to flags etc) is to subscribe in the onConnect() where CONNACK is 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies @borpin I should have looked you up before I sent you the last message - I'm new!! :-\

Good spot! I've just changed it to only publish() where CONNACK is 0. else error logged.

Firing the disconnect() event within the publish() event would work fine in most cases. However in the few cases where it doesn't the loopForever() would cause the script to hang.

The error log should provide an accurate description of what needs to be fixed to allow the script to connect successfully.

98565cf

let me know if you think I've missed something. Thanks.

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.

3 participants