philbowles
commented
4 years ago - Firstly, thanks for your thoughts. The fundamental issue is that any dynamic data sent to LwIP canonly be released when the underlying TCP stack ACKs the data as sent, which is asynchronous and may be seconds later. Thus all data sent would only ever have a reference count of 1. Further, since the whole library is asynchronous, there can be many messages "in-flight" all awaiting ACK in the correct order.
Secondly each "send" or "rcv" is potentially only a fragment of a message when using large packets thus the smaller components cannot be released until the last ACK is received (or all data sent)
Thirdly for QoS purpose, the reassembled messages must be retained until the server acknowledges them at the MQTT protocol level (as distinct from TCP ACK at the packet level).
Any memory management system is only "robust" if it works. Smart pointers used wrongly/badly do not guarantee robustness. Pangolin's memory management does the exact equivalent of smart pointers without the overhead and it works - it was tested to death and there have been no reports of leaks - so there is no reason to change it. Anyone who wants to rewrite the internal memory handling using smart pointers is welcome to do so - but they will end up re-writing 75% of the library. They will find it horribly complex and the code will be larger and slower.
Other libraries that do not adopt a similar strategy are littered with fatal bugs, regulalry crash at random, intervals and simply do not work at above QoS0. I hope this explains Pangolin's "unorthodox" (but 100% functional) strategy.
- All small MCUs suffer from memory fragmentation. Yes, some strategies may be worse than others, but given the lower level libraries available, some fragmentation is inevitable.
The whole point of Pangolin is to avoid the problems that other libraries have with buffers (see the bugs documentation). It is also designed to be portable across different MCUs (e.g. STM32) and different implementations of LwIP (and/or user build options) which arbitrarily change not only the size but the number of available TCP buffers. A fundamental feature of Pangolin is the correct fragmentation / reassembly of huge packets for which fixing any buffer size will result in sub-optimal code in the majority of use cases.
As described above the buffering startegy is already extremely complex - adding variable-sized buffers (which depend on the underlying LwIP implementation over which Pangolin has no control) would render it nigh-on unmanageable.
I appreciate these explanations may not be what you wanted to hear, but - as they say - "it is what it is".
Suggestion 1 - Buffer lifetime
There's a comment:
I am 'uninformed' in this case, since I've not studied your code in detail, but I believe I understand your motives. In this sort of situation, it's common (usual?) to use reference-counted smart pointers to manage buffers like this. Not only is this a robust solution, it's well understood, so would not warrant as many dire warnings of things looking like memory leaks.
As I say, I've not delved into the code, so perhaps there is some detail that would prevent you taking this approach, but it 'smells' plausible to me.
Suggestion 2 - Fragmentation problems
I've seen various comments in other bugs that reference fragmentation from your allocation strategies. A simple mitigation (not solution) to this is to allocate buffers of common sizes, rather than ideal sizes. For example, 674, 856 or 1020 bytes would each use a 1024 byte buffer, 1456 bytes might use 2048 and so on. Alternatively, everything might use a 2048 byte buffer, for example. The point is, that although it's slightly wasteful up-front, it's more robust longer-term.
Clearly, the machine might be doing something similar in the background anyway, but from the comments (and because the Arduino is pretty light-weight), I suspect it is not.
Similarly, you could allocate a few persistent buffers up front (configurable?) which you always use if you can, resorting to new heap allocations only if necessary.