Fix more STM32F1 Serial hangs due to overflows

[sjasonsmith]

Description

Earlier changes to the STM32F1 serial ISR has eliminated hanging due to serial overflows when using serial 1-5 for Marlin or DGUS communication.

These changes missed the following two usages of Serial, which could still hang:

1. Malyan LCD, which hard-coded Serial1 in its cpp file.
2. TMC Hardware Serial

To resolve this, several changes were made:

1. Add MALYAN_LCD_SERIAL_PORT to Configuration.h.
2. Update STM32F1 HAL.h to handle this new port like it does for other ports which may control the printer.
3. Always instantiate all MSerial ports (1-3 or 1-5, depending on board) to allow them to be easily used from pins files.
4. Make emergency parser optional in MSerial class, so it won't be enabled for MALYAN_LCD or TMC.
5. Update all STM32F1 pins files to use MSerial instead of Serial.
6. Add static asserts that detect anything using Serial classes, since they can cause the board to hang.

Benefits

Avoid potentially unsafe hangs when using hardware serial for a Malyan LCD or TMC UART communication.

Configurations

These configurations are for an SKR E3 DIP with TMC 2209 drivers and a Malyan LCD attached to the TFT port.
Using this I was able to reproduce hangs while printing prior to this change, even though this is using SoftwareSerial for the TMC Drivers.

Configurations_Malyan.zip

Related Issues

#18358

[sjasonsmith]

I believe the TMC Hardware Serial changes will work, but I have not tested them yet.

This also needs some more work to let this MALYAN_LCD_SERIAL_PORT option work on other HALs, most importantly STM32.

[sjasonsmith]

It is building for the STM32 HAL now, but I have not yet tested it. I need to test the following:

1. Actual behavior using a Malyan LCD with a Malyan control board

