Pubsubclient: setServer(const char *domain...) should deepcopy the domain, not the pointer

[TD-er]

See https://github.com/knolleary/pubsubclient/blob/master/src/PubSubClient.cpp#L565
As discussed here: https://github.com/letscontrolit/ESPEasy/pull/629#issuecomment-353794727
The problem is with setting the server in code like this:

{
  String hostname = F("foo.bar");
  MQTTclient.setServer(hostname.c_str(), port);
}

Then the pointer will be copied, but is no longer valid when being used in PubSubClient.

Suggestion for improvement: Keep the member variable domain as String or use new to allocate the data (and free in the destructor) and make a deepcopy when setting the server.

[bratello]

I use the following workaround:
void connect(const String& host, int port) { static char pHost[64] = {0}; strcpy(pHost, host.c_str()); MQTTclient.setServer(pHost, port); }

Static array allows you to preserve the domain ptr. Static array not so good approach, but in fact that Arduino single thread - it will work.

[TD-er]

@bratello Problem is, a static variable is being shared among all instances, so if you run more than one instance, you will run into strange issues.
The use of static variables in C++ is only useful in a limited amount of use cases and storing this is typical for a member variable.

[bratello]

@TD-er Sure, but generally you need only one MQTTClient connection per device, and Arduino device is single thread, so this workaround is pretty safe. Actually the same approach as strtok.

[TD-er]

Well on ESPeasy we may have more instances on the same node running.
So even if you use it on single threaded environments yourself, you may still keep these things in mind, for example if someone tries to run it on an ESP8266 or ESP32.
ESP8266 is still single threaded (as in single core), but still I run a lot more on it than you may expect considering it is a single core.

So please do not use static for variables, unless you really need to share variables among all instances of an object.

[bratello]

@TD-er You have convinced me. I don't like the PubSubClient implementation at all, specially the MQTT_MAX_PACKET_SIZE and non resized buffer, need to re-write it on dynamic buffer or even std::stream. You can ofcourse to put MQTT_MAX_PACKET_SIZE definition before #include line, but then you have unnecessary memory overhead

[TD-er]

Well there are of course other platforms where std:: functions may not be available or just introduce too much overhead.
To me it would already help if the define for the buffer size is wrapped in #ifndef statements, so you can define it outside the library at compile time.

Like:

#ifndef MQTT_MAX_PACKET_SIZE 
  #define MQTT_MAX_PACKET_SIZE  1234
#endif // MQTT_MAX_PACKET_SIZE 

[knolleary]

@TD-er the define of max packet size is already wrapped with ifndef:

https://github.com/knolleary/pubsubclient/blob/d2ff771f4a79950fa36e35c4c71e232da4f85f5d/src/PubSubClient.h#L25

[TD-er]

@knolleary I see.
Sorry, wrong example then and no idea why it is stuck in my head that the static buffer size was a problem with this library.

That's also a problem of myself, that I do almost everything from memory, which is for sure not flawless and also may be out of sync with reality. I notice (using Git Blame) this wrapper is already present for 4 years now, so that could not have been outdated information in my brain.

[bratello]

@knolleary Yes Nik. But most of the time the MQTT packet is very small, in about 100 bytes, except these situations when you need to send much more. So with #define MQTT_MAX_PACKET_SIZE 1024 approach you have the constant 1K memory footprint. That's why I would prefer the dynamic buffer instead. Maybe I miss something?

[TD-er]

@bratello Dynamic buffers are also not ideal.
For example if you use something like a STD container (e.g. vector/string), you cannot just keep a pointer to the element and use ++ to move to the next.
Even iterators may not be valid forever.
If you need to expand a STD container, almost all of them allocate the new size, copy all elements to the new one and then de-allocate the old array.
So if you keep a pointer to a section, you may end up in a location where once the buffer was but no more.
This means you should use indices and constant checking of the current size of the buffer.

Another problem with these is that it may lead to heap fragmentation, especially since these buffers remain allocated for a long time.
For short time use, heap allocation using std containers is not really an issue, but for long term allocated blocks it can be a real issue.
Also increasing a buffer like that may fail if no consecutive block of free memory is available to allocate (also due to fragmentation).

[bratello]

@TD-er

1. std::vector or std::basic_string provides reserve method, which manages the reallocation strategy, so you can adjust the frame size which will be added every time when additional size required.
2. PubSubClient buffer basically for short time use, when you publishing value or receiving the value as subscriber.

[TD-er]

I know that they have a reserve statement, but if you need to rely on that to keep the allocation constant, then you're defeating the purpose of using a dynamic sized container.
The whole idea of using a buffer is that you have a writer and a reader which work independent from each other.
So the reader may still be processing part of the buffer and that's not the one that will resize the buffer if needed.

[bratello]

@TD-er

1. I mean for the initial state buffer.reserve (packetSize); where packetSize can be changed by PubSubClient::setPacketSize(size_t newSize = 128). For the most of the cases it will be enough. But for the long chunk of data it will increase the buffer size as needed, at end of the publish you can reset buffer again.
2. Sure, read and write flows are using the same buffer, by rotation. PubSubClient doesn't seem to work in a multithreaded environment

