Fix extra (breaking) TMC serial begin calls

[simon-jouet]

Description

Using my ESP32 board, the I2S stepper stream doesn't work when using more than one TMC2209. With just one driver it's working perfectly fine, but as soon as a second driver is enabled the i2s stream data isn't working.

I am pretty convinced that the issue is because the Serial2 HardwareSerial is initialised somewhere and therefore pin 17 is redefined from being the I2S output to be the default Serial2 TX pin. By initialising I2S after the Serial port then we can properly define the pin direction and function overriding any default initialisation from the Serial port.

I think this however is hiding an issue in the serial port initialisation somewhere else, in my config Serial2 should never be used. I believe the issue might be coming from the TMC code for two reasons:

1. It only happens with TMC2209 enabled... (I haven't tried with other trinamic drivers but it's not an issue with DRV8825)
2. The ESP32 initialisation is done in both HAL_init() and HAL_init_board(), HAL_init() is done before everything else, while HAL_init_board() is done after a few things including the TMC initialisation. If I follow the same sequence of initialisation but place it in HAL_init() then it doesn't work (hence something redefined pin 17). If it's placed in HAL_init_board at it is now, then it's working fine.

Going through the code, I couldn't find where Serial2 could be initialised even if it's not enabled, and I didn't spend much time going through the TMC code

[thinkyhead]

Using my ESP32 board

What board would that be?

…because the Serial2 HardwareSerial is initialised somewhere…

I see this in ESP32 for those using ESP3D_WIFISUPPORT…

    #define MYSERIAL1 Serial2Socket

I'm not sure if that could be coming into play, or if it relates to the common Arduino "Serial2."

I'll take a look at the TMC stuff, but I can't imagine how pin 17 would be affected by UART initialization of a different and unused serial port, unless it's in the ESP32 platform's own Serial init.

In any case, I've been looking at whether we could init the Arduino-flavored serial ports as the first step in setup(), ahead of the HAL_init. That might be something to try here also.

[simon-jouet]

What board would that be?

It's the next iteration of the ESP32 I sent you previously, this one include i2s stepping and was the basis for a few other ESP32 boards and the reference for the current i2s implementation in Marlin. https://github.com/simon-jouet/ESP32Controller/tree/r2 (I still have to send you one of those, but now is probably not the time)

I see this in ESP32 for those using ESP3D_WIFISUPPORT…

#define MYSERIAL1 Serial2Socket
I'm not sure if that could be coming into play, or if it relates to the common Arduino "Serial2."

I had a quick look at that when debugging but I'm pretty sure that this is not Serial2 but the bridge to allow serial messages to be sent over wifi. @luc-github would be the ones to know that though

I'll take a look at the TMC stuff, but I can't imagine how pin 17 would be affected by UART initialization of a different and unused serial port, unless it's in the ESP32 platform's own Serial init.

I think port 17 here doesn't matter too much, it's just that on the ESP32 most peripherals can be routed to different GPIOs using the GPIO matrix and by default port 17 is used as TX for Serial2.

You're right it could be something to do with the ESP32 serial init, the reason I don't think it's the case it's because if I just have one TMC2209 configured everything is working fine. In that case Serial2 doesn't get initialised and the stepping is working fine. Also if it's the ESP32 initialisation, I would have expected what's in board_init() to work in hal_init() before the TMC are initialised but it doesn't and after that none of the Serial config is modified by the ESP32 HAL.

My guess (very much so) was that the Serial devices are used in sequence for the different TMC2209 drivers, but when using one UART shared between multiple drivers then some serial devices get initialised when the shouldn't. (Like I say that's a bit of a wild guess)

I will keep looking when i've got some time, I would like to find the source of the redefinition

[luc-github]

Serial2Socket is just a name for class bridge to wifi (Serial to Socket), nothing related to Serial2

[vivian-ng]

@simon-jouet

Going through the code, I couldn't find where Serial2 could be initialised even if it's not enabled, and I didn't spend much time going through the TMC code

Could this portion in HardwareSerial.cpp in the arduino-esp32 core be creating the serial objects?

#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL)
HardwareSerial Serial(0);
HardwareSerial Serial1(1);
HardwareSerial Serial2(2);
#endif

The thing is, even though the serial objects would be initialized with these lines, the actual assignment of pins seems to be done only if begin() is called.

[simon-jouet]

@vivian-ng thanks for that

I figured the issue, and it's a compound issue of how the TMC initialisation works and an issue in arduino-esp32

• trinamic.cpp does a begin() on the serial for each axis https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.0.x/Marlin/src/module/stepper/trinamic.cpp#L334 -- The problem here is that when using TMC2209 on a single UART the same Serial is initialised multiple times. It's equivalent to calling Serial1.begin(115200); Serial1.begin(115200); ... which AFAIK is incorrect. Anyway that causes the issue in the arduino ESP32 uart hal
• When begin() is called on an already initialised uart the arduino esp32 HardwareSerial forces the previous Serial connection to be closed https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/HardwareSerial.cpp#L40 which calls https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/esp32-hal-uart.c#L229
• The issue is then that the uartEnd seems to mess up the other serial ports than the one defined and in that case GPIO17 which is the default of Serial2 but used for something else is redefined. HardwareSerial freezing ESP32 espressif/arduino-esp32#3563 (comment)

I was able to reproduce exactly the same issue simply by commenting out tmc_serial_begin(); in MarlinCore.cpp and just having two Serial1.begin(115200). If I comment out the end() in the HardwareSerial implementation then it works.

Although there is clear issue in the HardwareSerial implementation of the esp32 I do think the trinamic code should not call .begin() multiple times on the same serial port as from everything i'm reading this is not correct without first flushing and ending the serial port.

Hope that clarifies the issue here!

[thinkyhead]

Thanks for the insights. If all the serial classes were under our control it would be easy to add a "did_begin" flag. The actual solution won't be quite as pretty, but I think I can get it together pretty quickly.

[thinkyhead]

I'd be curious to see if -DNO_GLOBAL_SERIAL makes much of a difference in build size, if at all. We only need to declare the hardware serial ports we're actually using, after all.

[thinkyhead]

Ok, this should do the trick to prevent extra initialization. The compiler might even be smart enough to see the invariance and strip out the unused initializations. If we wanted to do the same thing with the TMC steppers connected to software serial ports, we'd need to modify the TMCStepper class to expose some things that are currently protected. But AFAIK there's no similar issue with the software ports at this time.

[simon-jouet]

Thanks @thinkyhead for that. I will have a look at your point regarding -DNO_GLOBAL_SERIAL when I've got some spare time

[simon-jouet]

@thinkyhead I think there is a small typo in your commit, it should be !sp_helper.began then begin not the other way around.

#define HW_SERIAL_BEGIN(A) do{ if (!sp_helper.began(TMCAxis::A, &A##_HARDWARE_SERIAL)) \

[thinkyhead]

Thanks @simon-jouet — Clearly quarantine has me losing my mind.