Function to change the MQTT packet size outside of the library

[marcoschwartz]

Hello, great library! For several of my projects I'd need to change the MQTT packet size, but I need to do it from outside of your library.

Indeed, now I'd need to change MQTT_MAX_PACKET_SIZE inside your library, which is not very convenient. A function or a way to do that without changing your library would be great! Thanks.

[skorokithakis]

+1, I'm using PlatformIO which keeps a common cache for all libraries and this is presenting a big problem for me. An easier way to configure this would be great.

[lunanigra]

I need this too... 128 seems far too low.

We are using DeviceHub. Based on the long API key and Devic ID length you easily exceed 128.
So, perhaps set MQTT_MAX_PACKET_SIZE to 256 or better make it configurable.

Thanks :-)

[knolleary]

So the reason it's fixed is because we preallocate the memory at compile time. Given the limited memory of the original devices this targeted, I felt it was import to do this so potential memory issues could be caught then, and not at runtime (which are so much harder to debug). I also didn't want to set a default value that meant a lot of memory was pegged but went unused if you were only sending/receiving simple messages.

Any default value will have pros and cons.

Perhaps the answer is to move to fully dynamic memory allocation, but isn't a straight forward choice.

[skorokithakis]

Could it be made optional with an ifdef so it can be defined at the preprocessor level perhaps, at least?

[knolleary]

I never found a way to allow an arduino sketch to ifdef its own value to override the library - all to do with how the Arduino tool chain preprocesses the app's files.

[skorokithakis]

I use PlatformIO, which can do that (or, if not, I'm sure @ivankravets will approve a PR from me for it).

[ivankravets]

@knolleary what is problem with #ifdef https://github.com/knolleary/pubsubclient/blob/master/src/PubSubClient.h#L23?

#ifndef MQTT_MAX_PACKET_SIZE
#define MQTT_MAX_PACKET_SIZE 128
#endif

PlatformIO users have full control of build process and can pass own build_flags. For example, platformio.ini

[env:myboard]
platform = espressif
framework = arduino
board = nodemcu
build_flags = -DMQTT_MAX_PACKET_SIZE=256

[knolleary]

@ivankravets because the last two times I've tried that approach it hasn't worked. The arduino tool chain includes library headers before it includes your sketch. So you cannot #define something in a sketch and have it picked up in the library.

[skorokithakis]

The above would be ideal, and preserve backwards-compatibility.

[skorokithakis]

@knolleary The build flags are included in the compiler command line, so they would take precedence. They aren't defined in the sketch.

[knolleary]

Ahh - sorry, read the rest of your comment.... so in the PlatformIO environment I can see how that would help. Doesn't address it for everyone else... but would be useful applied to all the customizable #defines.

[skorokithakis]

@knolleary I think that's a good compromise, since it doesn't hurt Arduino IDE users but benefits PlatformIO users.

[knolleary]

I would certainly accept a PR that added that - as long as it applies to the other #defines and not just the packet size one.

[ivankravets]

@knolleary Please don't mix Arduino IDE and it's builder with PlatformIO. PlatformIO uses only source code of Arduino framework core for AVR/SAM.

[ivankravets]

I would certainly accept a PR that added that - as long as it applies to the other #defines and not just the packet size one.

Should I make it?

[skorokithakis]

@ivankravets Too late! #119.

[knolleary]

Thanks @skorokithakis - will get it merged later today.

[ivankravets]

@skorokithakis I'm not sure that need to redefine MQTT_VERSION. Let's wait for @knolleary.

[skorokithakis]

@ivankravets That's the supported version for the client, the actual versions are defined a few lines above that.

[ivankravets]

@knolleary thanks you too. Could you merge my PR with library.json? #44

[skorokithakis]

+1 on that as well, so we also get this PR autoupdated in PlatformIO.

[skorokithakis]

@knolleary Any news on this?

[knolleary]

Sorry, been flat out with work and have been at a conference yesterday/today. Will try to get to it all this weekend.

[skorokithakis]

Sounds great, thank you!

[SirUli]

I think that one is solved - at least i can see it in the code :)

[knolleary]

It's solved for platorms that allow you to provide #defines outside of the sketch. That doesn't solve it for the Arduino environment. Still need to decide if we want to provide a programmatic wy for a sketch to set the packet size.

[SirUli]

@knolleary thanks - you are right, i just looked through my platformio glasses ;) Sorry!

[marcoschwartz]

I would indeed go for the programmatic way to define the packet size - would be really useful for other libraries that uses this one :)

[marcoschwartz]

Hello, any news on this issue? We'd really need it for our aREST library. Thanks!

[jonnor]

Use a template parameter for the buffer size, and then provide a default instantiation that uses the pre-existing MQTT_MAX_PACKET_SIZE ?

[jonnor]