[TD-er]

I don't think you get my point here.
One of the disadvantages of using dynamic buffers is that it does add complexity to the functions handling the buffer.
So regardless if you add functions to resize the buffer etc. you still need this extra management if you allow the buffer to be resized at all.
This has nothing to do with multi threading. It is just that you need to address parts in the buffer through indices and have to check its current size etc.

I know the buffer is used as a ring buffer, which does add to the complexity if the buffer size can be changed. Distance computation functions also have to take into account the buffer length when trying to determine the amount of free space (or used space).

On top of that, even in non-multithreaded environments we do have call-back functions. These can be considered as interrupts and these can also interact with the data.

I'm not saying it cannot be done, I'm just saying that having a dynamic sized buffer does add a lot of complexity to the code.
It would be best to have then a buffer class, which does manage it all internal and a reader and writer to interact only with this buffer. But I doubt it will save a lot of resources in this use case and it does add a burden on the resources, like CPU cycles.

For the most of the cases it will be enough. But for the long chunk of data it will increase the buffer size as needed, at end of the publish you can reset buffer again.

Nope, it can only be resized again if the data is being sent. That's not the same as the end of the call to publish.
Publish is just entering it into the buffer and then from within the loop() function the buffer will be emptied.
When it is being sent, a new message can already be present in the buffer, so shrinking the buffer should have to wait until.... well you get the point. => Added complexity.

[bratello]

@TD-er
1.std::vector or std::basic_string already does all this management stuff.

1. callback called from loop, you just kicking the PubSubClient to read data from the network and trigger the callback if some topic data available, that's it.
2. At end of the publish method you have write method, which just issues the data to the server: rc = _client->write(buf+(MQTT_MAX_HEADER_SIZE-hlen),length+hlen);

Does not matter, I'll try to implement PubSubClient with dynamica by myself.

[TD-er]

Does not matter, I'll try to implement PubSubClient with dynamica by myself.

Just to be sure you'll know.
I'm not the author or maintainer of this library.

[bratello]

@TD-er Sure, thank you for your notes anyway.

[bratello]

@knolleary Nik I would also add the extended subscription API, to pass topic callback with unpacked arguments, for example:

mqttClient.subscribe("MaxTemperature", [this](const float& val) {
    //Payload already unpacked, do something with float value
});

It can be done +- like that:

template<typename T>
boolean subscribe(const char* topic, std::function<void (const T&)>& callback) {
    //Preserve callback
    this->topicCallbackMap[topic] = make_callback<T>(callback);
    //Notify MQTT server about new topic subscription
    subscribe(topic);
}

typedef     std::function<void (const uint8_t*, unsigned int)>  payload_callback_t;
template<typename T>
payload_callback_t  make_callback(const std::function<void (const T&)>& callback) {
    return [callback] (const uint8_t* payload, unsigned int len) {
        callback(from_payload<T>(payload, len));
    };
}

So you provide the set of the from_payload functions for proper topic arguments unpacking:

template<typename T>
T from_payload(const uint8_t* payload, unsigned int len) {
    return payload_to_string(payload, len);
}

template<>
bool    from_payload<bool>(const uint8_t* payload, unsigned int len);

template<>
float   from_payload<float>(const uint8_t* payload, unsigned int len);

End user can extend it by implementing from_payload for his user defined type:

template<>
MyStructure   from_payload<MyStructure>(const uint8_t* payload, unsigned int len);

All things are done at compile time, compiler will let you know what's wrong.

Same thing you can do for the publish method:

template<typename T>
boolean publish(const char* topic, const T& val) {
    return publish(topic, to_payload(val));
}

I understand that your PubSubClient should be compatible with very limited compilers on different platforms, but you can put this extended API under ifdef block, so the basic functionality will not be affected. But the most of the mistakes are related with data packing and unpacking, which sometimes very hard to identify.

Thanks.

[bratello]

@TD-er Gijs we both a little bit stupid, the PubSubClient provides approach for publishing the large message without busting the bank of america:

mqtt_large_message.ino

Print::print() method calls to virtual Print::write(const uint8_t *buffer, size_t size) which already implemented as PubSubClient::write(const uint8_t *buffer, size_t size), the PubSubClient::buffer not involved into this flow at all. So you can return back the MQTT_MAX_PACKET_SIZE and handle the large message different.

Definitely the lack of documentation generates the misunderstanding.

[TD-er]

I assumed you were worried about the size of the buffer itself taking so much resources.
I know the max message it can send is in the order of the buffer size, but I thought you wanted to free the buffer resources when you'd expect not to send large messages.

[bratello]

@TD-er No... Basically my device publishing in around 60 bytes of data, but at the start I need to publish the device schema JSON document, sort of device self describing, which requires 1.5K and more. As your understand, allocating 2K or more for the buffer it's memory squandering for the device that posting 60 bytes once per minute. But with the beginPublish & print approach it's possible without additional memory space.

[TD-er]

Ah OK, I get it. That's not what I initially got from your reply.

Good to know indeed.