publish qos 0,1,2; loop() before publish and subscribes; others

[Suxsem]

• default MQTT_KEEPALIVE increased to 30 seconds
• esp8266 example updated with
  1. onDisconnected callback
  2. non blocking reconnect
  3. authentication
  4. last will message
  5. payload cast to string
• loop() now processes ALL available mqtt packets in the tcp buffer, not just the first
• publish(), subscribe() and unsubscribe() calls are preceded by loop() call to process incoming messages (and ACKS) without filling the TCP buffer when multiple pub/sub actions are performed
• publish() now supports QoS 0, 1 and 2. See the updated readme for more info
• improved comments in readPacket(uint8_t* lengthLength) to better understand what it does

Signed-off-by: Stefano Semeraro semeraro.stefano@hotmail.it

Resolve #98, #59, #55

[Suxsem]

ops...test suite should be updated to match the new publish() signature :)

[knolleary]

Sorry, but I don't accept pull requests that contain multiple changes that have had no discussion. Especially when they are contained in a single commit so it is impossible to distinguish them.

There are also a number of issues with the proposed changes. At a glance, they include:

• changed default keepalive with no explanation for why
• change to use a while loop in the loop function is incorrect. In certain circumstances it will mean loop never exits as it is continually processing incoming packets
• used tabs when the rest of the file uses spaces
• changed the signature of existing publish functions that will break all existing users - the regression tests are there for a reason.
• I have explicit not added QoS 2 for a reason

If there are specific changes you want to make, raise an issue so we can discuss.

[Suxsem]

I understand. Anyway i made those changes for myself and other people, because me and other students of my course (i'm an engineering student) need those features.

I shared them here because maybe you too find them useful.

If you want to pick a specific change go ahead and make a new commit, i will continue to use my fork.

I want to use this space to thank you for the wounderful library, the most solid mqtt library for esp8266 that I found (now even more solid in my fork :D)

[knolleary]

OK, thank you for the contribution, but please take my feedback on board.
It is good practice to have a single feature/fix per commit, especially
when you intend to contribute back to a project - it makes the project
maintainer's task much easier to see what has changed and more importantly,
why it has changed.

When I'm next near my laptop, I will look at the subscribe/loop issue
properly to see what handling is needed, if any, in this code.

Thanks, Nick

On Sun, 20 Dec 2015, 19:27 Suxsem notifications@github.com wrote:

I understand. Anyway i made those changes for myself and other people,
because me and other students of my course (i'm an engineering student)
need those features.

I shared them here because maybe you too find them useful.

If you want to pick a specific change go ahead and make a new commit, i
will continue to use my fork.

I want to use this space to thank you for the wounderful library, the most
solid mqtt library for esp8266 that I found (now even more solid in my fork
:D)

—
Reply to this email directly or view it on GitHub
#100 (comment)
.

[Suxsem]

Sure, I will do that definitely.
Just two words about the changes.

1. publish qos. I found in a your comment to an issue that you don't want qos 1 or qos 2 for memory issues (qos 0 doesn't need persistence). But if you have read the Readme changes in my commit you will now that retransmission is not implemented and qos 1 and 2 are only there for the broker - receiver communication. So, if the persistence is not the problem, what is it?

2. while instead of if in loop. The infinite loop can only happens if the board receives more message in a second than it can process. Considering that 99% of the time this doesn't happens, having the while loop can prevent tcp buffer to fills, causing board to disconnects. Moreover, if the board receives more messages than it can process, we have a bigger problem. So, what circumstances you are referring in loop never ends?

3. change in signature. We should add a new signature without removing the old one. Anyway this seems to me useless. You could add a warning in the changelog, pushing users to add qos=0 when they set retain to true. They will suddenly read the changelog when their code will not build.

4. keep alive was changed because all other libraries that I found (and other examples of mqtt clients like Java paho) have their default values far bigger then 30 seconds. Anyway it should be made user defined. What do you think?

5. tabs were a mistake, sorry :)

[knolleary]

1. not sure what you mean by 'qos1/2 are only there for the broker-receiver communication'. The qos flows are just as important in either direction. But as I think about it some more, the only scenario where persistence is required is if the client supports non-clean sessions - as it would have to retry outstanding q1/2 publishes after a reconnect. But as this client only supports clean session connections then it is feasible to added q1/2 publish.
2. we had a recent issue where someone hit exactly this scenario after modifying the code as you had. It isn't that unlikely - you just need a fairly steady stream of incoming messages. There may be some trade off to be had - let client.loop handle more than one inbound message, but only to some limit, for example 5.
3. I take backwards compatibility very seriously. If it does have to break something, then the major version number needs to be increased to stick by the rules of semantic versioning. But I'm not convinced it does have to break the existing signatures.

[Suxsem]

1. let's start with the fact that retransission on esp8266 is not possible due to memory problems. So the esp8266 will always publish to the mqtt broker with no deliver guarantee; now, when the esp8266 send a message, it first deliver it to the broker, then the broker deliver it to the subscriber. let's focus on the delivering of that message to the subscriber (from the broker). The qos of that process it's the maximum between message qos and subscribe qos.
   So, if the subscriber (for example my smartphone) subscribes with qos=2, but the esp8266 sends it with qos=0, not only the deliver to the broker has qos=0, but ALSO the deliver from the broker to the subscriber.
   My english it's not good, I hope you can understand me :D

2. i can't understand how avoiding the while loop have fixed the problem of this guy. if the while loop never exited, that's mean that the incoming stream it's so quick that the esp can't process it. The only situation that could cause problems is when the mqtt messages are so big that the loop() finds the first bytes but then it must wait for the message to be entirely received.
   I think that we could edit the code in this way: only loops the wificlient.available() until the mqtt header is received. then check if the wificlient.available() is equals to the remaining length found inside the header. if not, exit the client.loop() and check again in the next client.loop(), if yes then the message is already entirely in the tcp buffer and can be process without wasting time!

3. i'm sure you have more experience then me in those things :)

[MSERTER]

hi. what is the version of the pubsubclient library that supports all QoS qualities? where can I get to this library?