Combined LPC / Serial fixes (#21178)

Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>

Marlin/src/HAL/DUE/MarlinSerialUSB.cpp

@@ -92,12 +92,11 @@ int MarlinSerialUSB::read() {
   return c;
 }
 
-bool MarlinSerialUSB::available() {
+int MarlinSerialUSB::available() {
-    /* If Pending chars */
+  if (pending_char > 0) return pending_char;
-  return pending_char >= 0 ||
+  return pending_char == 0 ||
-    /* or USB CDC enumerated and configured on the PC side and some
+    // or USB CDC enumerated and configured on the PC side and some bytes where sent to us */
-       bytes where sent to us */
+    (usb_task_cdc_isenabled() && udi_cdc_is_rx_ready());
-      (usb_task_cdc_isenabled() && udi_cdc_is_rx_ready());
 }
 
 void MarlinSerialUSB::flush() { }

Marlin/src/HAL/DUE/MarlinSerialUSB.h

@@ -39,7 +39,7 @@ struct MarlinSerialUSB {
   int peek();
   int read();
   void flush();
-  bool available();
+  int available();
   size_t write(const uint8_t c);
 
   #if ENABLED(SERIAL_STATS_DROPPED_RX)

Marlin/src/HAL/LPC1768/MarlinSerial.cpp

@@ -25,20 +25,46 @@
 #include "MarlinSerial.h"
 
 #if ANY_SERIAL_IS(0)
-  MSerialT MSerial(true, LPC_UART0);
+  MarlinSerial _MSerial(LPC_UART0);
-  extern "C" void UART0_IRQHandler() { MSerial.IRQHandler(); }
+  MSerialT MSerial(true, _MSerial);
+  extern "C" void UART0_IRQHandler() { _MSerial.IRQHandler(); }
 #endif
 #if ANY_SERIAL_IS(1)
-  MSerialT MSerial1(true, (LPC_UART_TypeDef *) LPC_UART1);
+  MarlinSerial _MSerial1((LPC_UART_TypeDef *) LPC_UART1);
-  extern "C" void UART1_IRQHandler() { MSerial1.IRQHandler(); }
+  MSerialT MSerial1(true, _MSerial1);
+  extern "C" void UART1_IRQHandler() { _MSerial1.IRQHandler(); }
 #endif
 #if ANY_SERIAL_IS(2)
-  MSerialT MSerial2(true, LPC_UART2);
+  MarlinSerial _MSerial2(LPC_UART2);
-  extern "C" void UART2_IRQHandler() { MSerial2.IRQHandler(); }
+  MSerialT MSerial2(true, _MSerial2);
+  extern "C" void UART2_IRQHandler() { _MSerial2.IRQHandler(); }
 #endif
 #if ANY_SERIAL_IS(3)
-  MSerialT MSerial3(true, LPC_UART3);
+  MarlinSerial _MSerial3(LPC_UART3);
-  extern "C" void UART3_IRQHandler() { MSerial3.IRQHandler(); }
+  MSerialT MSerial3(true, _MSerial3);
+  extern "C" void UART3_IRQHandler() { _MSerial3.IRQHandler(); }
+#endif
+
+#if ENABLED(EMERGENCY_PARSER)
+
+  bool MarlinSerial::recv_callback(const char c) {
+    // Need to figure out which serial port we are and react in consequence (Marlin does not have CONTAINER_OF macro)
+    if (false) {}
+    #if ANY_SERIAL_IS(0)
+      else if (this == &_MSerial) emergency_parser.update(MSerial.emergency_state, c);
+    #endif
+    #if ANY_SERIAL_IS(1)
+      else if (this == &_MSerial1) emergency_parser.update(MSerial1.emergency_state, c);
+    #endif
+    #if ANY_SERIAL_IS(2)
+      else if (this == &_MSerial2) emergency_parser.update(MSerial2.emergency_state, c);
+    #endif
+    #if ANY_SERIAL_IS(3)
+      else if (this == &_MSerial3) emergency_parser.update(MSerial3.emergency_state, c);
+    #endif
+    return true;
+  }
+
 #endif
 
 #endif // TARGET_LPC1768

Marlin/src/HAL/LPC1768/MarlinSerial.h

@@ -47,15 +47,21 @@ public:
   void end() {}
 
   #if ENABLED(EMERGENCY_PARSER)
-    bool recv_callback(const char c) override {
+    bool recv_callback(const char c) override;
-      emergency_parser.update(static_cast<Serial0Type<MarlinSerial> *>(this)->emergency_state, c);
-      return true; // do not discard character
-    }
   #endif
 };
 
-typedef Serial0Type<MarlinSerial> MSerialT;
+// On LPC176x framework, HardwareSerial does not implement the same interface as Arduino's Serial, so overloads
+// of 'available' and 'read' method are not used in this multiple inheritance scenario.
+// Instead, use a ForwardSerial here that adapts the interface.
+typedef ForwardSerial0Type<MarlinSerial> MSerialT;
 extern MSerialT MSerial;
 extern MSerialT MSerial1;
 extern MSerialT MSerial2;
 extern MSerialT MSerial3;
+
+// Consequently, we can't use a RuntimeSerial either. The workaround would be to use a RuntimeSerial<ForwardSerial<MarlinSerial>> type here
+// Right now, let's ignore this until it's actually required.
+#if ENABLED(SERIAL_RUNTIME_HOOK)
+  #error "SERIAL_RUNTIME_HOOK is not yet supported for LPC176x."
+#endif

Marlin/src/HAL/LPC1768/usb_serial.cpp

@@ -29,8 +29,8 @@
 
 EmergencyParser::State emergency_state;
 
-bool CDC_RecvCallback(const char buffer) {
+bool CDC_RecvCallback(const char c) {
-  emergency_parser.update(emergency_state, buffer);
+  emergency_parser.update(emergency_state, c);
   return true;
 }
 

Marlin/src/HAL/shared/Delay.cpp

@@ -51,7 +51,7 @@
   // Use hardware cycle counter instead, it's much safer
   void delay_dwt(uint32_t count) {
     // Reuse the ASM_CYCLES_PER_ITERATION variable to avoid wasting another useless variable
-    register uint32_t start = HW_REG(_DWT_CYCCNT) - ASM_CYCLES_PER_ITERATION, elapsed;
+    uint32_t start = HW_REG(_DWT_CYCCNT) - ASM_CYCLES_PER_ITERATION, elapsed;
     do {
       elapsed = HW_REG(_DWT_CYCCNT) - start;
     } while (elapsed < count);
@@ -114,7 +114,7 @@
         serialprintPGM(unit);
         SERIAL_ECHOLNPAIR(" took: ", total);
         serialprintPGM(unit);
-        if (do_flush) SERIAL_FLUSH();
+        if (do_flush) SERIAL_FLUSHTX();
       };
 
       uint32_t s, e;

Marlin/src/core/serial.h

@@ -58,7 +58,6 @@ extern uint8_t marlin_debug_flags;
 //
 // Serial redirection
 //
-typedef int8_t serial_index_t;
 #define SERIAL_ALL 0x7F
 #if HAS_MULTI_SERIAL
   #define _PORT_REDIRECT(n,p)   REMEMBER(n,multiSerial.portMask,p)

Marlin/src/core/serial_base.h

@@ -79,7 +79,7 @@ struct SerialBase {
   void end()                        { static_cast<Child*>(this)->end(); }
   /** Check for available data from the port
       @param index  The port index, usually 0 */
-  bool available(uint8_t index = 0) { return static_cast<Child*>(this)->available(index); }
+  int available(uint8_t index = 0)  { return static_cast<Child*>(this)->available(index); }
   /** Read a value from the port
       @param index  The port index, usually 0 */
   int  read(uint8_t index = 0)      { return static_cast<Child*>(this)->read(index); }

Marlin/src/core/serial_hook.h

@@ -24,6 +24,9 @@
 #include "macros.h"
 #include "serial_base.h"
 
+// Used in multiple places
+typedef int8_t serial_index_t;
+
 // The most basic serial class: it dispatch to the base serial class with no hook whatsoever. This will compile to nothing but the base serial class
 template <class SerialT>
 struct BaseSerial : public SerialBase< BaseSerial<SerialT> >, public SerialT {
@@ -35,10 +38,11 @@ struct BaseSerial : public SerialBase< BaseSerial<SerialT> >, public SerialT {
 
   void msgDone() {}
 
-  bool available(uint8_t index) { return index == 0 && SerialT::available(); }
+  // We don't care about indices here, since if one can call us, it's the right index anyway
-  int read(uint8_t index)       { return index == 0 ? SerialT::read() : -1; }
+  int available(uint8_t)  { return (int)SerialT::available(); }
-  bool connected()              { return CALL_IF_EXISTS(bool, static_cast<SerialT*>(this), connected);; }
+  int read(uint8_t)       { return (int)SerialT::read(); }
-  void flushTX()                { CALL_IF_EXISTS(void, static_cast<SerialT*>(this), flushTX); }
+  bool connected()        { return CALL_IF_EXISTS(bool, static_cast<SerialT*>(this), connected);; }
+  void flushTX()          { CALL_IF_EXISTS(void, static_cast<SerialT*>(this), flushTX); }
 
   // We have 2 implementation of the same method in both base class, let's say which one we want
   using SerialT::available;
@@ -65,18 +69,19 @@ struct ConditionalSerial : public SerialBase< ConditionalSerial<SerialT> > {
   bool    & condition;
   SerialT & out;
   NO_INLINE size_t write(uint8_t c) { if (condition) return out.write(c); return 0; }
-  void flush()            { if (condition) out.flush();  }
+  void flush()                      { if (condition) out.flush();  }
-  void begin(long br)     { out.begin(br); }
+  void begin(long br)               { out.begin(br); }
-  void end()              { out.end(); }
+  void end()                        { out.end(); }
 
   void msgDone() {}
-  bool connected()              { return CALL_IF_EXISTS(bool, &out, connected); }
+  bool connected()          { return CALL_IF_EXISTS(bool, &out, connected); }
-  void flushTX()                { CALL_IF_EXISTS(void, &out, flushTX); }
+  void flushTX()            { CALL_IF_EXISTS(void, &out, flushTX); }
+
+  int available(uint8_t )   { return (int)out.available(); }
+  int read(uint8_t )        { return (int)out.read(); }
+  int available()           { return (int)out.available(); }
+  int read()                { return (int)out.read(); }
 
-  bool available(uint8_t index) { return index == 0 && out.available(); }
-  int read(uint8_t index)       { return index == 0 ? out.read() : -1; }
-  using BaseClassT::available;
-  using BaseClassT::read;
 
   ConditionalSerial(bool & conditionVariable, SerialT & out, const bool e) : BaseClassT(e), condition(conditionVariable), out(out) {}
 };
@@ -97,10 +102,10 @@ struct ForwardSerial : public SerialBase< ForwardSerial<SerialT> > {
   bool connected()              { return Private::HasMember_connected<SerialT>::value ? CALL_IF_EXISTS(bool, &out, connected) : (bool)out; }
   void flushTX()                { CALL_IF_EXISTS(void, &out, flushTX); }
 
-  bool available(uint8_t index) { return index == 0 && out.available(); }
+  int available(uint8_t)        { return (int)out.available(); }
-  int read(uint8_t index)       { return index == 0 ? out.read() : -1; }
+  int read(uint8_t)             { return (int)out.read(); }
-  bool available()              { return out.available(); }
+  int available()               { return (int)out.available(); }
-  int read()                    { return out.read(); }
+  int read()                    { return (int)out.read(); }
 
   ForwardSerial(const bool e, SerialT & out) : BaseClassT(e), out(out) {}
 };
@@ -125,8 +130,8 @@ struct RuntimeSerial : public SerialBase< RuntimeSerial<SerialT> >, public Seria
     if (eofHook) eofHook(userPointer);
   }
 
-  bool available(uint8_t index) { return index == 0 && SerialT::available(); }
+  int available(uint8_t)  { return (int)SerialT::available(); }
-  int read(uint8_t index)       { return index == 0 ? SerialT::read() : -1; }
+  int read(uint8_t)       { return (int)SerialT::read(); }
   using SerialT::available;
   using SerialT::read;
   using SerialT::flush;
@@ -157,21 +162,22 @@ struct RuntimeSerial : public SerialBase< RuntimeSerial<SerialT> >, public Seria
 
   // Forward constructor
   template <typename... Args>
-  RuntimeSerial(const bool e, Args... args) : BaseClassT(e), SerialT(args...) {}
+  RuntimeSerial(const bool e, Args... args) : BaseClassT(e), SerialT(args...), writeHook(0), eofHook(0), userPointer(0) {}
 };
 
 // A class that's duplicating its output conditionally to 2 serial interface
-template <class Serial0T, class Serial1T, const uint8_t offset = 0>
+template <class Serial0T, class Serial1T, const uint8_t offset = 0, const uint8_t step = 1>
-struct MultiSerial : public SerialBase< MultiSerial<Serial0T, Serial1T, offset> > {
+struct MultiSerial : public SerialBase< MultiSerial<Serial0T, Serial1T, offset, step> > {
-  typedef SerialBase< MultiSerial<Serial0T, Serial1T, offset> > BaseClassT;
+  typedef SerialBase< MultiSerial<Serial0T, Serial1T, offset, step> > BaseClassT;
 
   uint8_t    portMask;
   Serial0T & serial0;
   Serial1T & serial1;
 
   enum Masks {
-    FirstOutputMask   =  (1 << offset),
+    UsageMask         =  ((1 << step) - 1), // A bit mask containing as many bits as step
-    SecondOutputMask  =  (1 << (offset + 1)),
+    FirstOutputMask   =  (UsageMask << offset),
+    SecondOutputMask  =  (UsageMask << (offset + step)),
     AllMask           = FirstOutputMask | SecondOutputMask,
   };
 
@@ -185,19 +191,19 @@ struct MultiSerial : public SerialBase< MultiSerial<Serial0T, Serial1T, offset>
     if (portMask & FirstOutputMask)   serial0.msgDone();
     if (portMask & SecondOutputMask)  serial1.msgDone();
   }
-  bool available(uint8_t index) {
+  int available(uint8_t index) {
-    switch(index) {
+    if (index >= 0 + offset && index < step + offset)
-      case 0 + offset: return serial0.available();
+      return serial0.available(index);
-      case 1 + offset: return serial1.available();
+    else if (index >= step + offset && index < 2 * step + offset)
-      default: return false;
+      return serial1.available(index);
-    }
+    return false;
   }
-  NO_INLINE int read(uint8_t index) {
+  int read(uint8_t index) {
-    switch(index) {
+    if (index >= 0 + offset && index < step + offset)
-      case 0 + offset: return serial0.read();
+      return serial0.read(index);
-      case 1 + offset: return serial1.read();
+    else if (index >= step + offset && index < 2 * step + offset)
-      default: return -1;
+      return serial1.read(index);
-    }
+    return -1;
   }
   void begin(const long br) {
     if (portMask & FirstOutputMask)   serial0.begin(br);

Marlin/src/gcode/queue.cpp

@@ -63,6 +63,10 @@ GCodeQueue queue;
 // Frequently used G-code strings
 PGMSTR(G28_STR, "G28");
 
+#if NO_TIMEOUTS > 0
+  static millis_t last_command_time = 0;
+#endif
+
 /**
  * GCode line number handling. Hosts may opt to include line numbers when
  * sending commands to Marlin, and lines will be checked for sequentiality.
@@ -288,6 +292,10 @@ void GCodeQueue::enqueue_now_P(PGM_P const pgcode) {
  *   B<int>  Block queue space remaining
  */
 void GCodeQueue::ok_to_send() {
+  #if NO_TIMEOUTS > 0
+    // Start counting from the last command's execution
+    last_command_time = millis();
+  #endif
   #if HAS_MULTI_SERIAL
     const serial_index_t serial_ind = command_port();
     if (serial_ind < 0) return;
@@ -324,29 +332,20 @@ void GCodeQueue::flush_and_request_resend() {
   ok_to_send();
 }
 
-inline bool serial_data_available() {
-  byte data_available = 0;
-  if (MYSERIAL0.available()) data_available++;
-  #ifdef SERIAL_PORT_2
-    const bool port2_open = TERN1(HAS_ETHERNET, ethernet.have_telnet_client);
-    if (port2_open && MYSERIAL1.available()) data_available++;
-  #endif
-  return data_available > 0;
-}
 
-inline int read_serial(const uint8_t index) {
+// Multiserial already handle the dispatch to/from multiple port by itself
-  switch (index) {
+inline bool serial_data_available(uint8_t index = SERIAL_ALL) {
-    case 0: return MYSERIAL0.read();
+  if (index == SERIAL_ALL) {
-    case 1: {
+    for (index = 0; index < NUM_SERIAL; index++) {
-      #if HAS_MULTI_SERIAL
+      if (SERIAL_IMPL.available(index) > 0) return true;
-        const bool port2_open = TERN1(HAS_ETHERNET, ethernet.have_telnet_client);
-        if (port2_open) return MYSERIAL1.read();
-      #endif
     }
-    default: return -1;
+    return false;
   }
+  return SERIAL_IMPL.available(index) > 0;
 }
 
+inline int read_serial(const uint8_t index) { return SERIAL_IMPL.read(index); }
+
 void GCodeQueue::gcode_line_error(PGM_P const err, const serial_index_t serial_ind) {
   PORT_REDIRECT(SERIAL_PORTMASK(serial_ind)); // Reply to the serial port that sent the command
   SERIAL_ERROR_START();
@@ -460,7 +459,6 @@ void GCodeQueue::get_serial_commands() {
   // If the command buffer is empty for too long,
   // send "wait" to indicate Marlin is still waiting.
   #if NO_TIMEOUTS > 0
-    static millis_t last_command_time = 0;
     const millis_t ms = millis();
     if (length == 0 && !serial_data_available() && ELAPSED(ms, last_command_time + NO_TIMEOUTS)) {
       SERIAL_ECHOLNPGM(STR_WAIT);

docs/Serial.md

@@ -30,14 +30,25 @@ In the `Marlin/src/core/serial_hook.h` file, the different serial feature are de
 Since all the types above are using CRTP, it's possible to combine them to get the appropriate functionality.
 This is easily done via type definition of the feature.
 
-For example, to present a serial interface that's outputting to 2 serial port, the first one being hooked at runtime and the second one connected to a runtime switchable telnet client, you'll declare the type to use as:
+For example, to create a single serial interface with 2 serial outputs (one enabled at runtime and the other switchable):
 ```cpp
 typedef MultiSerial< RuntimeSerial<Serial>, ConditionalSerial<TelnetClient> > Serial0Type;
 ```
 
+To send the same output to 4 serial ports you could nest `MultiSerial` like this:
+```cpp
+typedef MultiSerial< MultiSerial< BaseSerial<Serial>, BaseSerial<Serial1> >, MultiSerial< BaseSerial<Serial2>, BaseSerial<Serial3>, 2, 1>, 0, 2> Serial0Type;
+```
+The magical numbers here are the step and offset for computing the serial port. Simplifying the above monster a bit:
+```cpp
+MS< A = MS<a, b, offset=0, step=1>, B=MS<c, d, offset=2, step=1>, offset=0, step=2>
+```
+This means that the underlying multiserial A (with output to `a,b`) is available from offset = 0 to offset + step = 1 (default value).
+The multiserial B (with output to `c,d`) is available from offset = 2 (the next step from the root multiserial) to offset + step = 3.
+In practice, the root multiserial will redirect any index/mask `offset` to `offset + step - 1` to its first leaf, and any index/mask `offset + step` to `offset + 2*step - 1` to its second leaf.
+
 ## Emergency parser
-By default, the serial base interface provide an emergency parser that's only enable for serial classes that support it.
+By default, the serial base interface provide an emergency parser that's only enable for serial classes that support it. Because of this condition, all underlying types take a first `bool emergencyParserEnabled` argument to their constructor. You must take into account this parameter when defining the actual type used.
-Because of this condition, all underlying type takes a first `bool emergencyParserEnabled` argument to their constructor. You must take into account this parameter when defining the actual type used.
 
 ## SERIAL macros
 The following macros are defined (in `serial.h`) to output data to the serial ports: