-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix uart busy handling #317
Conversation
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). The peripheral has valid data stored in it's RDR register but reports 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 Is that understanding correct? :) |
No problem, take your time.
Yes, your understanding is correct
It would be hard to poll at exactly that point in time, especially when data comes in back-to-back. 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? Thanks, |
Thanks for the clarification! :)
You can ignore that for now. This lint is unrelated (probably a new clippy lint introduced lately, and this job fails on any warning).
That would be great, thanks! |
@@ -881,9 +881,7 @@ where | |||
{ | |||
let isr = usart.isr.read(); | |||
|
|||
Err(if isr.busy().bit_is_set() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
No problem. |
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. |
@barafael I don't think, that my modification causes your problems. @Sh3Rm4n If you think my branch mentioned above is worth a PR, please let me know |
@jr-oss thanks for this, it's way better now with the changes you have mentioned. Still misses plenty of characters for me, though. |
@barafael Try to leave receive interrupt enabled, i.e. comment this line "serial.configure_interrupt(Event::ReceiveDataRegisterNotEmpty, Toggle::Off);" |
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 :) |
There could be a valid character available in rdr while the next character reception already started, e.g. with back-to-back characters..
3891020
to
ed99e34
Compare
ed99e34
to
414f20e
Compare
Thanks for your contribution! |
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