Ensure serial output code match the intention of the developer

Marlin/src/HAL/AVR/MarlinSerial.cpp

@@ -595,7 +595,7 @@ MSerialT customizedSerial1(MSerialT::HasEmergencyParser);
     MarlinSerial<MMU2SerialCfg<MMU2_SERIAL_PORT>>::_tx_udr_empty_irq();
   }
 
-  template class MarlinSerial< MarlinSerialCfg<MMU2_SERIAL_PORT> >;
+  template class MarlinSerial< MMU2SerialCfg<MMU2_SERIAL_PORT> >;
   MSerialT3 mmuSerial(MSerialT3::HasEmergencyParser);
 #endif
 

Marlin/src/HAL/AVR/MarlinSerial.h

@@ -263,7 +263,7 @@
   };
 
   typedef Serial0Type< MarlinSerial< MMU2SerialCfg<MMU2_SERIAL_PORT> > > MSerialT3;
-  extern MSerial3 mmuSerial;
+  extern MSerialT3 mmuSerial;
 #endif
 
 #ifdef LCD_SERIAL_PORT

Marlin/src/MarlinCore.cpp

@@ -897,6 +897,8 @@ void setup() {
   #endif
   SERIAL_ECHOLNPGM("start");
 
+  //SERIAL_PRINT((uint8_t)3, 0);
+
   // Set up these pins early to prevent suicide
   #if HAS_KILL
     SETUP_LOG("KILL_PIN");

Marlin/src/core/macros.h

@@ -319,6 +319,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_base.h

@@ -22,22 +22,40 @@
 #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
@@ -78,39 +96,48 @@ 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'); }
+
+  // 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); }
 
-  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'); }
 
   // Print a number with the given base
   void printNumber(unsigned long n, const uint8_t base) {
-    if (!base) {
-      write((uint8_t)n);
-      return;
-    }
+    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,6 +149,16 @@ 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) {
@@ -147,7 +184,7 @@ 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;
       }

Marlin/src/feature/bedlevel/bedlevel.cpp

@@ -160,7 +160,7 @@ void reset_bed_level() {
     #ifndef SCAD_MESH_OUTPUT
       LOOP_L_N(x, sx) {
         serial_spaces(precision + (x < 10 ? 3 : 2));
-        SERIAL_ECHO(int(x));
+        SERIAL_ECHO(x);
       }
       SERIAL_EOL();
     #endif
@@ -172,7 +172,7 @@ void reset_bed_level() {
         SERIAL_ECHOPGM(" [");             // open sub-array
       #else
         if (y < 10) SERIAL_CHAR(' ');
-        SERIAL_ECHO(int(y));
+        SERIAL_ECHO(y);
       #endif
       LOOP_L_N(x, sx) {
         SERIAL_CHAR(' ');
@@ -196,7 +196,7 @@ void reset_bed_level() {
         #endif
       }
       #ifdef SCAD_MESH_OUTPUT
-        SERIAL_CHAR(' ', ']');            // close sub-array
+        SERIAL_ECHOPGM(" ]");            // close sub-array
         if (y < sy - 1) SERIAL_CHAR(',');
       #endif
       SERIAL_EOL();

Marlin/src/feature/bedlevel/ubl/ubl.cpp

@@ -150,7 +150,7 @@
     SERIAL_ECHO_SP(7);
     LOOP_L_N(i, GRID_MAX_POINTS_X) {
       if (i < 10) SERIAL_CHAR(' ');
-      SERIAL_ECHO((int)i);
+      SERIAL_ECHO(i);
       SERIAL_ECHO_SP(sp);
     }
     serial_delay(10);

Marlin/src/feature/direct_stepping.cpp

@@ -180,7 +180,7 @@ namespace DirectStepping {
     if (!page_states_dirty) return;
     page_states_dirty = false;
 
-    SERIAL_ECHO(Cfg::CONTROL_CHAR);
+    SERIAL_CHAR(Cfg::CONTROL_CHAR);
     constexpr int state_bits = 2;
     constexpr int n_bytes = Cfg::NUM_PAGES >> state_bits;
     volatile uint8_t bits_b[n_bytes] = { 0 };
@@ -192,10 +192,10 @@ namespace DirectStepping {
     uint8_t crc = 0;
     for (uint8_t i = 0 ; i < n_bytes ; i++) {
       crc ^= bits_b[i];
-      SERIAL_ECHO(bits_b[i]);
+      SERIAL_CHAR(bits_b[i]); // I think a binary output is required here, feel free to change to SERIAL_ECHO if a textual format is required instead
     }
 
-    SERIAL_ECHO(crc);
+    SERIAL_CHAR(crc);
     SERIAL_EOL();
   }
 

Marlin/src/feature/encoder_i2c.cpp

@@ -172,7 +172,7 @@ void I2CPositionEncoder::update() {
             float sumP = 0;
             LOOP_L_N(i, I2CPE_ERR_PRST_ARRAY_SIZE) sumP += errPrst[i];
             const int32_t errorP = int32_t(sumP * RECIPROCAL(I2CPE_ERR_PRST_ARRAY_SIZE));
-            SERIAL_ECHO(axis_codes[encoderAxis]);
+            SERIAL_CHAR(axis_codes[encoderAxis]);
             SERIAL_ECHOLNPAIR(" : CORRECT ERR ", errorP * planner.steps_to_mm[encoderAxis], "mm");
             babystep.add_steps(encoderAxis, -LROUND(errorP));
             errPrstIdx = 0;
@@ -192,7 +192,7 @@ void I2CPositionEncoder::update() {
     if (ABS(error) > I2CPE_ERR_CNT_THRESH * planner.settings.axis_steps_per_mm[encoderAxis]) {
       const millis_t ms = millis();
       if (ELAPSED(ms, nextErrorCountTime)) {
-        SERIAL_ECHO(axis_codes[encoderAxis]);
+        SERIAL_CHAR(axis_codes[encoderAxis]);
         SERIAL_ECHOLNPAIR(" : LARGE ERR ", int(error), "; diffSum=", diffSum);
         errorCount++;
         nextErrorCountTime = ms + I2CPE_ERR_CNT_DEBOUNCE_MS;
@@ -213,7 +213,7 @@ void I2CPositionEncoder::set_homed() {
     trusted++;
 
     #ifdef I2CPE_DEBUG
-      SERIAL_ECHO(axis_codes[encoderAxis]);
+      SERIAL_CHAR(axis_codes[encoderAxis]);
       SERIAL_ECHOLNPAIR(" axis encoder homed, offset of ", zeroOffset, " ticks.");
     #endif
   }
@@ -224,15 +224,15 @@ void I2CPositionEncoder::set_unhomed() {
   homed = trusted = false;
 
   #ifdef I2CPE_DEBUG
-    SERIAL_ECHO(axis_codes[encoderAxis]);
+    SERIAL_CHAR(axis_codes[encoderAxis]);
     SERIAL_ECHOLNPGM(" axis encoder unhomed.");
   #endif
 }
 
 bool I2CPositionEncoder::passes_test(const bool report) {
   if (report) {
     if (H != I2CPE_MAG_SIG_GOOD) SERIAL_ECHOPGM("Warning. ");
-    SERIAL_ECHO(axis_codes[encoderAxis]);
+    SERIAL_CHAR(axis_codes[encoderAxis]);
     serial_ternary(H == I2CPE_MAG_SIG_BAD, PSTR(" axis "), PSTR("magnetic strip "), PSTR("encoder "));
     switch (H) {
       case I2CPE_MAG_SIG_GOOD:
@@ -253,7 +253,7 @@ float I2CPositionEncoder::get_axis_error_mm(const bool report) {
               error = ABS(diff) > 10000 ? 0 : diff; // Huge error is a bad reading
 
   if (report) {
-    SERIAL_ECHO(axis_codes[encoderAxis]);
+    SERIAL_CHAR(axis_codes[encoderAxis]);
     SERIAL_ECHOLNPAIR(" axis target=", target, "mm; actual=", actual, "mm; err=", error, "mm");
   }
 
@@ -263,7 +263,7 @@ float I2CPositionEncoder::get_axis_error_mm(const bool report) {
 int32_t I2CPositionEncoder::get_axis_error_steps(const bool report) {
   if (!active) {
     if (report) {
-      SERIAL_ECHO(axis_codes[encoderAxis]);
+      SERIAL_CHAR(axis_codes[encoderAxis]);
       SERIAL_ECHOLNPGM(" axis encoder not active!");
     }
     return 0;
@@ -288,7 +288,7 @@ int32_t I2CPositionEncoder::get_axis_error_steps(const bool report) {
   errorPrev = error;
 
   if (report) {
-    SERIAL_ECHO(axis_codes[encoderAxis]);
+    SERIAL_CHAR(axis_codes[encoderAxis]);
     SERIAL_ECHOLNPAIR(" axis target=", target, "; actual=", encoderCountInStepperTicksScaled, "; err=", error);
   }
 
@@ -667,7 +667,7 @@ void I2CPositionEncodersMgr::report_position(const int8_t idx, const bool units,
   else {
     if (noOffset) {
       const int32_t raw_count = encoders[idx].get_raw_count();
-      SERIAL_ECHO(axis_codes[encoders[idx].get_axis()]);
+      SERIAL_CHAR(axis_codes[encoders[idx].get_axis()]);
       SERIAL_CHAR(' ');
 
       for (uint8_t j = 31; j > 0; j--)
@@ -723,7 +723,7 @@ void I2CPositionEncodersMgr::change_module_address(const uint8_t oldaddr, const
   // and enable it (it will likely have failed initialization on power-up, before the address change).
   const int8_t idx = idx_from_addr(newaddr);
   if (idx >= 0 && !encoders[idx].get_active()) {
-    SERIAL_ECHO(axis_codes[encoders[idx].get_axis()]);
+    SERIAL_CHAR(axis_codes[encoders[idx].get_axis()]);
     SERIAL_ECHOLNPGM(" axis encoder was not detected on printer startup. Trying again.");
     encoders[idx].set_active(encoders[idx].passes_test(true));
   }
@@ -748,7 +748,7 @@ void I2CPositionEncodersMgr::report_module_firmware(const uint8_t address) {
   if (Wire.requestFrom(I2C_ADDRESS(address), uint8_t(32))) {
     char c;
     while (Wire.available() > 0 && (c = (char)Wire.read()) > 0)
-      SERIAL_ECHO(c);
+      SERIAL_CHAR(c);
     SERIAL_EOL();
   }
 

Marlin/src/gcode/calibrate/G425.cpp

@@ -375,7 +375,7 @@ inline void probe_sides(measurements_t &m, const float uncertainty) {
 
   inline void report_measured_positional_error(const measurements_t &m) {
     SERIAL_CHAR('T');
-    SERIAL_ECHO(int(active_extruder));
+    SERIAL_ECHO(active_extruder);
     SERIAL_ECHOLNPGM(" Positional Error:");
     #if HAS_X_CENTER
       SERIAL_ECHOLNPAIR_P(SP_X_STR, m.pos_error.x);

Marlin/src/gcode/calibrate/M48.cpp

@@ -240,7 +240,7 @@ void GcodeSuite::M48() {
       sigma = SQRT(dev_sum / (n + 1));
 
       if (verbose_level > 1) {
-        SERIAL_ECHO((int)(n + 1));
+        SERIAL_ECHO(n + 1);
         SERIAL_ECHOPAIR(" of ", (int)n_samples);
         SERIAL_ECHOPAIR_F(": z: ", pz, 3);
         SERIAL_CHAR(' ');

Marlin/src/gcode/feature/leds/M7219.cpp

@@ -82,7 +82,7 @@ void GcodeSuite::M7219() {
     LOOP_L_N(r, MAX7219_LINES) {
       SERIAL_ECHOPGM("led_line[");
       if (r < 10) SERIAL_CHAR(' ');
-      SERIAL_ECHO(int(r));
+      SERIAL_ECHO(r);
       SERIAL_ECHOPGM("]=");
       for (uint8_t b = 8; b--;) SERIAL_CHAR('0' + TEST(max7219.led_line[r], b));
       SERIAL_EOL();

Marlin/src/gcode/host/M360.cpp

@@ -35,7 +35,7 @@
 static void config_prefix(PGM_P const name, PGM_P const pref=nullptr, const int8_t ind=-1) {
   SERIAL_ECHOPGM("Config:");
   if (pref) serialprintPGM(pref);
-  if (ind >= 0) { SERIAL_ECHO(int(ind)); SERIAL_CHAR(':'); }
+  if (ind >= 0) { SERIAL_ECHO(ind); SERIAL_CHAR(':'); }
   serialprintPGM(name);
   SERIAL_CHAR(':');
 }

Marlin/src/gcode/queue.cpp

@@ -298,10 +298,10 @@ void GCodeQueue::ok_to_send() {
   #if ENABLED(ADVANCED_OK)
     char* p = command_buffer[index_r];
     if (*p == 'N') {
-      SERIAL_ECHO(' ');
-      SERIAL_ECHO(*p++);
+      SERIAL_CHAR(' ');
+      SERIAL_CHAR(*p++);
       while (NUMERIC_SIGNED(*p))
-        SERIAL_ECHO(*p++);
+        SERIAL_CHAR(*p++);
     }
     SERIAL_ECHOPAIR_P(SP_P_STR, int(planner.moves_free()),
                       SP_B_STR, int(BUFSIZE - length));