Serial refactor. Default 8-bit ECHO to int, not char (MarlinFirmware#…

…20985)

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

Marlin/src/HAL/AVR/pinsDebug.h

@@ -235,8 +235,8 @@ static void print_is_also_tied() { SERIAL_ECHOPGM(" is also tied to this pin");
 
 inline void com_print(const uint8_t N, const uint8_t Z) {
   const uint8_t *TCCRA = (uint8_t*)TCCR_A(N);
-  SERIAL_ECHOPGM("    COM");
-  SERIAL_CHAR('0' + N, Z);
+  SERIAL_ECHOPAIR("    COM", AS_CHAR('0' + N));
+  SERIAL_CHAR(Z);
   SERIAL_ECHOPAIR(": ", int((*TCCRA >> (6 - Z * 2)) & 0x03));
 }
 
@@ -247,8 +247,8 @@ void timer_prefix(uint8_t T, char L, uint8_t N) {  // T - timer    L - pwm  N -
   uint8_t WGM = (((*TCCRB & _BV(WGM_2)) >> 1) | (*TCCRA & (_BV(WGM_0) | _BV(WGM_1))));
   if (N == 4) WGM |= ((*TCCRB & _BV(WGM_3)) >> 1);
 
-  SERIAL_ECHOPGM("    TIMER");
-  SERIAL_CHAR(T + '0', L);
+  SERIAL_ECHOPAIR("    TIMER", AS_CHAR(T + '0'));
+  SERIAL_CHAR(L);
   SERIAL_ECHO_SP(3);
 
   if (N == 3) {
@@ -262,19 +262,11 @@ void timer_prefix(uint8_t T, char L, uint8_t N) {  // T - timer    L - pwm  N -
   SERIAL_ECHOPAIR("    WGM: ", WGM);
   com_print(T,L);
   SERIAL_ECHOPAIR("    CS: ", (*TCCRB & (_BV(CS_0) | _BV(CS_1) | _BV(CS_2)) ));
-
-  SERIAL_ECHOPGM("    TCCR");
-  SERIAL_CHAR(T + '0');
-  SERIAL_ECHOPAIR("A: ", *TCCRA);
-
-  SERIAL_ECHOPGM("    TCCR");
-  SERIAL_CHAR(T + '0');
-  SERIAL_ECHOPAIR("B: ", *TCCRB);
+  SERIAL_ECHOPAIR("    TCCR", AS_CHAR(T + '0'), "A: ", *TCCRA);
+  SERIAL_ECHOPAIR("    TCCR", AS_CHAR(T + '0'), "B: ", *TCCRB);
 
   const uint8_t *TMSK = (uint8_t*)TIMSK(T);
-  SERIAL_ECHOPGM("    TIMSK");
-  SERIAL_CHAR(T + '0');
-  SERIAL_ECHOPAIR(": ", *TMSK);
+  SERIAL_ECHOPAIR("    TIMSK", AS_CHAR(T + '0'), ": ", *TMSK);
 
   const uint8_t OCIE = L - 'A' + 1;
   if (N == 3) { if (WGM == 0 || WGM == 2 || WGM ==  4 || WGM ==  6) err_is_counter(); }

Marlin/src/HAL/DUE/MarlinSerialUSB.cpp

@@ -33,10 +33,6 @@
 
 #include "MarlinSerialUSB.h"
 
-#if ENABLED(EMERGENCY_PARSER)
-  #include "../../feature/e_parser.h"
-#endif
-
 // Imports from Atmel USB Stack/CDC implementation
 extern "C" {
   bool usb_task_cdc_isenabled();
@@ -69,7 +65,7 @@ int MarlinSerialUSB::peek() {
 
   pending_char = udi_cdc_getc();
 
-  TERN_(EMERGENCY_PARSER, emergency_parser.update(emergency_state, (char)pending_char));
+  TERN_(EMERGENCY_PARSER, emergency_parser.update(static_cast<MSerialT*>(this)->emergency_state, (char)pending_char));
 
   return pending_char;
 }
@@ -91,7 +87,7 @@ int MarlinSerialUSB::read() {
 
   int c = udi_cdc_getc();
 
-  TERN_(EMERGENCY_PARSER, emergency_parser.update(emergency_state, (char)c));
+  TERN_(EMERGENCY_PARSER, emergency_parser.update(static_cast<MSerialT*>(this)->emergency_state, (char)c));
 
   return c;
 }
@@ -105,7 +101,6 @@ bool MarlinSerialUSB::available() {
 }
 
 void MarlinSerialUSB::flush() { }
-void MarlinSerialUSB::flushTX() { }
 
 size_t MarlinSerialUSB::write(const uint8_t c) {
 

Marlin/src/HAL/DUE/MarlinSerialUSB.h

@@ -34,21 +34,20 @@
 
 
 struct MarlinSerialUSB {
-  static void begin(const long);
-  static void end();
-  static int peek();
-  static int read();
-  static void flush();
-  static void flushTX();
-  static bool available();
-  static size_t write(const uint8_t c);
+  void begin(const long);
+  void end();
+  int peek();
+  int read();
+  void flush();
+  bool available();
+  size_t write(const uint8_t c);
 
   #if ENABLED(SERIAL_STATS_DROPPED_RX)
-    FORCE_INLINE static uint32_t dropped() { return 0; }
+    FORCE_INLINE uint32_t dropped() { return 0; }
   #endif
 
   #if ENABLED(SERIAL_STATS_MAX_RX_QUEUED)
-    FORCE_INLINE static int rxMaxEnqueued() { return 0; }
+    FORCE_INLINE int rxMaxEnqueued() { return 0; }
   #endif
 };
 typedef Serial0Type<MarlinSerialUSB> MSerialT;

Marlin/src/HAL/LPC1768/eeprom_sdcard.cpp

@@ -84,16 +84,16 @@ static void debug_rw(const bool write, int &pos, const uint8_t *value, const siz
   PGM_P const rw_str = write ? PSTR("write") : PSTR("read");
   SERIAL_CHAR(' ');
   serialprintPGM(rw_str);
-  SERIAL_ECHOLNPAIR("_data(", pos, ",", int(value), ",", int(size), ", ...)");
+  SERIAL_ECHOLNPAIR("_data(", pos, ",", value, ",", size, ", ...)");
   if (total) {
     SERIAL_ECHOPGM(" f_");
     serialprintPGM(rw_str);
-    SERIAL_ECHOPAIR("()=", int(s), "\n size=", int(size), "\n bytes_");
+    SERIAL_ECHOPAIR("()=", s, "\n size=", size, "\n bytes_");
     serialprintPGM(write ? PSTR("written=") : PSTR("read="));
     SERIAL_ECHOLN(total);
   }
   else
-    SERIAL_ECHOLNPAIR(" f_lseek()=", int(s));
+    SERIAL_ECHOLNPAIR(" f_lseek()=", s);
 }
 
 // File function return codes for type FRESULT. This goes away soon, but

Marlin/src/HAL/STM32F1/MarlinSerial.h

@@ -28,10 +28,6 @@
 #include "../../inc/MarlinConfigPre.h"
 #include "../../core/serial_hook.h"
 
-#if HAS_TFT_LVGL_UI
-  extern "C" { extern char public_buf_m[100]; }
-#endif
-
 // Increase priority of serial interrupts, to reduce overflow errors
 #define UART_IRQ_PRIO 1
 
@@ -49,28 +45,6 @@ struct MarlinSerial : public HardwareSerial {
       nvic_irq_set_priority(c_dev()->irq_num, UART_IRQ_PRIO);
     }
   #endif
-
-  #if HAS_TFT_LVGL_UI
-    // Hook the serial write method to capture the output of GCode command sent via LCD
-    uint32_t current_wpos;
-    void (*line_callback)(void *, const char * msg);
-    void *user_pointer;
-
-    void set_hook(void (*hook)(void *, const char *), void * that) { line_callback = hook; user_pointer = that; current_wpos = 0; }
-
-    size_t write(uint8_t c) {
-      if (line_callback) {
-        if (c == '\n' || current_wpos == sizeof(public_buf_m) - 1) { // End of line, probably end of command anyway
-          public_buf_m[current_wpos] = 0;
-          line_callback(user_pointer, public_buf_m);
-          current_wpos = 0;
-        }
-        else
-          public_buf_m[current_wpos++] = c;
-      }
-      return HardwareSerial::write(c);
-    }
-  #endif
 };
 
 typedef Serial0Type<MarlinSerial> MSerialT;

Marlin/src/HAL/shared/backtrace/backtrace.cpp

@@ -35,9 +35,9 @@ static bool UnwReportOut(void* ctx, const UnwReport* bte) {
   (*p)++;
 
   SERIAL_CHAR('#'); SERIAL_ECHO(*p); SERIAL_ECHOPGM(" : ");
-  SERIAL_ECHOPGM(bte->name ? bte->name : "unknown"); SERIAL_ECHOPGM("@0x"); SERIAL_PRINT(bte->function, HEX);
+  SERIAL_ECHOPGM(bte->name ? bte->name : "unknown"); SERIAL_ECHOPGM("@0x"); SERIAL_PRINT(bte->function, PrintBase::Hex);
   SERIAL_CHAR('+'); SERIAL_ECHO(bte->address - bte->function);
-  SERIAL_ECHOPGM(" PC:"); SERIAL_PRINT(bte->address,HEX); SERIAL_CHAR('\n');
+  SERIAL_ECHOPGM(" PC:"); SERIAL_PRINT(bte->address, PrintBase::Hex); SERIAL_CHAR('\n');
   return true;
 }
 

Marlin/src/MarlinCore.cpp

@@ -979,7 +979,7 @@ void setup() {
     );
   #endif
   SERIAL_ECHO_MSG("Compiled: " __DATE__);
-  SERIAL_ECHO_MSG(STR_FREE_MEMORY, freeMemory(), STR_PLANNER_BUFFER_BYTES, (int)sizeof(block_t) * (BLOCK_BUFFER_SIZE));
+  SERIAL_ECHO_MSG(STR_FREE_MEMORY, freeMemory(), STR_PLANNER_BUFFER_BYTES, sizeof(block_t) * (BLOCK_BUFFER_SIZE));
 
   // Some HAL need precise delay adjustment
   calibrate_delay_loop();

Marlin/src/core/macros.h

@@ -316,6 +316,12 @@
   namespace Private {
     template<bool, typename _Tp = void> struct enable_if { };
     template<typename _Tp>              struct enable_if<true, _Tp> { typedef _Tp type; };
+
+    template<typename T, typename U> struct is_same { enum { value = false }; };
+    template<typename T> struct is_same<T, T> { enum { value = true }; };
+
+    template <typename T, typename ... Args> struct first_type_of { typedef T type; };
+    template <typename T> struct first_type_of<T> { typedef T type; };
   }
   // C++11 solution using SFINAE to detect the existance of a member in a class at compile time.
   // It creates a HasMember<Type> structure containing 'value' set to true if the member exists

Marlin/src/core/serial.cpp

@@ -59,12 +59,14 @@ void serialprintPGM(PGM_P str) {
 void serial_echo_start()  { static PGMSTR(echomagic, "echo:"); serialprintPGM(echomagic); }
 void serial_error_start() { static PGMSTR(errormagic, "Error:"); serialprintPGM(errormagic); }
 
+void serial_echopair_PGM(PGM_P const s_P, serial_char_t v) { serialprintPGM(s_P); SERIAL_CHAR(v.c); }
 void serial_echopair_PGM(PGM_P const s_P, const char *v)   { serialprintPGM(s_P); SERIAL_ECHO(v); }
-void serial_echopair_PGM(PGM_P const s_P, char v)          { serialprintPGM(s_P); SERIAL_CHAR(v); }
+void serial_echopair_PGM(PGM_P const s_P, char v)          { serialprintPGM(s_P); SERIAL_ECHO(v); }
 void serial_echopair_PGM(PGM_P const s_P, int v)           { serialprintPGM(s_P); SERIAL_ECHO(v); }
 void serial_echopair_PGM(PGM_P const s_P, long v)          { serialprintPGM(s_P); SERIAL_ECHO(v); }
 void serial_echopair_PGM(PGM_P const s_P, float v)         { serialprintPGM(s_P); SERIAL_DECIMAL(v); }
 void serial_echopair_PGM(PGM_P const s_P, double v)        { serialprintPGM(s_P); SERIAL_DECIMAL(v); }
+void serial_echopair_PGM(PGM_P const s_P, unsigned char v) { serialprintPGM(s_P); SERIAL_ECHO(v); }
 void serial_echopair_PGM(PGM_P const s_P, unsigned int v)  { serialprintPGM(s_P); SERIAL_ECHO(v); }
 void serial_echopair_PGM(PGM_P const s_P, unsigned long v) { serialprintPGM(s_P); SERIAL_ECHO(v); }
 

Marlin/src/core/serial_base.h

@@ -22,25 +22,41 @@
 #pragma once
 
 #include "../inc/MarlinConfigPre.h"
+#include "macros.h"
 
 #if ENABLED(EMERGENCY_PARSER)
   #include "../feature/e_parser.h"
 #endif
 
-#ifndef DEC
-  #define DEC 10
-  #define HEX 16
-  #define OCT 8
-  #define BIN 2
-#endif
-
 // flushTX is not implemented in all HAL, so use SFINAE to call the method where it is.
 CALL_IF_EXISTS_IMPL(void, flushTX );
 CALL_IF_EXISTS_IMPL(bool, connected, true);
 
+// In order to catch usage errors in code, we make the base to encode number explicit
+// If given a number (and not this enum), the compiler will reject the overload, falling back to the (double, digit) version
+// We don't want hidden conversion of the first parameter to double, so it has to be as hard to do for the compiler as creating this enum
+enum class PrintBase {
+  Dec = 10,
+  Hex = 16,
+  Oct = 8,
+  Bin = 2
+};
+
+// A simple forward struct that prevent the compiler to select print(double, int) as a default overload for any type different than 
+// double or float. For double or float, a conversion exists so the call will be transparent
+struct EnsureDouble {
+  double a;
+  FORCE_INLINE operator double() { return a; }
+  // If the compiler breaks on ambiguity here, it's likely because you're calling print(X, base) with X not a double or a float, and a
+  // base that's not one of PrintBase's value. This exact code is made to detect such error, you NEED to set a base explicitely like this:
+  // SERIAL_PRINT(v, PrintBase::Hex) 
+  FORCE_INLINE EnsureDouble(double a) : a(a) {}
+  FORCE_INLINE EnsureDouble(float a) : a(a) {}
+};
+
 // Using Curiously Recurring Template Pattern here to avoid virtual table cost when compiling.
-// Since the real serial class is known at compile time, this results in compiler writing a completely
-// efficient code
+// Since the real serial class is known at compile time, this results in the compiler writing
+// a completely efficient code.
 template <class Child>
 struct SerialBase {
   #if ENABLED(EMERGENCY_PARSER)
@@ -78,39 +94,47 @@ struct SerialBase {
   FORCE_INLINE void write(const char* str)                    { while (*str) write(*str++); }
   FORCE_INLINE void write(const uint8_t* buffer, size_t size) { while (size--) write(*buffer++); }
   FORCE_INLINE void print(const char* str)                    { write(str); }
-  NO_INLINE void print(char c, int base = 0)              { print((long)c, base); }
-  NO_INLINE void print(unsigned char c, int base = 0)     { print((unsigned long)c, base); }
-  NO_INLINE void print(int c, int base = DEC)             { print((long)c, base); }
-  NO_INLINE void print(unsigned int c, int base = DEC)    { print((unsigned long)c, base); }
-  void print(unsigned long c, int base = DEC)             { printNumber(c, base); }
-  void print(double c, int digits = 2)                    { printFloat(c, digits); }
-  void print(long c, int base = DEC)                      {
-    if (!base) {
-      write(c);
-      return;
-    }
-    if (base == DEC && c < 0) {
-      write((uint8_t)'-'); c = -c;
-    }
-    printNumber(c, base);
-  }
+  // No default argument to avoid ambiguity
+  NO_INLINE void print(char c, PrintBase base)                { printNumber((signed long)c, (uint8_t)base); }
+  NO_INLINE void print(unsigned char c, PrintBase base)       { printNumber((unsigned long)c, (uint8_t)base); }
+  NO_INLINE void print(int c, PrintBase base)                 { printNumber((signed long)c, (uint8_t)base); }
+  NO_INLINE void print(unsigned int c, PrintBase base)        { printNumber((unsigned long)c, (uint8_t)base); }
+  void print(unsigned long c, PrintBase base)                 { printNumber((unsigned long)c, (uint8_t)base); }
+  void print(long c, PrintBase base)                          { printNumber((signed long)c, (uint8_t)base); }
+  void print(EnsureDouble c, int digits)                      { printFloat(c, digits); }
+
+  // Forward the call to the former's method
+  FORCE_INLINE void print(char c)                { print(c, PrintBase::Dec); }
+  FORCE_INLINE void print(unsigned char c)       { print(c, PrintBase::Dec); }
+  FORCE_INLINE void print(int c)                 { print(c, PrintBase::Dec); }
+  FORCE_INLINE void print(unsigned int c)        { print(c, PrintBase::Dec); }
+  FORCE_INLINE void print(unsigned long c)       { print(c, PrintBase::Dec); }
+  FORCE_INLINE void print(long c)                { print(c, PrintBase::Dec); }
+  FORCE_INLINE void print(double c)              { print(c, 2); }
+
+  FORCE_INLINE void println(const char s[])                  { print(s); println(); }
+  FORCE_INLINE void println(char c, PrintBase base)          { print(c, base); println(); }
+  FORCE_INLINE void println(unsigned char c, PrintBase base) { print(c, base); println(); }
+  FORCE_INLINE void println(int c, PrintBase base)           { print(c, base); println(); }
+  FORCE_INLINE void println(unsigned int c, PrintBase base)  { print(c, base); println(); }
+  FORCE_INLINE void println(long c, PrintBase base)          { print(c, base); println(); }
+  FORCE_INLINE void println(unsigned long c, PrintBase base) { print(c, base); println(); }
+  FORCE_INLINE void println(double c, int digits)            { print(c, digits); println(); }
+  FORCE_INLINE void println()                                { write('\r'); write('\n'); }
 
-  NO_INLINE void println(const char s[])                  { print(s); println(); }
-  NO_INLINE void println(char c, int base = 0)            { print(c, base); println(); }
-  NO_INLINE void println(unsigned char c, int base = 0)   { print(c, base); println(); }
-  NO_INLINE void println(int c, int base = DEC)           { print(c, base); println(); }
-  NO_INLINE void println(unsigned int c, int base = DEC)  { print(c, base); println(); }
-  NO_INLINE void println(long c, int base = DEC)          { print(c, base); println(); }
-  NO_INLINE void println(unsigned long c, int base = DEC) { print(c, base); println(); }
-  NO_INLINE void println(double c, int digits = 2)        { print(c, digits); println(); }
-  NO_INLINE void println()                                { write('\r'); write('\n'); }
+  // Forward the call to the former's method
+  FORCE_INLINE void println(char c)                { println(c, PrintBase::Dec); }
+  FORCE_INLINE void println(unsigned char c)       { println(c, PrintBase::Dec); }
+  FORCE_INLINE void println(int c)                 { println(c, PrintBase::Dec); }
+  FORCE_INLINE void println(unsigned int c)        { println(c, PrintBase::Dec); }
+  FORCE_INLINE void println(unsigned long c)       { println(c, PrintBase::Dec); }
+  FORCE_INLINE void println(long c)                { println(c, PrintBase::Dec); }
+  FORCE_INLINE void println(double c)              { println(c, 2); }
 
   // Print a number with the given base
-  void printNumber(unsigned long n, const uint8_t base) {
-    if (!base) {
-      write((uint8_t)n);
-      return;
-    }
+  NO_INLINE void printNumber(unsigned long n, const uint8_t base) {
+    if (!base) return; // Hopefully, this should raise visible bug immediately
+
     if (n) {
       unsigned char buf[8 * sizeof(long)]; // Enough space for base 2
       int8_t i = 0;
@@ -122,9 +146,19 @@ struct SerialBase {
     }
     else write('0');
   }
+  void printNumber(signed long n, const uint8_t base) {
+    if (base == 10 && n < 0) { 
+      n = -n; // This works because all platforms Marlin's builds on are using 2-complement encoding for negative number
+              // On such CPU, changing the sign of a number is done by inverting the bits and adding one, so if n = 0x80000000 = -2147483648 then
+              // -n = 0x7FFFFFFF + 1 => 0x80000000 = 2147483648 (if interpreted as unsigned) or -2147483648 if interpreted as signed.
+              // On non 2-complement CPU, there would be no possible representation for 2147483648.
+      write('-'); 
+    } 
+    printNumber((unsigned long)n , base);
+  }
 
   // Print a decimal number
-  void printFloat(double number, uint8_t digits) {
+  NO_INLINE void printFloat(double number, uint8_t digits) {
     // Handle negative numbers
     if (number < 0.0) {
       write('-');
@@ -147,13 +181,13 @@ struct SerialBase {
       // Extract digits from the remainder one at a time
       while (digits--) {
         remainder *= 10.0;
-        int toPrint = int(remainder);
+        unsigned long toPrint = (unsigned long)remainder;
         printNumber(toPrint, 10);
         remainder -= toPrint;
       }
     }
   }
 };
 
-// All serial instances will be built by chaining the features required for the function in a form of a template
-// type definition
+// All serial instances will be built by chaining the features required
+// for the function in the form of a template type definition.