Refactor serial class with templates

[X-Ryl669]

Description

In relation to #20755, this tries to improve the current serial class in Marlin so that G-Code console code is less hacky. It's touching a core feature, so care must be taken and I'm requesting for comments here.

I've tried to escape preprocessor's hell in serial.h, replacing it by template's hell.

More seriously, this merge all the different version of serial interface there are in Marlin (for USB, for Maple's Hardware, for STSTM32 Hardware, Linux, ESP3,2 etc...) into a single interface so that all platform will have EXACT same formatted output on the serial.

This interface is declared in src/core/serial_base.h.
I wanted to avoid virtual table here since the serial code is a core feature of Marlin and adding calling cost here would reduce the performance of the firmware. So I've used CRTP pattern, the serial base is using the interface of the given Child class via static dispatch that compiles to a direct code to the Child's class, so in effect, it's a zero cost solution.
I've added all the common serial interface here (a bunch of print/println/write method here) so I can remove them from all HAL (since they are different in the different HAL).

I've also added multiple Child implementation (in src/core/serial_hook.h):

1. BaseSerial is simply a redirect 1:1 to the underlying original code. In effect, this is the default implementation with 0 cost to use.
2. RuntimeSerial is a redirect with runtime modifiable hook functions. I'm interested in this for the G-Code console's PR above, but it can be used for many many usage (see below)
3. ConditionalSerial is a redirect with runtime checking a boolean variable before using the output. It's used to implement the ethernet's not-always-connected telnet, but it can be used for anything where the serial output can be masked
4. MultiSerial is a tee serial output that's redirecting its writed char to 2 templated output serial. It's using a mask to activate the output and input.

With this toolset, the mess of the serial output is largely simplified, since creating a serial output consist of writing the correct line of the desired type (like SerialBase<MultiSerial<MarlinSerial, ConditionalSerial<TelnetSerial>>>) and the compiler will compile this to a single class that's 0 overhead and multi-functionnal.

I've mapped all the existing serial configuration to the right type in serial.h and serial.cpp so it should be transparent; but any combination can now be written easily.

Requirements

Currently, I've modified almost all HAL.

Benefits

• Remove a lot of pseudo common code (like print/write/println/etc...)
• Reduce compiled binary size
• All assembling the serial output the way we want more easily
• Allow hooking the serial output easily (at compile time and at runtime)
• Common format on the serial output (for example, Linux's print functions weren't generating the same output as AVR or STM32)

In the future, I think it'll allow to simplify the WIFI code in the MKS's branch so that instead of having 2 GCode parser (Marlin and MKS's Wifi), we could have only one and hook the output of Marlin's processing to WIFI. This will create a true wireless serial console to Marlin.

Configurations

None, nothing changed here.

Related Issues

Please review and feel free to ask questions.

[GMagician]

On a very fast read I just see, on the samd side, that Serial and Serial1 are missing from this change. They are defined inside framework

[X-Ryl669]

Indeed, the same applies for Teensy. One need to extract the definition of the missing type from the relative framework to hook them.
I'll not be available for the next week, but I'll do it once I'm back.

[X-Ryl669]

Just a side note, is it possible to have the CI tools not cancel all task when it fails ? It'll be faster to fix all bug if they are all reported at once, instead of one after the other.

[thinkyhead]

is it possible to have the CI tools not cancel all task when it fails?

No. Apparently GitHub doesn't want to burn any more trees than it has to.

To do testing on your local system use the included mftest script. For a PR like this, it should suffice to run one test per architecture to confirm that the code is actually compiling.

At this time the linker step is not succeeding because it can't find any compiled instances of serial objects with the expected signatures. Once you've got this compiling for a few platforms, then push your updates and we'll see whether the rest of them also pass CI.

[sjasonsmith]

I think that overall it is a good idea to merge implementations, so that we will have more consistency between HALs.

As already mentioned, this touches core code, and is pretty high risk.

I suggest the following approach to move forward, once this is passing all build tests:

1. Create a list of important hardware/config combinations that should be tested pre-merge. Try to find volunteers to perform sanity-checks with this hardware.
2. Wait for 2.0.8 to release
3. Merge into bugfix-2.0.x, with the expectation that many bugs could be filed.

[X-Ryl669]

Now the implementation should be more or less final. Changes should be related to bug fixing the actual implementation (can't do that until next week). Please review the code and ask if anything seems wrong or confuse.

[GMagician]

@DrumClock your problem has been probably introduced by #20913 not by this PR

[X-Ryl669]

@GMagician is right here. The remaining compilation issues are due to #20913. We'll fix those first.
I don't know your setup. So I don't know what is supposed to work for your board and what not.
Ideally, we should have your initial configuration to work. If it does not, we'll start from here.

[GMagician]

@X-Ryl669 I traced down issue to:

  static void serial_echo_column_labels(const uint8_t sp) {
    SERIAL_ECHO_SP(7);
    LOOP_L_N(i, GRID_MAX_POINTS_X) {
      if (i < 10) SERIAL_CHAR(' ');
      SERIAL_ECHO(i);
      SERIAL_ECHO_SP(sp);
    }

what hangs the printer is SERIAL_ECHO(i);, going on

[GMagician]

@X-Ryl669 & @rhapsodyv I think I got it. It's a divide by 0 issue. I'll post a fix soon

[X-Ryl669]

Yes, since there is a write(const char*) method above.

[GMagician]

Ok, the problem here is:
void print(unsigned long c, int base = DEC) { printNumber(c, base); }
called by:
void print(unsigned char c, int base = 0)
that is called by all SERIAL_ECHO called inside LOOP_L_N. Loop uses uint8_t that is a char, but in G29 must print a number not a char.

First fast fix may be:

  void print(long c, int base = DEC)            { if (base) { write((const uint8_t*)"-", c < 0); printNumber(c < 0 ? -c : c, base); } else write(c); }
  void print(unsigned long c, int base = DEC)   { if (base) printNumber(c, base); else write(c); }

this will prevent divide by 0 but will not act as meant. I don't know if marlin print chars as chars with print calls so I can't propone a correct solution

[GMagician]

ok, I changed 'unsigned char' with default 'base=DEC' and 'char' with 'base=0'.
I also added divide by 0 check and posted a PR (#20938)

[X-Ryl669]

On your platform char is unsigned ? Please compile a static_assert(!std::is_same<char, uint8_t>::value, "Should break compilation"); to check if it breaks building.
In my code, I copied AVR's print functions as-is. It's probably a mistake if the signedness of char is changing between platform.

[GMagician]

On your platform char is unsigned ? Please compile a static_assert(!std::is_same<char, uint8_t>::value, "Should break compilation"); to check if it breaks building.
In my code, I copied AVR's print functions as-is. It's probably a mistake if the signedness of char is changing between platform.

My fault, uint8_t is unsigned char, chars are signed as they should be

[GMagician]

static_assert(!std::is_same<char, uint8_t>::value, "Should break compilation");

I just saw now that some environments use: -fsigned-char, I also read now that c/c++ on arm default chars to unsigned (as you stated), I was used to x86 c/c++ where chars were always signed

I confirm that samd51 is signed char by default (assertion doesn't break compilation) even if no argument is passed to compiler (but I think that such argument should be set by default on all platforms since Marlin need it)

[schimmi70]

First:
At Line 266 at Marlin/src/HAL/AVR/MarlinSerial.h
It should be "MSerialT3" not "MSerial3"

I got this compilation is errors:

Linking .pio\build\mega2560\firmware.elf
<artificial>:(.text+0xc): undefined reference to `MarlinSerial<MMU2SerialCfg<(unsigned char)1> >::available()'
<artificial>:(.text+0x14): undefined reference to `MarlinSerial<MMU2SerialCfg<(unsigned char)1> >::read()'
C:\Users\AGCS\AppData\Local\Temp\ccZJPAiJ.ltrans7.ltrans.o: In function `MMU2::tx_printf_P(char const*, int)':
<artificial>:(.text+0x64): undefined reference to `MarlinSerial<MMU2SerialCfg<(unsigned char)1> >::write(unsigned char)'
C:\Users\AGCS\AppData\Local\Temp\ccZJPAiJ.ltrans7.ltrans.o: In function `MMU2::tx_str_P(char const*)':
<artificial>:(.text+0x100): undefined reference to `MarlinSerial<MMU2SerialCfg<(unsigned char)1> >::write(unsigned char)'
C:\Users\AGCS\AppData\Local\Temp\ccZJPAiJ.ltrans7.ltrans.o: In function `MMU2::rx_str_P(char const*)':
<artificial>:(.text+0xf3c): undefined reference to `MarlinSerial<MMU2SerialCfg<(unsigned char)1> >::available()'
<artificial>:(.text+0xf48): undefined reference to `MarlinSerial<MMU2SerialCfg<(unsigned char)1> >::read()'
C:\Users\AGCS\AppData\Local\Temp\ccZJPAiJ.ltrans7.ltrans.o: In function `MMU2::init()':
<artificial>:(.text+0x1024): undefined reference to `MarlinSerial<MMU2SerialCfg<(unsigned char)1> >::begin(long)'
C:\Users\AGCS\AppData\Local\Temp\ccZJPAiJ.ltrans7.ltrans.o: In function `MMU2::mmu_loop()':
<artificial>:(.text+0x14f6): undefined reference to `MarlinSerial<MMU2SerialCfg<(unsigned char)1> >::write(unsigned char)'
collect2.exe: error: ld returned 1 exit status

Can i fix this ?

[GMagician]

@schimmi70 this may be not related to this PR.
Why do you say it should be MSerialT3? Aren't serial named, previuosly of this PR, Serial1, Serial2, Serial3 and so on?

[X-Ryl669]

@schimmi70 Indeed you're right. I made a mistake here. Please fix it in your code, I'm preparing a new PR with other fix, I'll include this one too.

[X-Ryl669]

@GMagician SerialX is the platform defined serial class. On some platform, it's being used directly in IRQ's functions in the platform, and in this case, the instance is wrapped in a ForwardSerial class that's using the SerialX instance. On other platform, there is no such usage, so the serial class is directly a BaseSerial or a RuntimeSerial (direct usage, the most efficient code).
All of these classes implement the SerialBase interface so that all code in Marlin only depend on a single common interface.

Because of so many difference between platform, I've created a typedef MSerialT (and respectively MSerialTX for 2nd, 3rd, etc...) so that the code can directly use a MSerialT instance without having to know about the underlying serial code.

So on your platform, you might have a SerialX instance because it's declared in the platform, but it's not used directly, the MSerialTX instance is used instead.

[schimmi70]

@GMagician because on all other places it is namend MSerialT3 or MSerialTX

@X-Ryl669 I fixed in the Code, but i can't find why I get the compiler errors.

[GMagician]

Because of so many difference between platform, I've created a typedef MSerialT

@X-Ryl669 I missed such typedef in your PR

[X-Ryl669]

@schimmi70 You've probably missed this line in MarlinSerial.cpp:598

  template class MarlinSerial< MMU2SerialCfg<MMU2_SERIAL_PORT> >; //<< HERE
  MSerialT3 mmuSerial(MSerialT3::HasEmergencyParser);

Alternatively, you can try #20985 as it contains the fix.