256dpi / arduino-mqtt

MQTT library for Arduino
MIT License
1.01k stars 230 forks source link

publish --> Mark as Duplicate #293

Closed gonzabrusco closed 6 months ago

gonzabrusco commented 1 year ago

Hello. First I would like to thank you for the great work.

I know that the library supports QoS1 but in reality, it's support is kind of halfway. I'm not asking to make it 100% compliant because it's simple not worth it. But I would like you to consider the possibility of extending the API.

It would be nice if the library remember the ID of the lasts messages sent successfully (one per topic). So then if the program using your library wants to send again the same payload (because the previous one failed) it can truly mark the package as duplicate (adding a boolean to the prototype of the publish method). That way, if the message is marked as duplicate, the library can really send it as duplicate using the last sent ID.

What do your think of this idea?

Thanks Gonzalo

gonzabrusco commented 1 year ago

Just a follow up. If you think that storing the last message ID sent per topic is too costly. It would be nice to atleast have another onpublish method like this:

bool publish(const String &topic, const String &payload, bool retained, int qos, unsigned int * messageId, bool duplicate)

The function can return true if the publish was successful or false if it failed. But also it can inform the user the message ID used for publishing that message storing it in the pointer messageID passed. Then my program could be sent again the same message passing duplicate to TRUE and the message ID.

What do you think?

gonzabrusco commented 1 year ago

https://github.com/256dpi/lwmqtt/pull/16

I have created a pull request adding this functionality to lwmqtt first.

256dpi commented 1 year ago

I added the experimental methods uint16_t lastPacketID() and void prepareDuplicate(uint16_t packetID) to support this. I was reluctant to add further overloads of the publish() method with a *dup_id parameter (as in lwmqtt) and opted for this simpler design. Most users of this library do not care for this functionality and ones that do, have now a way to do it. Let me know what you think. Here is the documentation from the Readme:


Obtain the last used packet ID and prepare the publication of a duplicate message using the specified packet ID:

uint16_t lastPacketID();
void prepareDuplicate(uint16_t packetID);
gonzabrusco commented 1 year ago

Hi @256dpi the duplicate logic seems to be working fine. Thanks.

One thing I noticed is the following, if I use dropOverflow(true) when a very big message arrives, mqtt disconnects. Is that normal? Shouldn't it just ignore the package without disconnecting?

256dpi commented 1 year ago

Thanks for the feedback! No, the client should not disconnect. On such an error loop() should return false and the error can be obtained using lastError(). What does it say?

gonzabrusco commented 1 year ago

LWMQTT_BUFFER_TOO_SHORT

gonzabrusco commented 1 year ago

Hi @256dpi I think I have a "kind of bug" in the way the duplicate is handled.

If you use the new function prepareDuplicate(duplicateID) it will publish as duplicate OK. But internally, it will not reset the class variable nextDupPacketID after publishing, thus every packet published afterwars will be marked as duplicate with the same ID unless you call prepareDuplicate(0) . Maybe it should be a good idea to reset nextDupPacketID after publish (successful or not)?

256dpi commented 1 year ago

Yes that makes sense. I'll see if I find time to fix that this week.

Decezaris commented 1 year ago

Any news about it?

256dpi commented 6 months ago

I just released a new version with the new methods included. The duplicated ID is now cleared after every publish, successful or not.