Support publishing and receiving large messages. #193
Conversation
…te packets (ref PR#113) for larger messages. Correctly identify topic start position for >127byte messages (ref PR#166). Resolves issue adafruit#102
| @@ -233,7 +233,7 @@ uint16_t Adafruit_MQTT::readFullPacket(uint8_t *buffer, uint16_t maxsize, | |||
| // will read a packet and Do The Right Thing with length | |||
| uint8_t *pbuff = buffer; | |||
|
|
|||
| uint8_t rlen; | |||
| uint16_t rlen; | |||
There was a problem hiding this comment.
Packets larger than 255 bytes would overflow rlen.
There was a problem hiding this comment.
This does solve an issue I was having today with a very large binary payload, thank you.
Correctly identify topic start position for >127byte messages (may also be addressed in PR166 -- happy to merge that fix in if preferred).
@flavio-fernandes Is this implemented in #166?
There was a problem hiding this comment.
hi @brentru . No, this fix is related but not exactly what I was trying to address in #166 . The #166 was done to avoid corrupting memory should we attempt to
publish more data than what is available by MAXBUFFERSIZE. I think the function introduced in this PR (topicOffsetFromLenth) is what #166 introduces, so we definitely should combine that. @xdylanm how about you incorporate that into this pr? You can take the credit; I do not care for that. ;) Basically, we need this logic in addition to what you are doing here: d6f07ed#diff-a20445c19076bfb61fa2cf57b03325851600fc2dc25391f7e97f2de072fda081R701
There was a problem hiding this comment.
@flavio-fernandes in my own build, I originally had a simpler version of the length test
uint16_t const topicoffset = (len > 127 ? 1 : 0)
but I liked your function approach better, so I followed that pattern and credit to you =). If #166 is close to being merged, I'm happy to just rebase off of that when it's merged and update my PR. Per your note, you're trying to solve a different problem, which should certainly get resolved.
That said, I realized that this solution does not support packets > 65535 bytes since the lengths are all stored in uint16_t, and I'm still not sure what the right solution is there.
There was a problem hiding this comment.
Understood. Yes, you correctly point out the fact that we cannot go beyond 65535 since we are using uint16.
I will defer to @brentru on how we want to proceed here, but my opinion is to go ahead with PR/166 as is,
and then have you leverage the function packetAdditionalLen in PR/193 and potentially improve on the uint16 limitation. Sounds reasonable, @brentru ?
There was a problem hiding this comment.
This sounds OK and I agree. - @xdylanm, see the comment below about using clang-format so we can approve this PR.
|
@xdylanm Could you please format this library using clang-format and then push a commit with your formatted code? We have instructions on how to do this here: https://learn.adafruit.com/the-well-automated-arduino-library/formatting-with-clang-format Thank you for your PR, I'll be reviewing it after it's passing CI. |
|
After formatting, please bump the library to version |
…brary into send_rcv_long_packets Pull in changes from flavio-fernandes
…g-format, version ticked to 2.3.0, replaced duplicate topicOffsetFromLength with packetAdditionalLen.
| // its variable length field. As per | ||
| // http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Table_2.4_Size | ||
| // See also readFullPacket | ||
| static uint16_t packetAdditionalLen(uint32_t currLen) { |
There was a problem hiding this comment.
I changed the signature here to explicitly take a uint32_t so that the comparisons are consistent. This doesn't change the fact that the max packet length is limited to 65536, but that is checked in readFullPacket
| @@ -267,7 +283,8 @@ uint16_t Adafruit_MQTT::readFullPacket(uint8_t *buffer, uint16_t maxsize, | |||
| DEBUG_PRINT(F("Packet Length:\t")); | |||
| DEBUG_PRINTLN(value); | |||
|
|
|||
| if (value > (maxsize - (pbuff - buffer) - 1)) { | |||
| // maxsize is limited to 65536 by 16-bit unsigned | |||
| if (value > uint32_t(maxsize - (pbuff - buffer) - 1)) { | |||
There was a problem hiding this comment.
value is uint32_t so it can handle the actual packet length. I made the cast explicit here in the conditional, but the key thing is that it's checked and for packets larger than 65536 (maxsize is uint16_t), you can't read past that size, and there is a debug message there.
|
@brentru - merged from master after #166, addressed clang-format comment, and ticked version |
|
Tested on hardware. I'll merge and release once the CI gives me the ✅ |
Send multiple 250 byte packets for a large message (this was proposed in PR113, but the change here should work for existing implementations that only support 250 byte packets by breaking the message into chunks).
Correctly identify topic start position for >127byte messages (may also be addressed in PR166 -- happy to merge that fix in if preferred).
Resolves issue 102.
Tested on Wemos D1 mini (ESP8266) with messages up to 1023 bytes (set MAXBUFFERSIZE and SUBSCRIPTIONDATALEN to 1024).