Question about lifetime management of topic / payload

[technyon]

Hi,

From what I can see in the code, if I call any publish method, the topic and payload are stored in the outbox as a "Packet". As far as I can see, both are stored as pointers, so the actual content isn't copied.

That would mean I have to make sure the data (topic / payload) are kept in a valid memory location until they are sent to the broker, which would be problematic to do.

For example:

string path = "...";
string payload = "...";
publish(path.c_str(), MQTT_QOS_LEVEL, true, payload.c_str());

This has a high chance to cause problems, because the pointer that c_str() returns isn't valid anymore after publish() is finished. By the time the library loops and sends the messages from the outbox the memory may or may not be overwritten by something else.

Is this correct, or am I misreading something in the code? It would make using the library way harder, because I have to take care the memory is valid ... which is hard because I don't know when the library loops and sends the data.

[bertmelis]

A pointer is stored in the queue (outbox) but that pointer points to a packet created in heap memory:
https://github.com/bertmelis/espMqttClient/blob/main/src/Packets/Packet.cpp#L310

this is done in the various constructors of the packet class.

[technyon]

OK thanks your your reply.

I have a few more questions :).

I'm testing your library for my project:

https://github.com/technyon/nuki_hub

All works quite well so far, where all other libraries were broken so far. However, my firmware can work over Wifi and Ethernet (W5500), so I had to make some modifications so that I can inject an ethernet client.

Also I'm storing a pointer to the mqtt client instance, which in your case could be espMqttClientSecure, espMqttClient, or espMqttClientEthernet which I added. The pointer points to an instance of MqttClientSetup, which is a template class. I've just removed the template part without any obvious problems to be able to create that pointer. Why is it necessary to be a template class. Every method in there returns "this", but the return value isn't really used anywhere (as far as I can see). Would it be an option to just remove the template part?

[bertmelis]

It was my choice to make this library's interface as close as possible to async-mqtt-client. That lib also returns a reference to itself in the "setup-methods". It's done to enable https://en.m.wikipedia.org/wiki/Method_chaining It is not really needed but at this point it is obviously a breaking change.

Now I'm curious why you can't just create the pointer to the class. What exactly is the error message you get?

I hope adding ethernet wasn't that difficult. I tried to design the lib in a way it would be rather straightforward adding a new type of transport.

[technyon]

Adding ethernet was actually quite simple, I've just coped espMqttClient and use EthernetClient instead of WifiClient. I've changed it though so I can inject it, because I'm using it for other things like a web interface (also storing it as a pointer). Maybe having a second constructor to inject it would make sense.

My problem is this: I have a base class that returns the mqtt client instance as a pointer:

virtual MqttClientSetup* mqttClient() = 0;

There are two other classes that abstract the handling of the network device (WifiDevice and W5500Device). They return either an instance of espMqttClient or espMqttClientEthernet. If it's a template class, putting MqttClientSetup* into the base class doesn't work because the compiler doesn't know the actual type.

See here:

https://github.com/technyon/nuki_hub/tree/espmqttlib/networkDevices

[bertmelis]

I'm on phone now. I'll have a look later today.

[bertmelis]

I'll explain a bit how this library is designed:

MqttClient class: base class with MQTT related code. The business logic so to say. I tried to make the parts that are needed to be able to create derived classes are marked protected instead of private. MqttClient relies on a pointer to a (base) class "Transport".

Transport class: pure virtual base class that defines the interface, the methods, needed for MqttClient. The actual transport classes inherit from this base.

MqttClientSetup class: Template class that defines the API of espMqttClient. This contains all the "setters" for protected variables of the MqttClient class.

espMqttClient class, espMqttClientSecure class, espMqttClientAsync class...: These are all specialized version of the MqttClientSetup class. Using CRTP, they implement the template functions defined in MqttClientSetup.

The steps to add a new transport type, as it was in my head:

• create a new class "YourTransportType" and (publicly) inherit from Transport.
• then you have a few options:
  • just like the espMqttClient and variants, specialize from MqttClientSetup and use it like in the examples
  • directly use MqttClient and set the protected member variable _transport to the instance of YourTransportType you created above
• In the second case, you still have to create your own interface class to access the member variables of MqttClient

[technyon]

That's pretty much what I did: Implement my own Transport, implement an MqttClientSetup variant and use that transport.

The problem comes with the interface part. Instead of having a getter for my MqttClientSetup, I could create proxy methods. That would also be cleaner, just a bit of tedious work ... I counted 8 methods so far. I'll probably go that path.

[technyon]

Got it working without making changes to the library. Thanks for writing this ... took me a while to find this, but it works without problems. Tried PubSubClient, Arduino MQTT and they all had their issues.

[bertmelis]

Good to hear. Let me know if you need any more support.

[alexdelprete]

@bertmelis thanks a lot for your support. Nuki Hub users will be grateful to you. :)

[bertmelis]

Looks amazing. I'm a big fan of these kind of these "hacks" because they are non-intrusive.

[alexdelprete]

Looks amazing. I'm a big fan of these kind of these "hacks" because they are non-intrusive.

actually, it's a replacement for the shitty product made by the vendor (Nuki Bridge) that costs $99 and sucks (only 1 http connection, and doesn't support MQTT, only REST). That's where open-source projects lik Nuki Hub shine: with a $10 ESP32 and open-source software, you can make a much better product than that.

Obviously thanks to the work of people like @technyon and you. :)

[technyon]

The new release is out which uses your library:

https://github.com/technyon/nuki_hub/releases/tag/8.0