Skip to content

samd: fix UART IRQ events#19215

Open
dpgeorge wants to merge 2 commits into
micropython:masterfrom
dpgeorge:samd-uart-fix-irq-events
Open

samd: fix UART IRQ events#19215
dpgeorge wants to merge 2 commits into
micropython:masterfrom
dpgeorge:samd-uart-fix-irq-events

Conversation

@dpgeorge
Copy link
Copy Markdown
Member

@dpgeorge dpgeorge commented May 13, 2026 •
edited
Loading

Summary

This PR fixes two related issues with UART Python IRQ events:

  • The UART DRE flag is always set if the DATA register is empty. That would lead to a TXIDLE event each time the IRQ handler was run. To fix that, only fire the TXIDLE event if the DRE interrupt flag is enabled.
  • The C IRQ handler can be called with multiple IRQ flags active, eg both RXC and DRE set. But the Python IRQ handler should only see the events that it registered. So mask the flags as appropriate.

Testing

tests/extmod/machine_uart_irq_txidle.py now passes on ADAFRUIT_ITSYBITSY_M0_EXPRESS

There are no regressions to other tests.

Generative AI

I did not use generative AI tools when creating this PR.

dpgeorge added 2 commits May 13, 2026 12:16
@dpgeorge
samd/machine_uart: Only fire the TXIDLE event once.
efa894b 
The UART DRE flag is always set if the DATA register is empty.  That would
lead to a TXIDLE event each time the IRQ handler was run.  To fix that,
only fire the TXIDLE event if the DRE interrupt flag is enabled.

Partially fixes the `tests/extmod/machine_uart_irq_txidle.py` test on
`ADAFRUIT_ITSYBITSY_M0_EXPRESS`.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
samd/machine_uart: Only pass registered events through to Python IRQ.
637c093 
The C IRQ handler can be called with multiple IRQ flags active, eg both
RXC and DRE set.  But the Python IRQ handler should only see the events
that it registered.  So mask the flags as appropriate.

Combined with the parent commit, `tests/extmod/machine_uart_irq_txidle.py`
now passes on `ADAFRUIT_ITSYBITSY_M0_EXPRESS`.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge requested a review from robert-hh May 13, 2026 02:34
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

Code size report:

Reference:  tests/ports/unix: Add gc_info_fast test. [936596c]
Comparison: samd/machine_uart: Only pass registered events through to Python IRQ. [merge of 637c093]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
      esp32:    +0 +0.000% ESP32_GENERIC
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +8 +0.003% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

robert-hh
robert-hh reviewed May 13, 2026
View reviewed changes
Comment thread ports/samd/machine_uart.c
// The handler for RXIDLE is called in the timer callback
if (self->mp_irq_trigger & mp_irq_flags) {
self->mp_irq_flags = mp_irq_flags;
self->mp_irq_flags = self->mp_irq_trigger & mp_irq_flags;
Copy link
Copy Markdown
Contributor

@robert-hh robert-hh May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good.

robert-hh
robert-hh reviewed May 13, 2026
View reviewed changes
Comment thread ports/samd/machine_uart.c
ringbuf_get(&self->write_buffer) | (ringbuf_get(&self->write_buffer) << 8);
}
} else {
} else if (uart->USART.INTENCLR.bit.DRE != 0) {
Copy link
Copy Markdown
Contributor

@robert-hh robert-hh May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that this bit is static and therefore the code path after line 160 is executed even if UART TX IRQ is not enabled. That causes the SERCOM_USART_INTFLAG_TXC to be set. However, with the masking in line 187 this has no effect.
If UART_TX IRQ is enabled, the code path below line 170 would be executed.
So this change seems to save executing 2 statements at the cost of an additional compare.

Copy link
Copy Markdown
Member Author

@dpgeorge dpgeorge May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change tests/extmod/machine_uart_irq_txidle.py would fail because two IRQ_TXIDLE events would be fired instead of just one.

Copy link
Copy Markdown
Contributor

@robert-hh robert-hh May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is reasonable. Otherwise the handler would be again at every RX IRQ event again. Verified that w/o the change the machine_uart_irq_txidle.py test fails.

Copy link
Copy Markdown
Member Author

@dpgeorge dpgeorge May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise the handler would be again at every RX IRQ event again.

Yes, exactly. If any IRQ comes in to this function and DATA is empty then DRE is set and so it previously would fire a TXIDLE interrupt to the Python handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@robert-hh robert-hh robert-hh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

port-samd

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dpgeorge @robert-hh