BufferedUart template and its specialisation for FreeRTOS queues #1156
Conversation
| txResult = {{ hal }}::TxCallback(); | ||
|
|
||
| if( {{ hal }}::FinalCallback ) | ||
| {{ hal }}::FinalCallback(rxResult || txResult); |
There was a problem hiding this comment.
Could we reduce these three callbacks to just a single callback?
There was a problem hiding this comment.
My original idea was to allow using different buffer types for Rx and Tx. I can see now that it would be quite difficult to do it generically, given that FreeRTOS does not guarantee that the interrupt handler can do anything after calling portYIELD_FROM_ISR.
If we add the restriction that the Rx buffer must be of the same type as the Tx one, then of course a single callback would be enough. Would you be OK with such restriction?
There was a problem hiding this comment.
Would you be OK with such restriction?
I think that's ok. I don't think it makes sense to mix want to have a TX buffer with a FreeRTOS Queue with a "polling" rx queue.
However, I would like to have the option to have a buffer only for one direction and zero-length buffers. For example, our board logger setup is output only, so it doesn't need a RX buffer at all. Similarly for some UART-based communication protocols like AMNB, it would be a software RX buffer of ≥32B and only the hardware TX buffer (aka software buffer of length 0).
| static inline modm::atomic::Queue<uint8_t, {{ options["buffer.rx"] }}> rxBuffer; | ||
| %% endif | ||
| %% if options["buffer.tx"] | ||
| static inline modm::atomic::Queue<uint8_t, {{ options["buffer.tx"] }}> txBuffer; |
There was a problem hiding this comment.
At this point I'd make this a templated class so that we can remove the buffer.* options. I'm ok with breaking changes here, this is a significant improvement.
There was a problem hiding this comment.
Actually if we're gonna do breaking changes we can also deduplicate the common code as you initially intended.
There was a problem hiding this comment.
If I don't need to keep it backward-compatible then it becomes much easier. I'll remove the buffer.* options and will do it via another template specialisation which uses modm::atomic::Queue
There was a problem hiding this comment.
(I really dislike the buffer.* options, they are still from xpcc, modm's ancient predecessor library)
There was a problem hiding this comment.
I deleted the buffer.* options (and the old Uart class), which obviously broke a lot of examples. I fixed the blue_pill_f103 examples, it was just a matter of adding
using Usart2 = BufferedUart<UsartHal2, PlainTxBuffer<256>>;
at the top of main.cpp. Before I go ahead and convert all other examples - are you happy with this?
There was a problem hiding this comment.
Yes, I think this is pretty awesome, very flexible and fullfills all requirements. The recursive callback chaining is also quite elegant.
| // ---------------------------------------------------------------------------- | ||
|
|
||
| #include <modm/board.hpp> | ||
| #include <modm/platform/uart/freertos_uart_buffer.hpp> |
There was a problem hiding this comment.
I think this is auto-included by <modm/platform.hpp>, which is included by <modm/board.hpp>
There was a problem hiding this comment.
No, it's not auto-included any more, since I moved it to ext/aws
| static inline modm::atomic::Queue<uint8_t, {{ options["buffer.rx"] }}> rxBuffer; | ||
| %% endif | ||
| %% if options["buffer.tx"] | ||
| static inline modm::atomic::Queue<uint8_t, {{ options["buffer.tx"] }}> txBuffer; |
There was a problem hiding this comment.
(I really dislike the buffer.* options, they are still from xpcc, modm's ancient predecessor library)
…reertos_uart_buffer.hpp
replaced it with BufferedUart parameterised by PlainTxBuffer and PlainRxBuffer
|
Sorry it took me so long, that was a bigger change than I expected.. I think I'm done, it's ready for a review. |
No pressure! I'm very happy with this work, thank you for taking this on! I will review and test this in depth on the weekend. |
As discussed in PR#1155, here is my attempt to create a wrapper around the UartHal driver which handles the buffering. Not sure if it's what you had in mind, I'm open to suggestions on how to improve it.
I kept the original buffering driver with minor tweaks and created a new templated class BufferedUart. It duplicates some code from the old driver, but I think that's a small price to pay for backward compatibility.