Skip to content

Conversation

@dpgeorge
Copy link
Member

Summary

This reverts commit c94a320.

The idea behind this reverted commit was that it allowed to reconfigure the UART to change only the baudrate, which is important in the context of a PPP connection where the baudrate may be changed as part of the protocol. Also, other ports like the rp2 port have this behaviour, where individual parameters of the UART can be changed with the .init() method.

But this commit was no good for a few reasons:

  1. It's a subtle breaking change to the UART API, because existing code that constructs or initialises a UART with just the baudrate would expect all other parameters to be reset to their defaults. But with this commit those parameters would remain unchanged.

  2. Constructing a UART like UART(1, 9600) also hits this code path of only changing the baudrate and does not reset other parameters, which is unexpected.

  3. It doesn't support setting the baudrate via keyword, eg UART.init(baudrate=9600).

  4. The timeout_char field is not updated when changing only the baudrate, which can lead to unexpected timeouts when reading/writing.

Due to point (4), this commit broke the tests/ports/stm32/uart.py test, the uart.writechar(1) has a timeout because the uart.init(2400) does not set the timeout_char for the new baudrate.

Points (2)-(4) could be fixed, but point (1) (being a breaking change) would remain as an issue. So the commit is reverted.

Testing

Prior to reverting, tests/ports/stm32/uart.py failed on PYBv1.0 and PYBD-SF2 (and most likely all other stm32 boards). Once reverted, this tests passes.

Trade-offs and Alternatives

The alternative is to fix points (2)-(4) above. But the API is going to be different and it's not a good idea to change that.

@github-actions
Copy link

Code size report:

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

@dpgeorge dpgeorge requested a review from projectgus October 21, 2024 01:17
Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

Good this got covered by a test, at least. Maybe one to add to a 2.0 planned changes list?

@dpgeorge
Copy link
Member Author

Maybe one to add to a 2.0 planned changes list?

Yeah... we definitely need a way to change the baudrate without reconfiguring everything else.

This reverts commit c94a320.

The idea behind this reverted commit was that it allowed to reconfigure the
UART to change only the baudrate, which is important in the context of a
PPP connection where the baudrate may be changed as part of the protocol.
Also, other ports like the rp2 port have this behaviour, where individual
parameters of the UART can be changed with the `.init()` method.

But this commit was no good for a few reasons:

1. It's a subtle breaking change to the UART API, because existing code
   that constructs or initialises a UART with just the baudrate would
   expect all other parameters to be reset to their defaults.  But with
   this commit those parameters would remain unchanged.

2. Constructing a UART like `UART(1, 9600)` also hits this code path of
   only changing the baudrate and does not reset other parameters, which is
   unexpected.

3. It doesn't support setting the baudrate via keyword, eg
   `UART.init(baudrate=9600)`.

4. The `timeout_char` field is not updated when changing only the baudrate,
   which can lead to unexpected timeouts when reading/writing.

Due to point (4), this commit broke the `tests/ports/stm32/uart.py` test,
the `uart.writechar(1)` has a timeout because the `uart.init(2400)` does
not set the `timeout_char` for the new baudrate.

Points (2)-(4) could be fixed, but point (1) (being a breaking change)
would remain as an issue.  So the commit is reverted.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge force-pushed the stm32-revert-uart-init-baudrate-only branch from 7293d3b to 7ed480f Compare October 22, 2024 00:50
@dpgeorge dpgeorge merged commit 7ed480f into micropython:master Oct 22, 2024
9 of 10 checks passed
@dpgeorge dpgeorge deleted the stm32-revert-uart-init-baudrate-only branch October 22, 2024 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants