Fix buffering of user payloads

[TMRh20]

Per #242 fix buffer overrun

[2bndy5]

Just to be sure, did you see this debug message before the changes here?

RF24Network/RF24Network.cpp

Line 537 in fd8126a

IF_RF24NETWORK_DEBUG(printf_P(PSTR("NET **Drop Payload** Buffer Full")));

[TMRh20]

Yup, got that msg. it was filling up the buffer because of the added delay and overwriting the buffer for sure.

[2bndy5]

Looking at these lines, I feel like the header (8 bytes) and message size (2 bytes) are not accounted for in the conditional statement. Given that,

RF24Network/RF24Network.h

Line 949 in b9095a7

uint8_t frame_queue[MAIN_BUFFER_SIZE]; /** Space for a small set of frames that need to be delivered to the app layer */

Where

RF24Network/RF24Network_config.h

Line 78 in fd8126a

#define MAIN_BUFFER_SIZE (MAX_PAYLOAD_SIZE + FRAME_HEADER_SIZE)

I think that MAIN_BUFFER_SIZE is the right choice for the condition's sentinel.

To prevent overflow, I'd 10 + message_size and use MAIN_BUFFER_SIZE:

-    if (message_size + (next_frame - frame_queue) <= MAIN_BUFFER_SIZE) {
+    if (message_size + 10 + (next_frame - frame_queue) <= MAIN_BUFFER_SIZE) {

I haven't tested this though.

Its important we calculate "if all the data would fit in frame_queue", not just the message's size, because of how we advance next_frame.

RF24Network/RF24Network.cpp

Lines 525 to 530 in b9095a7

	next_frame += (message_size + 10);
	#if !defined(ARDUINO_ARCH_AVR)
	if (uint8_t padding = (message_size + 10) % 4) {
	next_frame += 4 - padding;
	}
	#endif

Hmm, the conditional padding may have to be calculated before entering the conditional block, so it can be ifdef'd into the condition.

So, here's what I came up with in review:

diff --git a/RF24Network.cpp b/RF24Network.cpp
index c468ef4..12c9498 100644
--- a/RF24Network.cpp
+++ b/RF24Network.cpp
@@ -515,7 +515,17 @@ uint8_t ESBNetwork<radio_t>::enqueue(RF24NetworkHeader* header)
     return 0;
 }
     #else // !defined(DISABLE_USER_PAYLOADS)
-    if (message_size + (next_frame - frame_queue) <= MAX_PAYLOAD_SIZE) {
+
+        #if !defined(ARDUINO_ARCH_AVR)
+    uint8_t padding = (message_size + 10) % 4;
+    padding = padding ? 4 - padding : 0;
+    if (padding +
+        #else
+    if (
+        #endif
+            message_size + 10 + (next_frame - frame_queue)
+        <= MAIN_BUFFER_SIZE)
+    {
         memcpy(next_frame, &frame_buffer, 8);
         memcpy(next_frame + 8, &message_size, 2);
         memcpy(next_frame + 10, frame_buffer + 8, message_size);
@@ -524,9 +534,7 @@ uint8_t ESBNetwork<radio_t>::enqueue(RF24NetworkHeader* header)

         next_frame += (message_size + 10);
         #if !defined(ARDUINO_ARCH_AVR)
-        if (uint8_t padding = (message_size + 10) % 4) {
-            next_frame += 4 - padding;
-        }
+        next_frame += padding;
         #endif
         //IF_RF24NETWORK_DEBUG_FRAGMENTATION( Serial.print("Enq "); Serial.println(next_frame-frame_queue); );//printf_P(PSTR("enq %d\n"),next_frame-frame_queue); );

[TMRh20]

Hmm, I didn't really analyze this much, just threw in what I though was the fix. I see you have thought things through a bit more. I will have to take some time to understand your changes and make sure we have it right this time, but in the mean time I will test it out.

[2bndy5]

Its similar to what you propose. Instead of subtracting from one side of the <=, I added to the other side.

You knew I'd make it more complicated. 😜

[TMRh20]

WTF u reading my mind now @2bndy5 ? I was literally going to say something about u making it more complicated...

Tests ok in my limited tests, and seems to all make sense.

[2bndy5]

I went through any code that altered next_frame. The only other blocks that do, aside from what was discussed here, are when the last frag is received or when network.read() is called. Both instances seem fine to me, but pointers are hard.

FWIW, I don't have to worry about this low-level tediousness in rust. Instead, the rust compiler complains about tracking data ownership, which is still easier to cope with.