STM32: USART: Add eager_reads config option

[adamgreig]

Add a new option to control the behaviour of BufferedUartRx and RingBufferedUartRx.

Currently, BufferedUartRx returns as soon as any data is available in the buffer, even just a single byte, while RingBufferedUartRx returns either when line idle is detected or when the DMA internal buffer half-full or full position is written over.

In my case it's useful to have BufferedUartRx wait for more data so I can process larger packets (and typically complete packets due to the line idle), but in other cases it's useful for RingBufferedUartRx to return eagerly to reduce latency. This change allows choosing either behaviour and makes the two types consistent.

For BufferedUartRx the modification is fairly straightforward; RingBuffer gains is_half_full(), the interrupt handler wakes the waker if either "eager and buffer not empty" or "not eager and buffer half full" or "line idle". The RXNE interrupt is always enabled anyway.

For RingBufferedUartRx it's a bit trickier. We didn't use RXNE at all previously so it's now conditionally enabled when eager is set. The interrupt handler can't observe the RXNE bit since the DMA has already cleared it, so it always wakes the waker. The waker previously checked if IDLE was set and returned NotReady otherwise but now we'd like it to check if the DMA buffer has received any data (indicating the interrupt was due to a received word). However, the second future is waiting for DMA half/full completion, so holds a borrow on the DMA buffer. Instead, we always return Ready from the UART future whenever the interrupt fires and eager is configured, and then afterwards check if the DMA buffer contains any data and recreate the futures if not.

Currently this isn't tested but I think it's feature-complete, I plan to do some testing in the next couple of days.

CC @kpfleming @DrTobe

[kpfleming]

I'm no expert here but the code looks correct to me (it matches the description of the intended changes). The one concern I have is that BufferedUartRx currently implements only eager mode and this PR would change its default to non-eager mode; this would be a pretty significant breaking change for its users. If there's a practical way to have the default set to match the current behavior that would resolve the issue.

Related: I really wish there was a way to 'push down' the "desired number of bytes" from the higher-level read operations, so that the UART drivers could return when that number of bytes have been read or idle is detected.

[adamgreig]

