stm32/uart: Add irq handler support for RXNE (character received). #4599

andrewleech · 2019-03-12T23:34:03Z

This extends the uart irq support from 372e7a4 / #3971 to allow running the irq handler for UART_FLAG_RXNE, ie for every character received.

dpgeorge · 2019-03-13T01:59:09Z

Just wondering, did you have a need for this? And did you find that you could respond to these IRQs fast enough (I guess it depends on the baudrate though)?

andrewleech · 2019-03-13T02:18:26Z

This isn't replacing the existing C interrupt handler, so it's still moving data into the rx buffer at full speed.

This RXNE interrupt just gives a higher update rate than the IDLE one which I'm using to pass incoming data on to a library for further processing.
I've also found sometimes I don't get an IDLE interrupt even though there's more data needing handling, probably due to new data coming in while the previous lot is being processed. Adding the RXNE interrupt has fixed that for me.

Actually thinking about it now, the problem may just be a race condition in the existing uart_irq_handler() between when it handles incoming data and clears interrupt flags. It should probably snapshot and clear all flags at the start of the function and work on the snapshot, rather than checking and clearing as it goes down.

Since pushing this I've found I also need a TX Complete interrupt for the same library, along with a modified uart_tx_data that doesn't block until complete.
I think I might be able to do it cleanly enough to be worth adding to this PR (a non-blocking write with tx complete interrupt), or I could do that in a separate PR?

dpgeorge · 2019-03-13T02:38:46Z

Thanks for the info.

Actually thinking about it now, the problem may just be a race condition in the existing uart_irq_handler()

Maybe. In any case, that should be fixed separately.

I think I might be able to do it cleanly enough to be worth adding to this PR (a non-blocking write with tx complete interrupt), or I could do that in a separate PR?

It sounds like the TXC IRQ is independent to the non-blocking write, true? If they are separate commits they can go in this same PR.

andrewleech · 2019-03-13T04:51:33Z

I've replaced the original commit with two separate ones.

Firstly, by moving the IDLE flag check to the top of the interrupt handler my unreliability in it has disappeared.
I tried to add a "snapshot and clear" of the interrupt status flags like I've done on other micros in the past, but it appears it's not that simple on stm32.

The second one, rather than just adding explicit support for RXNE, I've removed the restrictions entirely.
@tobbad Could you chime in here with the motivations for black/white-listing the interrupt types allowed for uart? If feels somewhat naive to just remove this code but it makes it all a lot less limiting, even if some interrupt flags will likely produce strange results.

tobbad · 2019-03-18T20:45:31Z

The aim of not touching the RXNE was, to not interfere with the REPL. In my opinion if you deactivate the RXNE (which will be allowed if you remove the black/white list) your interactive REPL is gone and there is no communication possible with the device any more. Further my impression is, that it is rather dangerous to allow the user to activate such high frequency IRQs which will come in at a rate which MP just can not handle.

For RX Idle: As you probably noticed, you only get that IRQ if you have a gap in your input data stream on the RX pin for at least one character. Therefore if you have an endless stream you would probably get a overwrite in the ring buffer and never an RX Idle IRQ - as RX is never idle.

For your application I would rather vote for a buffer half or 3/4 full/empty callback from the interrupt. The callback would be called from the IRQ context if the conditions are full filled and you'll have larger datablocks to be processed in MP.

For short: I think to process single chars (RXNE/TXNE IRQs) IRQ driven in MP is not that great and would stall the normal MP processing. Try to process larger blocks of data at lower frequency better suits - at least in my opinion - the "spirit" of Micropython.

andrewleech · 2019-03-18T21:00:52Z

@tobbad Thanks for the clarification.
In my particular application, I'm handling the interrupt callbacks in a c function using the recently-merged cmodules support, so the rate of interrupts is less of an issue in my case.

That being said, preventing access to single char interrupts just in case a user's application can't keep up is quite limiting. I'd prefer to perhaps document that it may be too slow to keep up, but it depends on the baud rate and the cpu speed (200Mhz stm32's can keep up with a lot even via the python vm).

Regarding turning RXNE off, I can't see where it's currently being switched on?
I know it's used in the low level interrupt handler, and I don't think my changes disable it... ? Could you point out where it's currently enabled?

Either way, with the re-ordering of how the user interrupt flags is set in the interrupt handler, my IDLE interrupt now seems to be working reliably without needing RXNE so my arguments there are purely academic.
Though, even with IDLE working, I probably do need a buffer full interrupt as well for handling large packets of data. I do have flow control enabled though, so chances are when the data is stopped due to full buffer, I probably get an idle interrupt anyway.

tobbad · 2019-03-18T21:47:46Z

