-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Refactor to allow specifying keepAlive, socketTimeout and buffersize #389
base: master
Are you sure you want to change the base?
Conversation
Well I would avoid dynamic memory allocations as much as possible on embedded systems because of memory fragmentation issues. Other than that this library offers the possibility to modify the buffer size and other mqtt related stuff also. In the PubSubClient.h file we have this part: So the user has two options here:
Now I'm not against your approach - don't get me wrong here - this is just another way of doing it. And it's documented here: API Documentation - Configuration |
Method 1 does not work in Arduino and that is the main reason for refactoring: Given that you can specify size in the additional constructors defined, correct size will be allocated right away and if used in a static instance no fragmentation will ever occur. |
@kotl Yes you're very right about that. Again I'm not contesting your PR it's just that there are other libraries around that are using dynamic allocations as you do and I'm looking at the overall picture. Let's say that I'm using your PR along with this library and I'm using Adafruit Neopixel library too and some others that are doing dynamic allocations. But for example there are some libraries that are doing both dynamic memory allocation and de-allocation at runtime so what happens then? Doesn't that lead to memory fragmentation in time? So again I'm looking here at the big picture and at the cumulative effect. Maybe I'm wrong and I don't really understand how dynamic memory allocation works and what fragmentation is. Thanks and keep up the good work. |
Yes, I looked #282 initially, but did not like the fact that it required to be reallocated and duplication of code in constructors and went on a journey to improve that. Now there is no code duplication in constructors and additional constructors that take parameter to specify initial size which avoids realloc. |
Thanks for suggesting this. I've since merged #282 with some tweaks. And I've pushed a separate change of my own to allow the keepalive to be set. Will follow up with the other constants. I am interested in the constructor changes you've made. I do very little with C++, so would like to dig into that part of it a bit. So whilst I may not merge this PR as-is, I certainly appreciate the work you put into making these proposals. |
Allow specifying keepAlive, socketTimeout and buffersize in constructor and through member methods.
Simplify constructors through 2-level nested calling of constructors in the same class. (g++ 4.8 will be required, and it does work with latest Arduino compiler, for example ARDUINO 1.8.5). It comes down to one main constructor and 3-second level constructors that call setServer differently. 100% compatible with constructors in previous version.
Why all this? Well, when you need bigger buffer set, you CAN NOT define size in your .ino file because of Arduino compiler limitations, it will not be picked up by the library. The only way to do this before was to modify local copy of the library, which, if you do forget to do on another computer, results in hard to debug problems. Allowing this specifying through the library eliminates need to modify library locally ever. So the right thing to do is allow dynamic buffer to be used and realloc it.