Fix uart busy handling

[jr-oss]

I tried a small test program on the Discovery board and suffered from receive overflows while polling the serial interface via "uart.read()".
Data rate is 19200 baud and polling interval is about 50-60us, so this shouldn't be possible.

I could fix my problem by the change in this PR

It looks like there is a chance to poll when some data is still in shift register and in the next poll cycle rdr got updated and the start-bit of the next character is already in the shift register.
This sets isr.busy and read() returns Error::WouldBlock although there is a valid character in rdr.

To test this I added a uart test case to the testsuite, which fails with the original implementation and passes with my fix.

Please let me know, if the PR is invalid, needs a change or if I missed something.
I can also squash the commits, if that fits better.

Thanks,
Ralf

[Sh3Rm4n]

Sorry for the delayed response. Thanks for the fix and especially providing a test-case with it. Much appreciated 🙏

I haven't looked too deeply into that issue as of know. (Pretty busy currently, hopefully I've some time this weekend for a proper review).
Just to restate the issue to confirm, that I it correctly.

The peripheral has valid data stored in it's RDR register but reports BUSY, because it currently receives data which is being stored in the shift register. In that scenario, the data in the RDR register can't be obtained (because the function returns WouldBlock)?

The alternative is to poll the function, to catch the point where the peripheral is not busy anymore busy and stores the SDR data inside the RDR register. But the unfortunate side effect is, that the old RDR data get's overwritten (which is probably confirmed by an Event::OverrunError).

Is that understanding correct? :)

[jr-oss]

Sorry for the delayed response. Thanks for the fix and especially providing a test-case with it. Much appreciated pray

No problem, take your time.
I hadn't expected an immediate response anyway during Easter weekend.
(Generally health and family/friends are more important for me than a project)

The peripheral has valid data stored in it's RDR register but reports BUSY, because it currently receives data which is being stored in the shift register. In that scenario, the data in the RDR register can't be obtained (because the function returns WouldBlock)?

Yes, your understanding is correct

The alternative is to poll the function, to catch the point where the peripheral is not busy anymore busy and stores the SDR data inside the RDR register. But the unfortunate side effect is, that the old RDR data get's overwritten (which is probably confirmed by an Event::OverrunError).

It would be hard to poll at exactly that point in time, especially when data comes in back-to-back.
I had a look at the implementation for stm32l4 and there the busy-bit is not queried in read(), which supports my assumption, that the busy bit should not be considered an error.
https://github.com/stm32-rs/stm32f4xx-hal/blob/70894b458a0323de44e48c53d64b109151ef5c0f/src/serial.rs#L839

My understanding of the busy bit is, that it helps detecting a busy line in a half-duplex transmission to avoid bus collisions.

I am a bit unsure about the failed Clippy check. Is there something that I missed?
And shall I add the link to this PR to the Changelog?

Thanks,
Ralf

[Sh3Rm4n]

Thanks for the clarification! :)

I am a bit unsure about the failed Clippy check. Is there something that I missed?

You can ignore that for now. This lint is unrelated (probably a new clippy lint introduced lately, and this job fails on any warning).

And shall I add the link to this PR to the Changelog?

That would be great, thanks!

[Sh3Rm4n]

Also, clarification for myself. This function would still return nb::Error::WouldBlock in case this bit is set / nothing is in the RDR register, because this case is handled by the else branch now.

[Sh3Rm4n]

Sorry for the long delay, I think this fix is valid. I'll rebase and merge until I get my hands on my hardware again :)

[jr-oss]

Sorry for the long delay,

No problem.
Thank you for maintaining the project.

[barafael]

I have tried these changes on an f3-discovery board using the RTIC example from the HAL. Using this example, with and without the changes, the transmission is unreliable (though in slightly different ways). Given the RTIC example code, failure means the serial receive task is no longer called due probably interrupt misconfiguration.

Without the changes, there is a chance any transmission fails, whether sending one character, or many. It seems more likely when sending many back-to-back.

With the changes, sending a single character always works (haven't found a single hangup). But, sending more than one, fails every single time.
Now, this could well be due to the RTIC example code not doing the right thing...

[jr-oss]

@barafael I don't think, that my modification causes your problems.
Can you check whether this helps: https://github.com/jr-oss/stm32f3xx-hal/tree/serial_echo_rtic_fixes
See commit messages and comments for explanations.
It does not prevent loosing characters, when there is an overrun conditions, but it should recover properly.
For further discussions, it might be better to open an issue because this is unrelated to this PR

@Sh3Rm4n If you think my branch mentioned above is worth a PR, please let me know
The last commit is just for my discovery board and would not be part of the PR

[barafael]

@jr-oss thanks for this, it's way better now with the changes you have mentioned. Still misses plenty of characters for me, though.

[jr-oss]

@barafael Try to leave receive interrupt enabled, i.e. comment this line "serial.configure_interrupt(Event::ReceiveDataRegisterNotEmpty, Toggle::Off);"
I guess the example plays with interrupt enable/disable for demonstration purposes.

[Sh3Rm4n]

Thanks for continuing working on that issue, I'll assure you to look at your fixes @jr-oss

In about 2 weeks, I should have my setup up and running again :)

[Sh3Rm4n]

Thanks for your contribution!