Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic packet size #282

Merged
merged 3 commits into from
May 19, 2020
Merged

Dynamic packet size #282

merged 3 commits into from
May 19, 2020

Conversation

mrdunk
Copy link
Contributor

@mrdunk mrdunk commented Apr 30, 2017

Allow dynamic allocation of buffer size so rebuilding library is not required if user needs greater than MQTT_MAX_PACKET_SIZE (128) byte packet size.
See discussion at #110

@knolleary
Copy link
Owner

Hi, thanks for this. It is, on face value, a reasonable change. It's just needs me to have sufficient spare time to review in detail and go through the process of merging and releasing etc. Can't give you an outlook for that right now.

@mrdunk
Copy link
Contributor Author

mrdunk commented May 4, 2017 via email

@hessijames79
Copy link

+1, also interested in dynamic packet size

@jonnor
Copy link

jonnor commented Jun 27, 2017

Any news on this? Right now when using this library have to tell everyone to download a forked copy or hack the existing define in their library folder...

@knolleary
Copy link
Owner

No news - will try to get to it in the next week.

@Aat0304
Copy link

Aat0304 commented Jul 16, 2017

+1, more than interested in dynamic packet size. Thx

@MarkoHoe
Copy link

+1

@marcostrullato
Copy link

Interested as well.

Thanks

@gilles906
Copy link

+1 Interested as well.

Thanks

@svanscho
Copy link

yep +1 here, seems like some of my messages get lost if they are bigger than 128 bytes

@coogle
Copy link

coogle commented Sep 21, 2017

+1 Please merge this! It's a completely unnecessary burden to ask people to adjust a compile-time setting to build a library when it's got a dependency on this library. Looking at the code it seems completely backward compatible.

@mew1033
Copy link

mew1033 commented Oct 24, 2017

Definitely interested in this. Thanks for the PR!

@gerhardgoosen
Copy link

do i have to pull this code manually into my cloned repo.. not seeing this in the master branch

Copy link

@Titantompa Titantompa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was done in a hurry so there may be other comments.

@@ -12,93 +12,125 @@ PubSubClient::PubSubClient() {
this->_client = NULL;
this->stream = NULL;
setCallback(NULL);
this->buffer_size = MQTT_MAX_PACKET_SIZE;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woudn't it be better to move the buffer allocation to a support function, rather than duplicate the same code in all constructors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree in principal but
i was trying to match the existing code style. (ie, explicitly calling everything in each constructor.)

@knolleary any opinion?

}

PubSubClient::~PubSubClient() {
free(this->buffer);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should check if this->buffer is NULL or not since setBufferSize() can set it to NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. according to the spec std::free() should treat a null pointer as a no-op.
from http://www.open-std.org/JTC1/SC22/wg14/www/docs/n1124.pdf :

The free function causes the space pointed to by ptr to be deallocated, that is, made available for further allocation. If ptr is a null pointer, no action occurs.

(it makes sense because std::malloc returns null pointer on failure.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sweet, I haven't really coded C/C++ in ages and back then it was a no-no since freeing a null pointer would cause some environments to go belly up. :-)

@@ -586,3 +618,13 @@ PubSubClient& PubSubClient::setStream(Stream& stream){
int PubSubClient::state() {
return this->_state;
}

boolean PubSubClient::setBufferSize(uint16_t size) {
this->buffer = (uint8_t*)realloc(this->buffer, size);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use a temp variable. If realloc fails it will return a NULL pointer without freeing the original memory. Thus, if NULL is returned this method should not make any change to this->buffer since it is still valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. this makes sense.

@knolleary is this pull request likely to ever be merged? is it worth me doing this?

@@ -242,6 +279,7 @@ int main()
test_receive_stream();
test_receive_max_sized_message();
test_receive_oversized_message();
test_resize_buffer();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the merge request contains functionality to dynamically change the buffer size it would be suitable to have tests that verify that this works well. E g:

  • What happens if setBufferSize() fails to allocate the memory?
  • If the buffer size is shrunk will it fail (appropriately) to receive a message that was within the prior limits?
  • And the reverse, if it fails to receive a message due to size will increasing the buffer size accordingly help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, all make sense. Let's see if this is likely to be merged first.

@coogle
Copy link

coogle commented Apr 8, 2018

I'm curious if this is going to be merged? My project (https://github.com/ThisSmartHouse/CoogleIOT) relies on PubSubClient for it's MQTT and the lengthy topics the framework uses causes problems (specifically with my heartbeats of devices). Obviously we can side-step this issue with a compile-time flag but what can we do to actually get this dynamic allocation code into the library proper?

@janvda
Copy link

janvda commented Sep 10, 2018

+1

@sskorol
Copy link

sskorol commented May 6, 2019

heavy plus for this feature (as well as for other similar configuration stuff)!..

forcing people to direct sources' modification means additional maintenance effort delegation to client code... so instead of just clicking "install" in a library manager to retrieve a new version, people have to keep an up to date overridden sources in their own libraries or frameworks... that might be quite disappointing... don't you think so?..

but anyway thanks for the great lib!..

@TheCoolBrit
Copy link

TheCoolBrit commented May 19, 2020

Any news on this?
I am not very up to date on programming so using Arduino IDE with a TTGO T-Display to display 240x135 pictures via MQTT using PubSubClient via WiFi
Getting even this far has been a long winding slog for me so Please
Can anyone show me how to change the PubSubClient to allow #define MQTT_MAX_PACKET_SIZE 64800
Thank you in anticipation.

@coogle
Copy link

coogle commented May 19, 2020

At this point I don't understand why it hasn't been merged, other than the owner of the project just doesn't want to do it 👎

@knolleary
Copy link
Owner

I agree this is a long overdue feature.

The challenge has always been finding the time and energy to do the work needed to accept a change like this - especially as it's a feature I have little personal need for. I've also been fairly preoccupied with one of my other OSS projects...

That said, I have recently started dusting off my hardware so I can test some of the proposed enhancements to the library. This one is high on the list... in fact, I may well have a play with it this evening...

@TheCoolBrit
Copy link

What do you make to this possible alternative >
https://github.com/marvinroger/async-mqtt-client

Appreciate any comments

@knolleary knolleary merged commit 64e9811 into knolleary:master May 19, 2020
@knolleary
Copy link
Owner

Well, it took me 3 years, but we got there eventually. Thanks for your patience @mrdunk.

@Aat0304
Copy link

Aat0304 commented May 20, 2020 via email

1technophile pushed a commit to 1technophile/OpenMQTTGateway that referenced this pull request Jun 18, 2020
* Automatically set mqtt buffer size

Current versions of pubsubclient allows the buffer size to be set
dynamically: knolleary/pubsubclient#282

We can take advantage of this to avoid having to modify one of the
dependencies and instead automatically up the buffer size when a
powerful board is detected.
@mrdunk
Copy link
Contributor Author

mrdunk commented Jul 31, 2020

oh cool, just saw this made it in.
thanks knolleary!
i know it's hard to spend time maintaining stuff you don't have much use for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.