modm-io / modm

modm: a C++23 library generator for AVR and ARM Cortex-M devices
https://modm.io
Mozilla Public License 2.0
720 stars 128 forks source link

FreeRTOS queues for the uart module #1155

Closed kapacuk closed 3 months ago

kapacuk commented 3 months ago

I suggest a small improvement for the uart module. For projects which are using both FreeRTOS and buffered uart, waiting could be done much more efficiently - instead of a busy loop a task could just sleep. Also, with this change it's now possible to call read and write functions with a timeout.

If you agree with this approach it can be extended to other peripherals (SPI, I2C, etc)

salkinium commented 3 months ago

Do you think it would be possible to extract the data handling from the UART driver entirely? I don't like the queue size configuration via lbuild, I'd rather configure this in C++ and at the same time also chose which queue implementation I'd like ie. a simple FIFO, or a priotity queue, or a RTOS queue, or a DMA transaction etc. Ideally those queue strategies would also be compatible with other data peripherals.

Is that something you would find interesting to work on? The current trajectory will create a giant mess in the HAL if applied at scale.

kapacuk commented 3 months ago

Is that something you would find interesting to work on? The current trajectory will create a giant mess in the HAL if applied at scale.

I agree that it would be better to do it at C++ level. I'm interested and I would like to work on it, but I will need some guidance (at least initially) because I don't know the codebase that well.

rleh commented 3 months ago

As @salkinium has already said, I would prefer a solution too, that split the uart driver into the actual driver that interacts with the hardware and a wrapper that handles the buffering. This would make it very easy to maintain different implementations for buffering, without the unnecessarily complicated Jinja-C++ code mix and simple unit test options for all implementations. (Our unit tests currently only cover code-branches accessible through lbuild default settings.)

kapacuk commented 3 months ago

OK, I'll give it a try, will do it on a separate branch.

kapacuk commented 3 months ago

If we configure buffering in C++, then what should we do with the interrupt handler? Depending on the type of queue chosen by the user, the ISR would have to do different things. I don't think it's possible to create a template which would produce an ISR when it is instantiated.

rleh commented 3 months ago

The Uart driver could provide a function to register an ISR handler which is executed in the ISR, similar to e.g. the attachConfigurationHandler(…) from our SPI device interface.

using IsrHandler = void(*)();

void registerIsrHandler(IsrHandler handler);

and the wrapper registers a callback for the ISR.

kapacuk commented 3 months ago

The Uart driver could provide a function to register an ISR handler which is executed in the ISR

That means some runtime overhead, but I guess it's unavoidable. It's not that much anyway.