2. STM32F1 Emergency Parser functionality (to make sure I didn't accidentally break it)

[sjasonsmith]

@minosg did the original investigation into the Serial hangs and was the remaining person able to still reproduce hangs after the changes from @rhapsodyv. They just posted this comment in the bug report:

@sjasonsmith I would like to report that after 24 hours of non stop testing the PR appears to fix the issue. I didn't modify the malyan lcd and just used the one in the branch, copying over the configurations .

Sir I think that this is it.....

I will try to finish the remaining testing needed on this tonight.

[thinkyhead]

I consolidated to a single LCD_SERIAL_PORT, did additional cleanup, and now it seems to be in good shape for a test.

[thisiskeithb]

Is Serial* and MSerial* the same thing?

I ask because in 4b928b2, the pins file for the SKR Mini E3 V2 (and others) changed X_HARDWARE_SERIAL from Serial4 to MSerial4 as an example.

.pio/build/STM32F103RC_btt_512K_USB/src/src/module/stepper/trinamic.cpp.o: In function `tmc_serial_begin()':
/Volumes/3TB/github/Marlin/Marlin/src/module/stepper/trinamic.cpp:482: warning: undefined reference to `MSerial4'
.pio/build/STM32F103RC_btt_512K_USB/src/src/module/stepper/trinamic.cpp.o: In function `_GLOBAL__sub_I_stepperX':
/Volumes/3TB/github/Marlin/Marlin/src/module/stepper/trinamic.cpp:846: warning: undefined reference to `MSerial4'

It compiles, but I still need to test it on real hardware to see if TMCs work correctly.

[sjasonsmith]

Is Serial* and MSerial* the same thing?

I ask because in 4b928b2, the pins file for the SKR Mini E3 V2 (and others) changed X_HARDWARE_SERIAL from Serial4 to MSerial4 as an example.

It compiles, but I still need to test it on real hardware to see if TMCs work correctly.

It will probably crash or otherwise behave badly right now, since it doesn't have the change to MarlinSerial.cpp that makes sure these are always instantiated.

The change to MSerial is important, because it solved the hangs due to the Serial ISR, but part of the change has already been committed and part is still in this PR.

[sjasonsmith]

Testing update, related to the current state of this PR (not Keith's comment above):

Malyan LCD is still working fine on an SKR E3 DIP. No hangs observed.

Switched to an SKR Mini E3 1.0, so that I could test with hardware serial for TMC UARTs.
I wanted to see if I could hang the controller with nothing but TMC serial using the Maple interrupt handlers.
I made the following changes, to create my own "worse-case scenario".

1. Changed back to Serial4 in the pins file, instead of MSerial4
2. Increase TMC_BAUD_RATE to 500000 to increase the likelihood of a UART overrun.
3. Enabled MONITOR_DRIVER_STATUS
4. Dialed my feed rate of my test print up to 200%, which is high as this Malyan LCD lets me go.

I then started the print and sent M122 repeatedly. I reproduced hangs several times within the first 10 queries or so.

With the exact same configuration, but switched back to MSerial (as this PR does)
I can't get anything to hang. I ran M122 until I got tired, I haven't even observed a communication issue. TMCStepper's error detection and retries are probably hiding the overflows that resultsed in hangs when using Serial.

Now to try this on an actual Malyan board...

[sjasonsmith]

On a Malyan STM32F070CB board I was able to communicate with the Malyan LCD, which is the main thing I wanted to test with that board.

[sjasonsmith]

@thinkyhead I just found one more tiny fix that I pushed.

I think this is ready to go in, I'm not planning any more testing on my end. I didn't test the Emergency Parser. I realized I didn't have a great way to test it since my F1 boards use USB Serial for host communication and not one of the impacted UARTs.

Given the issue @thisiskeithb pointed out, where STM32F1 pins files were already updated to use MSerial for TMC drivers, I think it is pretty urgent to merge this before a huge pile of SKR Mini E3 issues show up.

[thinkyhead]

I haven't looked too closely at the details of the emergency parser changes, but I see that the initializer structures (e.g., MarlinSerialCfg) already contain a flag whether to apply emergency parser to the serial port. How does the addition of the serial_handles_emergency interact with the existing behavior initializing serial ports as emergency-capable (or not)?

if (Cfg::EMERGENCYPARSER) emergency_parser.update(emergency_state, c);

        if (serial.emergency_parser_enabled)
          emergency_parser.update(serial.emergency_state, c);

[thinkyhead]

I've done some more review while looking at adding an emergency_parser_enabled() method to all the MarlinSerial classes so they all follow the same semantics….

I see that STM32F1 version of MarlinSerial is inheriting from HardwareSerial, and it doesn't follow the approach that other MarlinSerial classes use. Other varying MarlinSerial classes initialize with a SerialConfig structure that contains a Cfg::EMERGENCYPARSER field.

Should we modify this MarlinSerial class so it follows the same pattern and gets the value from a constant template element, or do you feel it is better going forward for the STM32F1 version to use a const data member flag, as it does now?

I'll push my commit adding emergency_parser_enabled(), and then I'll look at which construction is most efficient.

I also noticed that we could filter out construction of unused serial ports with the addition of an ANY_SERIAL_IS() macro to check whether a given serial port is needed.

[rhapsodyv]

I see that STM32F1 version of MarlinSerial is inheriting from HardwareSerial, and it doesn't follow the approach that other MarlinSerial classes use. Other varying MarlinSerial classes initialize with a SerialConfig structure that contains a Cfg::EMERGENCYPARSER field.

MarlinSerial from: LPC, STM32F1 and STM32 inherits from the current framework HardwareSerial class.
MarlinSerialCfg is a AVR class, isn't?

But yes, we can try to have a more standard MarlinSerial class. I took STM32 and LPC MarlinSerial as base, when I did it to STM32F1.

[thinkyhead]

Well, MarlinSerialCfg is just a structure used as a template argument. That technique allows C++ to reduce some redundancy by creating separate classes only if they differ in their initialization, and allows some values to exist as template constants rather than as formal data members. In some cases, it can be more efficient. It tends to reduce SRAM usage. But if SRAM is not too scarce, then the usual C++ practice of having only a single class, but adding fields to modify its behavior can sometimes be more desirable.

Typically, when a question like this arises, we'll try both, see what kind of garbage the compiler produces, and then pick the one that seems less bad.

[sjasonsmith]

I also noticed that we could filter out construction of unused serial ports with the addition of an ANY_SERIAL_IS() macro to check whether a given serial port is needed.

Just make sure this includes TMC Hardware Serial. I think I saw a similar macro that was only checking for the three “parsing” ports. This is really only needed for STM32F1, where TMC serial needs to use MSerial to get the safe interrupt handler.

[thinkyhead]

Just make sure this includes TMC Hardware Serial.

It looks like we may only be able to skip those that are trivially defined. It's unfortunate that the pins files are declaring the hardware ports by name instead of by index. If they were defined by index they could be used both to determine the serial port index they use, but also to determine if an extra serial port needs to be defined for them.

[sjasonsmith]

It's unfortunate that the pins files are declaring the hardware ports by name instead of by index.

Perhaps this could be a good follow up. That would also allow removing the hacky static asserts I used to detect if the wrong serial class was specified.

I’d rather see it as a follow up than block this PR for that work, especially since bugfix is currently broken for some of these F1 boards.

[thinkyhead]

Agreed.

[sjasonsmith]

Looks like the merge beat my last comment. Thanks!