I could not find a way which wasn't completely ugly using template parameter. Issue is that each and every method needs to become a template on the buffer size, which is a lot of distraction and complication. And one needs to the templated class, and then typedef an explicit instantiation for compatibility with existing code... Probably not worth it for saving 128 bytes in a cornercase.

So I reviewed #110 a bit instead

[mrdunk]

hi,
i'd like to add my interest in this feature as well.
I fully understand why the initial decision was made to statically allocate this buffer but it is very limiting for esp8266 users who have a reasonable amount of memory to play with. There are certainly lots of people online running into this issue.

I'd rather not have to include instructions in my code (which uses this library) on how to manually download and modify MQTT_MAX_PACKET_SIZE.
There's a nice opinion of why using a #define in a library is the wrong tool for a user changeable option here: https://stackoverflow.com/questions/14418936/overriding-define-in-libraries/14429506#14429506

So, solutions:

Option 1:
@vicatcu has a proposed change that i could be persuaded to support.
I agree with the majority of the comments in @jonnor 's review comments.
@vicatcu have you looked at @jonnor 's suggestions?

Option 2:
new in the constructor:

class PubSubClient {
  ......
 public:
  PubSubClient(Client& client) : buffer(new uint8_t[MQTT_MAX_PACKET_SIZE]), buffer_size(MQTT_MAX_PACKET_SIZE) ;
  PubSubClient(Client& client, uint16_t _buffer_size) : buffer(new uint8_t[buffer_size]), buffer_size(_buffer_size);
  ......
 private:
  uint8_t* buffer;
  uint16_t buffer_size;
  .....
}

Option 3:
A dynamically allocated solution would be trivial to implement:

class PubSubClient {
  ......
 public:
  PubSubClient(Client& client) : buffer(malloc(MQTT_MAX_PACKET_SIZE)), buffer_size(MQTT_MAX_PACKET_SIZE) ;
  ......
  bool setBuffer(uint16_t size){
    buffer = realloc(buffer, size);
    buffer_size = size;
    return (bool)buffer;
  }
  .....
 private:
  uint8_t* buffer;
  uint16_t buffer_size;
  .....
}

If it were up to me, i'd probably go with Option 3.
For both 2 and 3 very little code would need to change.
buffer would still be accessed and indexed in exactly the same way.
I don't really see the concerns about added complexity when debugging... Sure, buffer is on the Heap rather than the Stack but it's only allocated once by the constructor at startup (and once if the user elects to change the size).

Let me know your thoughts. I am prepared to work on any of these options if they are likely to be committed.
thanks for reading,
dunk.

[mrdunk]

Anyone have thoughts on pr #282 ?
It implements "Option 3" above in a way that does not change the existing API of this module; all existing code will continue to work as before.

[skorokithakis]

I'm afraid I'm not knowledgeable enough to opine, but what you've done seems very reasonable to me, and without any downsides (it works the same as before unless the user elects to resize, no?).

[mrdunk]

Thanks for the feedback.

it works the same as before unless the user elects to resize, no?

Yes exactly. Existing users will not need to change code in any way.
Anyone who needs a larger buffer can re-size after initialization.

@knolleary has commented on pr #282 .
It probably makes sense to move further discussion there.

[Abdul-Samad-Saleem]

@knolleary Any update to modifying the maximum packet size in Arduino IDE?. Are we anywhere near to closing this issue?.

[Huibean]

is it possible to confige buffer using psram?

[jpswade]

I resolved this by switching to another library:

#include <MQTT.h>                 //https://github.com/256dpi/arduino-mqtt
MQTTClient mqtt(256);

[BingXiong1995]

This is because the defaul maxium length is 128b. So just go to PubSubClient.h and change #define MQTT_MAX_PACKET_SIZE 128 to #define MQTT_MAX_PACKET_SIZE 1024

[mcer12]

Since this issue is dead for so long, I wonder if the library is even still being maintained. MQTT_MAX_PACKET_SIZE should be an optional parameter to init the library with. I personally got around this in my project by adding a compiler error so that users at least know what's wrong and how to fix it:

#if MQTT_MAX_PACKET_SIZE < 512  // If the max message size is too small, throw an error at compile time. See PubSubClient.cpp line 359
#error "MQTT_MAX_PACKET_SIZE is too small in libraries/PubSubClient/src/PubSubClient.h at line 359, increase it to 512"
#endif

[knolleary]

Hi @mcer12

it is maintained... but that doesn't mean I add every single feature users ask for, particularly ones that have been discussed many times before.

[mcer12]

Seeing how many people struggle with this, one would think this should be a priority. It's absolutely your call what to prioritize in your own library, I am not refuting that.

[kartben]

I believe this is fixed. There is now a PubSubClient#setBufferSize(size) API.

[knolleary]

Yup - all fixed. Just not done a trawl of the 300+ issues to spot which ones that feature closes ;)