Fix G-Code queue

[X-Ryl669]

Description

This fix many subtle and hard to find bugs in the G-Code queue, see #21119 for details.
Mainly:

• Possible command buffer overflow
• Command buffer out-of-bound access
• Error handling blocking other serial port from making progress
• Wrong flushing of serial port with errors

It also fix a bug in LPC176x serial class I've missing in my previous round of fixes due to incompatible HardwareSerial method signatures (see #21010)

I've also removed Meatpack from the queue code and instead made it a serial class so it's transparent for Marlin and there is no special case anymore for Meatpack (and, in consequence, removed one level of looping in the queue code, so we can break out the loop upon error and continue processing the other serial port, instead of returning and dropping over serial port's data).

Requirements

Any board should be used

Benefits

It should fix #21010 and #21119

Configurations

See configuration attached in #21010

Related Issues

#21010 #21119

[ldursw]

I tried printing with this patch and something went wrong starting at line 37250 in the attached log.
serial.zip

[X-Ryl669]

Don't do that now, it's full of debugging code for a LPC1769 platform with multiserial hardcoded in it, so it'll break serial output. I'll inform you when it's ready, but for now, it's not ready...

[X-Ryl669]

In fact your serial log was very useful. The issue you had around line 37250 is due to a serial error and the printer actually resumed from the error (this does not happen with the code in bugfix) so this means that the fixes in this PR works.

Upon error, it's starting to dump all the serial state (character by character) so it becomes crazy in your log but it's very useful for me to figure out what's happening.

The error you've experienced is here

2021-02-19 18:16:46,021 - Send: N17690 G3 X127.995 Y115.889 I-13.533 J-12.327 E0.03128 F4200*13
2021-02-19 18:16:46,107 - Recv: echo:Unknown command: "17690 G3 X127.995 Y115.889 I-13.533 J-12.327 E0.03128 F4200"
2021-02-19 18:16:46,109 - Recv: ok P0 B63
2021-02-19 18:16:46,114 - Send: N17691 G1 X127.724 Y116.29 E0.02209*91
2021-02-19 18:16:46,118 - Recv: Error:Line Number is not Last Line Number+1, Last Line: 17689
2021-02-19 18:16:46,138 - Recv: Resend: 17690
2021-02-19 18:16:46,601 - Recv:  T:210.00 /210.00 B:54.94 /55.00 @:66 B@:24
2021-02-19 18:16:46,607 - Send: N17690 G3 X127.995 Y115.889 I-13.533 J-12.327 E0.03128 F4200*13
2021-02-19 18:16:46,614 - Recv: echo:A:5 L0 R0 CN SiS:0 SLB:N17691 G1 X127.724 Y116.29 E0.02209*91 SC:1
2021-02-19 18:16:46,617 - Recv: echo:A:43 L0 R0 C1 SiS:0 SLB:N17691 G1 X127.724 Y116.29 E0.02209*91 SC:2

So the printer failed to understand the command with line N17690 (I think the 'N' did not work). So it's asking to resend it and your host does that, and it resumes from the error.

[ldursw]

I know the PR is a draft but I tried it anyway because I was hopeful it would fix a long-standing issue with my printer where the first few characters of a command is sporadically lost. I've explained it in more detail here.

I'm sure it is not caused by a bad serial cable and it only happens when the gcode contains arcs. My guess is that it has something to do with the serial queue and how the string is processed when received.

[X-Ryl669]

In your case, you can monitor the number of bytes in your USB's serial port with the 'A:' part of the answer. If it's goes above the size of the buffer (I'm not speaking about BUFSIZE, but the Arduino's serial port internal buffer), the data will be lost resulting to a truncated command (as seen above).

If you look closely, at line 37660 and later, you'll see that you'll get errors when this number becomes larger than 63, I suspect you have a 64 byte buffer for the serial port. If I were you, I would reduce BUFSIZE, (it's almost useless anyway) to 4 or 8, and look at your framework's serial buffer size (look for HardwareSerial or CDCSerial, I don't know) and increase the buffer from 64 to 128.

[X-Ryl669]

@ldursw Try to set #define RX_BUFFER_SIZE 128 in your Configuration_adv.h file, it should solve your issue.

[ldursw]

Just tested the PR up to 052bc24 and MeatPack broke again with missing characters. This patch makes it work again.

diff --git a/Marlin/src/feature/meatpack.h b/Marlin/src/feature/meatpack.h
index e439913da6e3785aa22fb9e4c9cf38db26114117..96bab6d88dcee1e618c55bfca9c28ff3df26e65b 100644
--- a/Marlin/src/feature/meatpack.h
+++ b/Marlin/src/feature/meatpack.h
@@ -146,16 +146,16 @@ struct MeatpackSerial : public SerialBase <MeatpackSerial < SerialT >> {
     // There is a potential issue here with multiserial, since it'll return its decoded buffer whatever the serial index here.
     // So, instead of doing MeatpackSerial<MultiSerial<...>> we should do MultiSerial<MeatpackSerial<...>, MeatpackSerial<...>>
     // TODO, let's fix this later on
-    if (!charCount) {
-      // Don't read in read method, instead do it here, so we can make progress in the read method
-      while (out.available(index) > 0) {
-        const int r = out.read(index);
-        if (r == -1) return 0; // This is an error from the underlying serial code
-        meatpack.handle_rx_char((uint8_t)r, index);
-        charCount = meatpack.get_result_char(serialBuffer);
-        readIndex = 0;
-      }
+
+    // Don't read in read method, instead do it here, so we can make progress in the read method
+    while (charCount == 0 && out.available(index) > 0) {
+      const int r = out.read(index);
+      if (r == -1) return 0; // This is an error from the underlying serial code
+      meatpack.handle_rx_char((uint8_t)r, index);
+      charCount = meatpack.get_result_char(serialBuffer);
+      readIndex = 0;
     }
+
     return charCount;
   }

If get_result_char returns more than 0 and the buffer happens to have more data available the loop will run more than once causing data loss.

As an alternative the loop could be replaced with an if statement to prevent an infinite loop. I've also tested this patch and it works as well.

diff --git a/Marlin/src/feature/meatpack.h b/Marlin/src/feature/meatpack.h
index e439913da6e3785aa22fb9e4c9cf38db26114117..0b9d51e857521b881291c0aa60c37673910b7b8e 100644
--- a/Marlin/src/feature/meatpack.h
+++ b/Marlin/src/feature/meatpack.h
@@ -146,16 +146,17 @@ struct MeatpackSerial : public SerialBase <MeatpackSerial < SerialT >> {
     // There is a potential issue here with multiserial, since it'll return its decoded buffer whatever the serial index here.
     // So, instead of doing MeatpackSerial<MultiSerial<...>> we should do MultiSerial<MeatpackSerial<...>, MeatpackSerial<...>>
     // TODO, let's fix this later on
-    if (!charCount) {
-      // Don't read in read method, instead do it here, so we can make progress in the read method
-      while (out.available(index) > 0) {
-        const int r = out.read(index);
-        if (r == -1) return 0; // This is an error from the underlying serial code
-        meatpack.handle_rx_char((uint8_t)r, index);
-        charCount = meatpack.get_result_char(serialBuffer);
-        readIndex = 0;
-      }
-    }
+
+    if (charCount > 0) return charCount; // The buffer still has data
+    if (out.available(index) <= 0) return 0; // No data to read
+
+    // Don't read in read method, instead do it here, so we can make progress in the read method
+    const int r = out.read(index);
+    if (r == -1) return 0; // This is an error from the underlying serial code
+    meatpack.handle_rx_char((uint8_t)r, index);
+    charCount = meatpack.get_result_char(serialBuffer);
+    readIndex = 0;
+
     return charCount;
   }

[kad]

This PR broke BINARY_FILE_TRANSFER feature:

Marlin/src/gcode/sd/M28_M29.cpp:52:64: error: 'class GCodeQueue' has no member named 'port'
Marlin/src/gcode/sd/M28_M29.cpp:52:75: error: 'class GCodeQueue' has no member named 'index_r'; did you mean 'inject_P'?

this diff below probably will fix it, but I'm not sure in solution, haven't tried to use binary transfer to validate fix. People more familiar with that part of code, please double check:

diff --git a/Marlin/src/gcode/sd/M28_M29.cpp b/Marlin/src/gcode/sd/M28_M29.cpp
index 6f3f2450a1..f34edb6f7c 100644
--- a/Marlin/src/gcode/sd/M28_M29.cpp
+++ b/Marlin/src/gcode/sd/M28_M29.cpp
@@ -49,7 +49,7 @@ void GcodeSuite::M28() {
     // Binary transfer mode
     if ((card.flag.binary_mode = binary_mode)) {
       SERIAL_ECHO_MSG("Switching to Binary Protocol");
-      TERN_(HAS_MULTI_SERIAL, card.transfer_port_index = queue.port[queue.index_r]);
+      TERN_(HAS_MULTI_SERIAL, card.transfer_port_index = queue.ring_buffer.command_port());
     }
     else
       card.openFileWrite(p);

[thisiskeithb]

This PR broke BINARY_FILE_TRANSFER feature

Please open a bug report.

[X-Ryl669]

@kad, Yep you're right and your fix is correct IMHO. Either you open a PR with this fix, or I can do it if you don't know how.

[X-Ryl669]

Ok, I've done it. Thanks for the report!

[kad]

@X-Ryl669 I did (#21209), but yours got merged faster. Anyway, main point that this is solved now :)