In the top of uart.c there are some #defines which de-/activate the RXNE IRQ. These Macros are used in uart_set_rxbuf to activate the IRQ if a buffer is given to the MP UART constructor helper in uart_machine (Approx line# 312, source as today)

andrewleech · 2019-03-18T22:49:20Z

Ah, I see. Yes my changes wont have affected this, so RXNE will always be enabled by default. Even if it gets disabled, it's re-enabled in the interrupt handler if anything else triggers that and the hardware rxne flag is set.

dpgeorge · 2019-03-26T04:55:10Z

ports/stm32/uart.c

+    self->mp_irq_flags = self->uartx->SR;
+    if (self->uartx->SR & USART_SR_IDLE) {
+        (void)self->uartx->SR;
+        (void)self->uartx->DR;


I'm pretty sure this will break the RXNE handler below because it'll read the current incoming data (from DR) and clear the RXNE flag. Probably best to cache the SR and DR values here, or similar. It's a bit tricky, and is different on F4 vs other MCUs.

Do you know why DR was ever being read/cleared by this block?

The way I read it, if the rx buffer was already full (above) DR would not have been read into the buffer, but then this would clear / throw it away. Perhaps it's better to just clear SR and delete the DR line.

ok ignore that, the ref manual does list this as the only way to clean idle flag.

dpgeorge · 2019-03-26T04:57:04Z

In general having less restrictions on what the user can do, the better. In the case here, it still might be useful in some scenarios to have access to RXNE from Python. For low baudrates it would work fine, and for UARTs not attached to the REPL it wouldn't interfere with the REPL. So I think it's a good change to remove the restriction.

dpgeorge · 2019-05-06T05:58:27Z

@andrewleech I see you pushed some commits. Is this ready for re-review? But note, there is still a compile failure on Travis.

andrewleech · 2019-05-31T05:38:09Z

@dpgeorge I've finally fixed the build issues on F4 in the latest push.

I've run the change on my stm32F429 discovery board which has a uart repl to PC via the onboard debugger interface.
This still works with the change so it looks to me like the data register is being read correctly at least in basic operation.

I tried a flood test by sending a large amount of data in one go (3700 bytes) and it misses a lot of them (only ~3400 show up on screen, starts missing every few chars part way though the block).
A good number of tests before and after the change show pretty consistent amounts received irrespective of the change so I don't think it's made anything worse in that department.

dpgeorge · 2019-06-05T02:59:15Z

Thanks for updating. But I think there are still issues on the F4: since DR is read if IDLE is set it may be that a character is lost if the RX buffer is already full, or if RX buffering is disabled. This is because the DR value that is read at the start of the IRQ handler is only used if buffering is enabled and not full, otherwise it's discarded. I think the way around this would be to have a special 1-character buffer in the uart object to cache it. This would require a lot of testing to make sure it works, so I understand if you don't have time to do that (maybe I will get to it...).

Also, doesn't there need to be a new constant like UART.IRQ_RXNE?

andrewleech · 2019-09-25T06:39:09Z

it may be that a character is lost if the RX buffer is already full, or if RX buffering is disabled

That's something I'd missed, however I think that bug might exist in the current implementation already.

If the buffer is already full, int data = self->uartx->DR isn't run, with disable interrupt run instead. Irrespective of that though, the rest of the function is still run, ending with

    #if defined(STM32F4)
    if (self->uartx->SR & USART_SR_IDLE) {
        (void)self->uartx->SR;
        (void)self->uartx->DR;
        self->mp_irq_flags |= UART_FLAG_IDLE;
    }

which will read/clear the DR register regardless of whether it was copied into buffer earlier or not.

I think it would likely be better to never run (void)self->uartx->DR; in the USART_SR_IDLE clearing block, as if there's buffer room it will be read and cleared, if there's no buffer room then uart interrupt will be disabled so it wont fire anyway.

dpgeorge · 2019-11-27T04:24:45Z

@andrewleech what's the status of this, is it ready for re-review?

andrewleech · 2019-11-27T04:29:11Z

I believe so, though my previous comment still stands. I think there's an issue in the F4 implementation still that already exists in current code regarding the case where the buffer is already full.

andrewleech · 2020-01-30T02:31:39Z

ports/stm32/uart.c

-    #if defined(STM32F4)
-    if (self->uartx->SR & USART_SR_IDLE) {
-        (void)self->uartx->SR;
-        (void)self->uartx->DR;


I believe this is a bug in the existing implementation, which still occurs in my changes. In the case where the buffer is already full, the data in DR will not have been read above and will still be sitting there. That data will now be cleared/lost here if the IDLE flag is also set.

I guess the reality in any uart system is if the buffer isn't read fast enough data is being lost, so this is no different. It might be nice to detect this and set a flag the user can see though?

Other than this, I do believe this PR is good for final review, I've been using it unchanged for a long time now on our project.

You're right, it was broken before this PR! Eg if rxbuf=0 then there is no buffer and it is supposed to allow 1 character reads, but with irqs enabled all chars disappear.

dpgeorge · 2020-05-27T13:08:59Z

I think it would likely be better to never run (void)self->uartx->DR; in the USART_SR_IDLE clearing block, as if there's buffer room it will be read and cleared, if there's no buffer room then uart interrupt will be disabled so it wont fire anyway.

It looks like this change has now broken this PR completely on F4 MCUs.... Testing on a PYBv1.0 the IRQ_RXIDLE callback now locks up the device (even with a large rx buffer). This is because when the IDLE IRQ comes in this flag is never cleared, the SR register is read but DR is never read so it never clears the flag and the IRQ is called continuously.

dpgeorge · 2020-05-27T15:22:27Z

See #6082 for an attempt to try and fix the issues here in a different way.

…orage Turn storage back on for NeoPixel Trinkey

The idea is to try and make the IRQ handler more atomic in the operations: - get current IRQ status flags - deal with RX character - clear remaining status flags - call user handler On the STM32F4 it's very hard to get this right because the only way to clear IRQ status flags is to read SR then DR, but the read of DR may read some data which should remain in the register until the user wants to read it. And it won't work to cache the read because RTS/CTS flow control will then not work. So instead the code will disable interrupts if the DR is full and wait for the user to read it before reenabling the interrupts. Fixes issue mentioned in micropython#4599.

Prior to this commit IRQs on STM32F4 could be lost because SR is cleared by reading SR then reading DR. For example, if both RXNE and IDLE IRQs were active upon entry to the IRQ handler, then IDLE is lost because the code that handles RXNE comes first and accidentally clears SR (by reading SR then DR to get the incoming character). This commit fixes this problem by making the IRQ handler more atomic in the following operations: - get current IRQ status flags - deal with RX character - clear remaining status flags - call user handler On the STM32F4 it's very hard to get this right because the only way to clear IRQ status flags is to read SR then DR, but the read of DR may read some data which should remain in the register until the user wants to read it. And it won't work to cache the read because RTS/CTS flow control will then not work. So instead the new code disables interrupts if the DR is full and waits for the user to read it before reenabling the interrupts. Fixes issue mentioned in micropython#4599 and micropython#6082.

Prior to this commit IRQs on STM32F4 could be lost because SR is cleared by reading SR then reading DR. For example, if both RXNE and IDLE IRQs were active upon entry to the IRQ handler, then IDLE is lost because the code that handles RXNE comes first and accidentally clears SR (by reading SR then DR to get the incoming character). This commit fixes this problem by making the IRQ handler more atomic in the following operations: - get current IRQ status flags - deal with RX character - clear remaining status flags - call user handler On the STM32F4 it's very hard to get this right because the only way to clear IRQ status flags is to read SR then DR, but the read of DR may read some data which should remain in the register until the user wants to read it. And it won't work to cache the read because RTS/CTS flow control will then not work. So instead the new code disables interrupts if the DR is full and waits for the user to read it before reenabling the interrupts. Fixes issue mentioned in micropython#4599 and micropython#6082. Signed-off-by: Damien George <damien@micropython.org>

dpgeorge · 2021-10-28T02:28:18Z

Missing IDLE IRQ is fixed by 83827e8

Prior to this commit IRQs on STM32F4 could be lost because SR is cleared by reading SR then reading DR. For example, if both RXNE and IDLE IRQs were active upon entry to the IRQ handler, then IDLE is lost because the code that handles RXNE comes first and accidentally clears SR (by reading SR then DR to get the incoming character). This commit fixes this problem by making the IRQ handler more atomic in the following operations: - get current IRQ status flags - deal with RX character - clear remaining status flags - call user handler On the STM32F4 it's very hard to get this right because the only way to clear IRQ status flags is to read SR then DR, but the read of DR may read some data which should remain in the register until the user wants to read it. And it won't work to cache the read because RTS/CTS flow control will then not work. So instead the new code disables interrupts if the DR is full and waits for the user to read it before reenabling the interrupts. Fixes issue mentioned in micropython#4599 and micropython#6082. Signed-off-by: Damien George <damien@micropython.org>

pi-anl force-pushed the stm32_uart_irq_rxne branch from b762087 to e80f077 Compare March 13, 2019 04:40

dpgeorge reviewed Mar 26, 2019

View reviewed changes

pi-anl force-pushed the stm32_uart_irq_rxne branch from e80f077 to 42e548c Compare April 29, 2019 01:28

pi-anl force-pushed the stm32_uart_irq_rxne branch 2 times, most recently from 921374d to 5620c33 Compare May 31, 2019 05:12

dpgeorge added the port-stm32 label Jun 5, 2019

pi-anl force-pushed the stm32_uart_irq_rxne branch 2 times, most recently from 60a5c70 to 5b13788 Compare October 1, 2019 04:24

pi-anl force-pushed the stm32_uart_irq_rxne branch from 5b13788 to c8e20dd Compare November 27, 2019 01:39

andrewleech commented Jan 30, 2020

View reviewed changes

pi-anl force-pushed the stm32_uart_irq_rxne branch from c8e20dd to 2e4bab9 Compare January 30, 2020 02:34

pi-anl force-pushed the stm32_uart_irq_rxne branch from 2e4bab9 to 2385d25 Compare March 11, 2020 23:24

pi-anl force-pushed the stm32_uart_irq_rxne branch from 2385d25 to 31bd679 Compare March 26, 2020 07:11

andrewleech force-pushed the stm32_uart_irq_rxne branch 2 times, most recently from 1a3e577 to 9c31627 Compare May 4, 2020 10:21

andrewleech force-pushed the stm32_uart_irq_rxne branch from 9c31627 to de3673c Compare May 12, 2020 00:33

pi-anl added 2 commits May 27, 2020 16:41

stm32/uart: Fix interrupt handler race condition on IDLE flag.

fbcf8d9

stm32/uart: Remove trigger restrictions on uart irq

3502983

andrewleech force-pushed the stm32_uart_irq_rxne branch from de3673c to 3502983 Compare May 27, 2020 06:53

dpgeorge mentioned this pull request May 27, 2020

stm32/uart: Fix race conditions and clearing status in IRQ handler #6082

Merged

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Apr 13, 2021

Merge pull request micropython#4599 from dhalbert/neopixel-trinkey-st…

9a3f04a

…orage Turn storage back on for NeoPixel Trinkey

dpgeorge closed this Oct 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stm32/uart: Add irq handler support for RXNE (character received). #4599

stm32/uart: Add irq handler support for RXNE (character received). #4599

andrewleech commented Mar 12, 2019

dpgeorge commented Mar 13, 2019

andrewleech commented Mar 13, 2019

dpgeorge commented Mar 13, 2019

andrewleech commented Mar 13, 2019

tobbad commented Mar 18, 2019 •

edited

andrewleech commented Mar 18, 2019 •

edited

tobbad commented Mar 18, 2019

andrewleech commented Mar 18, 2019

dpgeorge Mar 26, 2019

andrewleech Apr 29, 2019

andrewleech Apr 29, 2019

dpgeorge commented Mar 26, 2019

dpgeorge commented May 6, 2019

andrewleech commented May 31, 2019

dpgeorge commented Jun 5, 2019

andrewleech commented Sep 25, 2019 •

edited

dpgeorge commented Nov 27, 2019

andrewleech commented Nov 27, 2019

andrewleech Jan 30, 2020

dpgeorge May 27, 2020

dpgeorge commented May 27, 2020

dpgeorge commented May 27, 2020

dpgeorge commented Oct 28, 2021

stm32/uart: Add irq handler support for RXNE (character received). #4599

stm32/uart: Add irq handler support for RXNE (character received). #4599

Conversation

andrewleech commented Mar 12, 2019

dpgeorge commented Mar 13, 2019

andrewleech commented Mar 13, 2019

dpgeorge commented Mar 13, 2019

andrewleech commented Mar 13, 2019

tobbad commented Mar 18, 2019 • edited

andrewleech commented Mar 18, 2019 • edited

tobbad commented Mar 18, 2019

andrewleech commented Mar 18, 2019

dpgeorge Mar 26, 2019

Choose a reason for hiding this comment

andrewleech Apr 29, 2019

Choose a reason for hiding this comment

andrewleech Apr 29, 2019

Choose a reason for hiding this comment

dpgeorge commented Mar 26, 2019

dpgeorge commented May 6, 2019

andrewleech commented May 31, 2019

dpgeorge commented Jun 5, 2019

andrewleech commented Sep 25, 2019 • edited

dpgeorge commented Nov 27, 2019

andrewleech commented Nov 27, 2019

andrewleech Jan 30, 2020

Choose a reason for hiding this comment

dpgeorge May 27, 2020

Choose a reason for hiding this comment

dpgeorge commented May 27, 2020

dpgeorge commented May 27, 2020

dpgeorge commented Oct 28, 2021

tobbad commented Mar 18, 2019 •

edited

andrewleech commented Mar 18, 2019 •

edited

andrewleech commented Sep 25, 2019 •

edited