Skip to content

rp2: Fix dropped UART REPL bytes on soft reset. #15914

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 26, 2024
Merged

rp2: Fix dropped UART REPL bytes on soft reset. #15914

merged 1 commit into from
Sep 26, 2024

Conversation

projectgus
Copy link
Contributor

@projectgus projectgus commented Sep 26, 2024 •
edited
Loading

Summary

Necessary to fix "mpremote run" over hardware UART (i.e. when MICROPY_HW_ENABLE_UART_REPL is set).

Bisect shows bug was introduced by d420b4e (#13718), but looks like made more complex by 01c046d (#14041). Specifically: resetting and re-initialising the REPL UART during soft reset clears the FIFO before it's done printing the "MPY: soft reboot" line.

Fixed by adding a UART TX flush in the deinit path.

Testing

  1. Build rp2 port with MICROPY_HW_ENABLE_UART_REPL set.
  2. Open interactive REPL and type Ctrl-D. Note the "MPY: soft reboot" line is not printed, only one invalid character is received.
  3. Alternatively, try to use "mpremote run" over the hardware UART. It times out waiting for the "soft reboot" line.

Fails without this PR, works with this PR.

Trade-offs and Alternatives

  • Alternative would be to not reset the REPL UART on soft reset. This may be better from the "principle of least surprise" point of view (i.e. if the user hasn't changed the UART settings then it makes no difference, and if they have changed them then having Soft Reboot change them and lose the REPL seems a bit unexpected.) However doing it that way has less clean layering, as the machine.UART code would need to know about the REPL configuration.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 26, 2024 •
edited
Loading

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +8 +0.001% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

dpgeorge
dpgeorge approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@dpgeorge dpgeorge left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. I verified the bug and that this PR fixes it.

It's true that the alternative would be to not reset the UARTs and leave them as they are for a soft reset. That's not too difficult, considering they already have static state. The only thing that really needs deiniting is their IRQ handlers because the Python callback is freed on soft reset.

Maybe that's a nice change, not to deinit UARTs, but it's a much bigger change (and stm32 for example does reset all UARTs on soft reset).

So let's go with this fix!

@dpgeorge
Copy link
Member

The only thing that really needs deiniting is their IRQ handlers because the Python callback is freed on soft reset.

Actually, no, the RX and TX buffers are only on the GC heap. So the UART really must be deinited on soft reset.

@projectgus
Copy link
Contributor Author

I guess I was thinking to still deinit the machine.UART objects (including the IRQs and the buffers) but leave the hardware UART peripheral setting alone if that UART is also used for REPL (i.e. the part implemented in rp2/uart.c which AFAIK doesn't interact with any of the machine module code.)

This still seems cleaner overall, though.

@robert-hh
Copy link
Contributor

The change looks good for me. For a moment I had doubts that using uart_tx_wait_blocking() was suitable, because is only looks at the FIFO. But as long as IRQ is enabled, the FIFO will eventually get refilled from the TX buffer.

@projectgus @dpgeorge
rp2/machine_uart: Fix dropped UART REPL bytes on soft reset.
73feaaf 
Necessary to fix "mpremote run" over hardware UART.

Bisect shows bug was introduced by d420b4e, but looks like made more
complex by 01c046d. Specifically: resetting and re-initialising the REPL
UART during soft reset clears the FIFO before it's done printing the "MPY:
soft reboot" line.

Fixed by adding a UART TX flush in the deinit path.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@dpgeorge dpgeorge force-pushed the bugfix/rp2_uart_soft_reset branch from 318b9bb to 73feaaf Compare September 26, 2024 13:43
@dpgeorge dpgeorge merged commit 73feaaf into micropython:master Sep 26, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgeorge dpgeorge dpgeorge approved these changes

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

Assignees
No one assigned
Labels
port-rp2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@projectgus @dpgeorge @robert-hh