[FR] Serial reading via DMA to improve serial communication reliability

[rondlh]

Is your feature request related to a problem? Please describe.

This feature requests is related to unreliable serial data communication I found when putting load on the motherboard by printing faster. I use a TFT (BTT TFT35 V3.0) with serial interface to print. I found that under high loads the motherboard (MKS Monster8 V1.0) sometimes drops a character, but no error codes or flags are raised. I managed to get DMA reading enabled in Marlin which completely solved this issue. By default Marlin uses the Arduino platform to read serial data, at 250K baud this is unstable, with DMA serial reading enabled 1M is rock solid. There must be something wrong in the Arduino platform.
The current implementation (#26328) supports the STM32F1/F2/F4 platform. F0 and F7 are still very experimental, and have not been tested on actual hardware, but are likely to work because F0 and F1 are about the same, and F4 and F7 are also almost the same.
The used DMA reading strategy writes data to a circular buffer, and doesn't need an interrupt service routine, which makes is very efficient. The key benefit is reliable serial communication at higher speeds.

Are you looking for hardware support?

No

Describe the feature you want

DMA serial reading to improve serial reading reliability and allow for higher serial baud rates.

[EvilGremlin]

You have to write this in STM32 HAL (starting with MarlinSerial::begin i guess) and make it completely transparent for all other codebase. While some small arch-dependent branching is ok in main code, it should be kept to absolute minimum.

[rondlh]

You have to write this in STM32 HAL (starting with MarlinSerial::begin i guess) and make it completely transparent for all other codebase. While some small arch-dependent branching is ok in main code, it should be kept to absolute minimum.

I have a bare metal implementation starting at "nothing" and handling the channel/stream selection depending on the USART used, ending with Serial_Get and Serial_Put. Using STM32 HAL would be very easy.

[EvilGremlin]

It'd be really cool if you can replace another piece of junk arduino code.
btw, you don't have to bother with flow control, it does not benefit our text terminal case. You can dig up thread on octoprint forum where this was attempted and failed.

[rondlh]

I see what needs to happen, basically a HardwareSerial class (HardwareSerial.cpp/h, uart.c/h) needs to be made. Lots of things in there are not even needed.

One odd thing I noticed, when I have data corruption (dropped character) there is a call to HAL_UART_ErrorCallback in uart.c, but ErrorCode is 0, no error flags are set, HAL_UART_GetError(huart) reports a 0. What's going on?

[rondlh]

I managed to isolate Class HardwareSerial out of Arduino so I can modify things.
Then created a class HardwareSerial2, and updated Marlin to work with HardwareSerial2 (just rename).
I disabled EMERGENCY_PARSER for now, but I can get that working too if needed.
I don't know how to override a method of HardwareSerial, only a few changes are required. Anybody???

Changed methods:
void HardwareSerial::begin(unsigned long baud, uint8_t config) (Start the free running DMA process)
int HardwareSerial2::available(void) (Update the buffer head pointer to the current DMA progress)
int HardwareSerial2::peek(void) (not sure if it is even used) (Update the buffer head pointer to the current DMA progress)
int HardwareSerial2::read(void) (Update the buffer head pointer to the current DMA progress)

Then it works already... and the best thing is, it solves my data corruption issue. I think the issue is actually in the Arduino platform, but it only shows under stress. Now 250K baud is stable. I'm going to test faster serial speeds now.
I will release the source code after a cleanup (STM32F2-F4 only). Anybody interested?

[EvilGremlin]

Yep, just open draft PR. We can clean it up and integrate properly later. Better name it same HardwareSerial and wrap in #if ENABLED(SERIAL_DMA), something like that.

[thisiskeithb]

I disabled EMERGENCY_PARSER for now, but I can get that working too if needed.

It'd be good to get EMERGENCY_PARSER working.

[EvilGremlin]

Yep, EMERGENCY_PARSER is pretty much obligatory by now, waaaaay too many things rely on it

[rondlh]

OK OK OK, no problem, EMERGENCY_PARSER IS WORKING... took a whole 4 lines of additional code.
It seems the EMERGENCY_PARSER just gets all available data in the RX buffer immediately.

I had to cheat a little bit to get it working, with interrupt based serial reading (1 interrupt per received character), the received data is just stored in the RX buffer and forwarded to EMERGENCY_PARSE. But here I use serial DMA to a circular RX buffer, so I don't get an interrupt for every byte received, I don't even use an interrupt handler. I could enable the Serial Idle interrupt, which would generate an interrupt after a message is received IF, and only if, the serial line goes to idle afterwards, which will not happening if commands keep coming (like with repeated M118 commands). So what I did instead is that when Marlin check for available data, or reads or peeks data, then I update the RX Buffer head with the current reported DMA status, and send all new data to the Emergency parse. I gave it a quick try when running my printer at 10%, and it seems to work fine.

I did some more speed testing, before I would have to lower the serial speed to 115200 baud to make sure the communication is stable, with DMA reading enabled I can run the printer at 1M baud without any issues. I know the communication is correct , at least towards the motherboards, because I use a checksum. I also use a stress test that sends over 1400 messages/s to the motherboards and get an Advanced OK back and the response to the send M118 command. I'm sure there is something going wrong in the Arduino framework!

I forked Marlin, please note that this is very experimental code, and it is likely to make your printer explode. I tested it on an MKS Monster8 V1.0 (STM32F407), the code should "work" on STM32F2/F4 MCU, if you use USART1-4.

[EvilGremlin]

Are F1/F7/H7 series DMA different? or you didn't look at it?

[rondlh]

Are F1/F7/H7 series DMA different? or you didn't look at it?

F1 is a little bit different, some register names are different (see below).
I'm not sure about F7 and H7, but the actual code needed to get this working is not so much.
So if you want it and can read a STM32 Reference Manual, then you should be able to get it working in an hour or 2.

F1:
// CCR DMA stream x configuration register
// CNDTR DMA stream x number of data register
// CPAR DMA stream x peripheral address register
// CMAR DMA stream x memory 0 address register
//
// ISR DMA low/high interrupt status register
// IFCR DMA low/high interrupt flag clear register

F2/F4:
// CR DMA stream x configuration register
// NDTR DMA stream x number of data register
// PAR DMA stream x peripheral address register
// M0AR DMA stream x memory 0 address register
//
// LISR DMA low interrupt status register
// HISR DMA high interrupt status register
// LIFCR DMA low interrupt flag clear register
// HIFCR DMA high interrupt flag clear register

[rondlh]

PR is up!

BTW: Adding DMA serial writing is also possible, but doesn't bring much in my view. Interrupt writing is fine, but interrupt serial reading (at least Arduino's version) is not.

[EvilGremlin]

no PR

[rondlh]

@EvilGremlin Now it's there... #26328

Update: I replaced HAVE_HWSERIALx with USING_HW_SERIALx.
So the correct serial port should be selected automatically.

Note: This code works for multiple serial ports, but does not affect the USB Serial port emulation.

[rondlh]

I can wrap the code with "#if ENABLED(SERIAL_DMA)", no problem.

How important is F7 and H7 support? There are only a few boards like this around.

A key problem I currently have is that a lot of build test get cancelled, and a few fail because "SERIAL_PORT 0", if I change that to 1 then they also build correctly. Anybody know how to deal with this?

[EvilGremlin]

Wrong topic. I guess CI can't pull configs, again. You can only wait for github to fix their shit, or Scott to fix settings, i don't know really.

[thisiskeithb]

Closing since you’ve opened a PR.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.