fixed async read w/o at_cmd

[alexiionescu]

Found a bug when set_at_cmd was not used.
Somehow the at_cmd interrupt is triggered so often that breaks everything on embassy async.
To fix this I have added an additional parameter on read function to wait_at_cmd interrupt or not. If you have a better suggestion I can implement.

While at it, I have also implemented the RxFifo Overflow interrupt and error. It is important when using high speed baud rates to know if your processing is keeping up with the received stream.

Another issue I encounter on esp32 chipset. Using set_rx_fifo_full_threshold with 128 is allowed, but esp32 has only 7 bits for the value. Basically using 128(0x80) is working on esp32c3, but breaks everything on esp32 (triggers rx_fifo_full when 0 bytes are in rx fifo)
Not sure how to solve this at compile time, maybe some check at runtime?
I lost a day trying to figure out why the code working on esp32c3 does not work on esp32.

Must

• The code compiles without errors or warnings.
• All examples work.
• cargo fmt was run.
• Your changes were added to the CHANGELOG.md in the proper section.
• You updated existing examples or added examples (if applicable).
• Added examples are checked in CI

Nice to have

• You add a description of your work to this PR.
• You added proper docs for your newly added features and code.

[MabezDev]

Found a bug when set_at_cmd was not used.
Somehow the at_cmd interrupt is triggered so often that breaks everything on embassy async.
To fix this I have added an additional parameter on read function to wait_at_cmd interrupt or not. If you have a better suggestion I can implement.

I don't understand, if at_cmd isn't used then the interrupt isn't enabled, so how would it break things?

[alexiionescu]

Here is what I noticed:
Even if the interrupt is not triggered (put a panic in here intr_handler and never panicked), when poll is checking the bit flag in here event_bit_is_clear the bit is cleared and poll signal ready.
It makes no sense indeed. I will check again tomorrow to see if this is still true.

[Update]
Checked again today in 2 scenarios:

1. only with AT_CMD interrupt enabled

UartFuture::new(Event::RxCmdCharDetected, self.inner()).await;

AT_CMD interrupt is enabled.
The interrupt will never trigger (correct, the set_at_cmd was not called).
When poll the event_bit_is_clear returns false (correct)

2.AT_CMD and FIFO FULL

select(
    UartFuture::new(Event::RxCmdCharDetected, self.inner()),
    UartFuture::new(Event::RxFifoFull, self.inner()),
)
.await;

AT_CMD and FIFO FULL interrupt are enabled.
The interrupt will never trigger for AT_CMD intr_handler (correct, the set_at_cmd was not called).
When poll the event_bit_is_clear returns true (incorrect)

However it seems that after I have set correct the rx fifo full threshold for esp32, the issue was solved.
Now the only question remains: should we enable the AT_CMD interrupt even the set_at_cmd was not called? Seems to behave strangely when combined with other interrupts.
If you think it is OK I can close this PR.

[alexiionescu]

I have found the issue:
If you enable the interrupt AT_CMD and set_at_cmd is not called, at first interrupt handler call the interrupt enable bit for AT_CMD is cleared.
We should not enable the AT_CMD interrupt in read function (call UartFuture::new for Event::RxCmdCharDetected), if set_at_cmd is not called.

But the bool parameter for read might not be the best solution.
Another would be to save a boolean in the struct Uart in case set_at_cmd was called.

[MabezDev]

I have found the issue: If you enable the interrupt AT_CMD and set_at_cmd is not called, at first interrupt handler call the interrupt enable bit for AT_CMD is cleared. We should not enable the AT_CMD interrupt in read function (call UartFuture::new for Event::RxCmdCharDetected), if set_at_cmd is not called.

But the bool parameter for read might not be the best solution. Another would be to save a boolean in the struct Uart in case set_at_cmd was called.

I think saving Option<AtCmdConfig> inside Uart and then only awaiting the AT_CMD event if the config is some is the best option.

[alexiionescu]

I have found the issue: If you enable the interrupt AT_CMD and set_at_cmd is not called, at first interrupt handler call the interrupt enable bit for AT_CMD is cleared. We should not enable the AT_CMD interrupt in read function (call UartFuture::new for Event::RxCmdCharDetected), if set_at_cmd is not called.
But the bool parameter for read might not be the best solution. Another would be to save a boolean in the struct Uart in case set_at_cmd was called.

I think saving Option<AtCmdConfig> inside Uart and then only awaiting the AT_CMD event if the config is some is the best option.

Ok.
Should I implement when async feature is set only?
How about the set_rx_fifo_full_threshold and 7 bits max value on esp32. Should I add an issue about it?

[MabezDev]

Should I implement when async feature is set only?

Hmm not sure about that. @bjoernQ @jessebraham was there any reason we didn't store the at cmd config in the Uart originally?

How about the set_rx_fifo_full_threshold and 7 bits max value on esp32. Should I add an issue about it?

I think we just need a check on the esp32 to ensure the passed-in threshold does not exceed the 7bit max on the esp32.

[bjoernQ]

Hmm not sure about that. @bjoernQ @jessebraham was there any reason we didn't store the at cmd config in the Uart originally?

Don't think there really was a reason for that other than there wasn't a need for it back then

[alexiionescu]

How about the set_rx_fifo_full_threshold and 7 bits max value on esp32. Should I add an issue about it?

I think we just need a check on the esp32 to ensure the passed-in threshold does not exceed the 7bit max on the esp32.

Ok. I will also check the manuals for esp32c6, esp32h2 there is the same u8 cast for them in the code.
This function will return an error if the value is not compliant.
Casting to u8 is dangerous (0x100u16 as u8 is 0x00).

[alexiionescu]

How about the set_rx_fifo_full_threshold and 7 bits max value on esp32. Should I add an issue about it?

I think we just need a check on the esp32 to ensure the passed-in threshold does not exceed the 7bit max on the esp32.

Ok. I will also check the manuals for esp32c6, esp32h2 there is the same u8 cast for them in the code. This function will return an error if the value is not compliant. Casting to u8 is dangerous (0x100u16 as u8 is 0x00).

Thinking about it. What is someone does not call set_rx_fifo_full_threshold either.
Read should return error if no at_cmd and no rx_fifo_full is set.
We should make the same implementation as the at_cmd case for fifo_threshold also.

[MabezDev]

Sorry for the delay @alexiionescu, this dropped off my radar for a moment.

[MabezDev]

Thanks! This looks good to go, could you rebase on main to avoid the conflicts please?