Async uart idle line detection

[t-moe]

I have some issues with the async uart implementation (on the main branch).

Expected behavior, after reading embedded-io-async::Read :

• If there is something in the uart FIFO, copy it to the buffer and return immediately
• If there is nothing in the uart FIFO, wait until at least one byte is available and then return

Actual behavior (https://github.com/esp-rs/esp-hal/blob/main/esp-hal-common/src/uart.rs#L1443-L1498)

• Regardless of the current contents of the FIFO, the method will not proceed until a UART interrupt has been triggered or has previously been triggered (FIFO full/overflow interrupt or at command interrupt)
• In contrast, to what the documentation of read_async says, the method will not error out if neither set_rx_fifo_full_threshold or set_at_cmd was called, instead it will just hang forever. (At least I cannot see code that would error out in this case).

I suggest that we add separate async methods, that can be used to wait on a FIFO full/overflow interrupt or at command interrupt, and that the embedded-io-async::Read implementation behaves as one expects when looking at the trait documentation (like in 02667e8).

[elpiel]

Yes, I can confirm that on ESP32-C3 I got unexpected behaviour when reading NMEA sentences sent via UART.
Bytes from 2 non-full messages were returned when doing single read with missing characters between the sentences.

[MabezDev]

This is a mixture of incorrect docs and missing functionality. For it to work the way docs say, we need to implement idle line detection so that when the uart stops receiving data an interrupt is fixed and we can return those bytes.

Yes, I can confirm that on ESP32-C3 I got unexpected behaviour when reading NMEA sentences sent via UART.
Bytes from 2 non-full messages were returned when doing single read with missing characters between the sentences.

I believe your issue is probably #882

[t-moe]

It may not be necessary to include idle line detection, as this isn't a requirement specified by embedded-io-async::Read.

The implementation presented in the commit at 02667e8 seems to align closely with the documentation for embedded-io-async::Read.

However, I concur that users may have varying expectations regarding when an asynchronous read operation should complete. For instance, they might expect it to finish when:

• There is any amount of data in the FIFO, as demonstrated in the commit mentioned.
• An AT command interrupt service routine (ISR) triggers.
• The FIFO becomes full, and an ISR triggers.
• The line becomes idle.
• Or potentially a mix of these conditions.

To address these diverse needs, I would suggest either offering a way to configure this behavior or providing different methods for asynchronous reads that complete upon specific events. I'm undecided on which approach is more in line with Rust's conventions. If you provide guidance on which variation you prefer, I'm happy to help.

[MabezDev]

It may not be necessary to include idle line detection, as this isn't a requirement specified by embedded-io-async::Read.

I agree it's not required, but it's something we want because it uses the hardware more efficiently than setting the FIFO threshold to 1 byte.

Or potentially a mix of these conditions.

The current approach is to allow a mix of these conditions, and I think for the default embedded-io-async::Read implementation it should stay that way. We can add specific methods for only using certain events later if we wish.

If you provide guidance on which variation you prefer, I'm happy to help.

I think adding support for idle line detection will be the most beneficial addition. That should hopefully be relatively straightforward to add; I'm happy to mentor adding it :). After that, we can take a look at possibly adding separate read methods for the different approaches.

[t-moe]

What kind of idle line detection to do have in mind? With a timer to check the line has been idle for a while? or is there some uart hw peripheral feature (I haven't read the entire technical reference doc yet)...

[MabezDev]

Idle line detection is built into the UART, take a look at the UART_IDLE_CONF_REG in the terms. UART has basically stayed the same across all chips, so doing it for one should port cleanly to all of them.

[t-moe]

Ok. This should indeed be straightforward to add. I'll try to do this in the coming weeks...