Pubsubclient: 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? https://github.com/knolleary/pubsubclient/pull/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!

[skorokithakis]

This was implemented, as far as I know. Unfortunately there's no linked commit, but the parameter name should be easy to find if you look in the code.

[knolleary]

@skorokithakis no it wasn't. The library still uses the #define to fix maximum message size.

My comment here still stands.

[vicatcu]

There is an in-between option without going to dynamic memory allocation. Give the user the opportunity to allocate a buffer, in whatever manner they chose, in their sketch and pass a pointer to it and its size to the library using am optional "setPacketBuffer" method or something like that. The library could keep its existing compile-time allocation behavior, and _if_ a user packet buffer is provided at run-time, through the setPacketBufffer method, it can use that instead of the the compile-time allocated buffer. That's a better compromise for embedded, in my humble opinion, than adopting a dynamic memory allocation approach.

[vicatcu]

I've just implemented what I just described on a branch of my fork here: https://github.com/vicatcu/pubsubclient/tree/allow-external-buffer-allocation. Pretty straightforward, but untested as of yet. What do you think @knolleary et al?

What I intended to implement was:

• rename the internal buffer variable to _buffer
• rename the internal buffer size to _MQTT_MAX_PACKET_SIZE
• make new internal variables buffer and MQTT_MAX_PACKET_SIZE
• make new method setPacketBuffer that takes a pointer to a buffer and a length (with default _MQTT_MAX_PACKET_SIZE)
• in every constructor, call setPacketBuffer to point at the internal _buffer

By construction, this code should function identically to the existing code, as I've just underscored the old variables/constants and invented 'new' variables with the old names without underscores (used throughout the code) that initialize to the old values, but that can be changed at runtime by calling setPacketBuffer on the object with appropriate arguments from the user program (e.g. in setup).

I was nervous that the existing code was using sizeof(buffer) but luckily, I see no evidence of that so it shouldn't be a problem for this proposal.

[marcoschwartz]

@vicatcu @knolleary Any chances of pushing this feature into the master branch soon? Would be awesome :)

[knolleary]

I'm have very little spare time to give this the attention it needs at the moment. Not the sort of change I can merge in without going through in detail.

[vicatcu]

@knolleary I agree, it deserves adequate attention and shouldn't be merged hastily, but I've submitted the pull request for when you get around to it. @marcoschwartz could you help by writing some tests and trying out the PR ahead of it being merged?

[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 https://github.com/knolleary/pubsubclient/pull/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 ;)