On the behaviour change: we discussed this a bit on matrix; I don't think it's technically a breaking change as the previous behaviour wasn't documented to be early-return or otherwise and either way you still get bytes through. More to the point I can't think of a way to have the config change for each type (and I'm not sure we would want to either!).

For asking for a certain number of bytes, I think we could fairly easily extend both Buffered and RingBuffered to do that in a later PR since this PR adds all the infrastructure to return after a certain number of bytes are found, but I'm not sure what the use-case is - if the remote end transmits continuously you presumably want to fill/half-fill the buffer, and if it has gaps between messages the idle detection should pick it up?

[kpfleming]

On the behaviour change: we discussed this a bit on matrix; I don't think it's technically a breaking change as the previous behaviour wasn't documented to be early-return or otherwise and either way you still get bytes through. More to the point I can't think of a way to have the config change for each type (and I'm not sure we would want to either!).

That's fair, although it's likely to end up being a Hyrum's Law-type breaking change instead of a documented breaking change. Anyone who has been expecting the future to be completed when the first byte arrives, because that's how it has been behaving, will be surprised when that no longer happens.

For asking for a certain number of bytes, I think we could fairly easily extend both Buffered and RingBuffered to do that in a later PR since this PR adds all the infrastructure to return after a certain number of bytes are found, but I'm not sure what the use-case is - if the remote end transmits continuously you presumably want to fill/half-fill the buffer, and if it has gaps between messages the idle detection should pick it up?

The protocol between my host and device has packets as small as 10 bytes and as large as 2060 bytes (approximately). This means the buffer has to be large enough for the largest possible packet, so waiting for the 50%/100% lines to be crossed will induce substantial latency when the host sends 3 or 4 10-20 byte packets in a row. Since I'd like to be able to respond to a packet as soon as it has arrived and not have to wait for the line to go idle, I'd prefer for the future to complete as soon as the minimum number of bytes for a packet have arrived - if those bytes are not a complete packet, then I'll wait for more (and I'll know how many to wait for as well).

[adamgreig]

Ah, good reasoning for wanting to just get a few bytes then. I realise I have some similar use cases, where e.g. you might know the first 6 bytes give you a header with a length, and then you could do a second read for the rest of the packet all quite efficiently. Does read_exact() work for you though?

[vDorst]

I make use of COBS which uses a NUL-byte as frame-delimiter. Uart supports sigle-byte/address matching, with can generate a interrupt when the nul-byte is detected. Accourding to the RM0481, it is called address mark detection.

Love to see that this also works with both IDLE and address mark detection enabled.

[kpfleming]

I make use of COBS which uses a NUL-byte as frame-delimiter. Uart supports sigle-byte/address matching, with can generate a interrupt when the nul-byte is detected. Accourding to the RM0481, it is called address mark detection.

In the G0 series 'address mark detection' is quite limited and only available when using MUTE mode (it's part of the 'multiprocessor communication' feature set).

[kpfleming]

Does read_exact() work for you though?

Originally I thought it might, but after looking through the driver code I don't think it would work because I don't think it can tolerate the line going idle (if the sender stops transmitting prematurely). I haven't actually tested that though.

[adamgreig]

OK, I've tested this and it behaves as expected. Ready for review/merge I think.

I've added two new commits though:

• Add new de_assertion_time and de_deassertion_time config options which are used when the DE pin is used. These turn out to be essential for many devices to communicate properly.
• Change BufferedUartRx behaviour so all bytes from the internal buffer are returned when possible, rather than returning partial results whenever the internal buffer has wrapped around its end. Combined with the new behaviour of waiting for line idle or buffer half full, this makes it possible to reliably receive whole idle-delimited packet(s) with a sufficiently large buffer.

I think it would be neat to extend this to support also stopping at arbitrary buffer sizes as @kpfleming said - BufferedUartRx could very easily do it with an extra check in the interrupt handle instead/alongside half_full, while RingBufferedUartRx would have to enable the RXNE interrupt to allow it to check (as it does now for eager_reads) but could then also perform the same check. However I don't need it and don't propose to do it right now, and I think it would extend this PR rather than replace it, so it could come later easily.

Similarly I think a character detect interrupt could be easily supported by adding a config option, setting ADD and enable CMIE, and then detecting CM in the interrupt handler and waking the Rx waker. It looks like for some families at least this works outside of MUTE mode (e.g. H7).

[adamgreig]

Originally I thought it might, but after looking through the driver code I don't think it would work because I don't think it can tolerate the line going idle (if the sender stops transmitting prematurely). I haven't actually tested that though.

You're right, it would hang waiting for bytes that won't arrive instead of detecting idle. I think this is better as a config option alongside/instead of eager_reads and it would affect all the read methods.

[adamgreig]

I updated the new eager_reads to use an Option<usize> which allows specifying the size of buffer you'd like (the previous behaviour was basically either None or Some(1), now you can have Some(10) to return after getting 10 bytes etc). I think this should address @kpfleming's use case (though note that using this with the RingBufferedUartRx is not especially efficient since it still has to have an interrupt fire for every received byte, even though the user code doesn't have to handle it and the DMA still does the actual data movement).

[adamgreig]

Rebased to main.

[adamgreig]

Rebased to main again. Any chance of getting this reviewed/merged?

[bugadani]

If "Some(0) is treated as None.", wouldn't this be better as NonZeroUsize?

[adamgreig]

It could be Option<NonZeroUsize>, which would make sense but is much more annoying to actually use and I don't think gives any meaningful benefit here.

[bugadani]

Ah okay I thought this is state, not configuration. I guess that makes you right :)

[Dirbaio]

🚀