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.