Skip to content

ports/stm32/uart: Add an option to disable UART RX pull-up. #15798

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

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

iabdalkader
Copy link
Contributor

@iabdalkader iabdalkader commented Sep 5, 2024

Summary

We're using an RS485 transceiver and UART fails to receive if this pull-up is enabled.

Testing

We've tested this new arg, and it works. It shouldn't have any side-effects as it's set to true by default, so old behavior is maintained by default.

Trade-offs and Alternatives

An alternative would be to add a compile-time config option (vs the runtime arg). This will result in less changes to the APIs, especially machine_uart. The changes will be in uart.c and will look like this:

diff --git a/ports/stm32/uart.c b/ports/stm32/uart.c
index 12d2e41c0..edcb26fb2 100644
--- a/ports/stm32/uart.c
+++ b/ports/stm32/uart.c
@@ -252,6 +252,8 @@ bool uart_init(machine_uart_obj_t *uart_obj,
 
     const machine_pin_obj_t *pins[4] = {0};
 
+    bool rx_pullup = true;
+
     switch (uart_obj->uart_id) {
         #if defined(MICROPY_HW_UART1_TX) && defined(MICROPY_HW_UART1_RX)
         case PYB_UART_1:
@@ -271,6 +273,9 @@ bool uart_init(machine_uart_obj_t *uart_obj,
             }
             #endif
             __HAL_RCC_USART1_CLK_ENABLE();
+            #if defined(MICROPY_HW_UART1_NO_RX_PULLUP)
+            rx_pullup = false
+            #endif
             break;
         #endif

However we need one for every UART/LPUART instance. It's fine with me either way.

@iabdalkader iabdalkader changed the title Uart rx pull ports/stm32/uart: Add an option to disable UART RX pull-up. Sep 5, 2024
Copy link

github-actions bot commented Sep 5, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:   +32 +0.008% 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
Copy link
Member

An alternative would be to add a compile-time config option

I think that's a slightly better way to do it. Because otherwise we have to agree on the changes to the API which may then be used by other ports. And probably it should default to no pull-ups, because that's what most (all?) other ports have.

And with a compile-time config option, the user doesn't have to worry about which UART needs the pull up enabled/disabled.

I'm assuming that your board itself has the RS485 transceiver installed on a fixed UART instance, and so a static configuration would suffice?

Also note that both RX and CTS are pulled high. CTS is more important to pull high than RX, otherwise transmission can get blocked if the other side doesn't do the right thing with CTS (it's RTS). Did you intend for this option to apply to both RX and CTS, or just RX?

@dpgeorge
Copy link
Member

Instead of using MICROPY_HW_UART1_NO_RX_PULLUP as the config option I suggest MICROPY_HW_UART1_RX_PULL (that matches MICROPY_HW_USRSW_PULL). Then if it's not defined it can default to GPIO_PULLUP.

@iabdalkader
Copy link
Contributor Author

And probably it should default to no pull-ups, because that's what most (all?) other ports have.

mimxrt seems to enable some default pull-ups on all lines (even TX and RTS). I'm not sure if any of them is needed though.

I'm assuming that your board itself has the RS485 transceiver installed on a fixed UART instance, and so a static configuration would suffice?

Yes exactly. This is for the OPTA's board which is currently being added upstream. So I think I should get this done first because it will require changes to the board config file.

Did you intend for this option to apply to both RX and CTS, or just RX?

We didn't test them separately, but without the pull-up on both modbus started working. Would you like to still use MICROPY_HW_UART1_RX_PULL for both ? or something like MICROPY_HW_UART1_NO_PULL ?

@iabdalkader iabdalkader force-pushed the uart_rx_pull branch 2 times, most recently from 38b0e0e to 1cad516 Compare September 19, 2024 07:40
@dpgeorge
Copy link
Member

Would you like to still use MICROPY_HW_UART1_RX_PULL for both ? or something like MICROPY_HW_UART1_NO_PULL ?

I think they need to be individually configurable... even if that does increase the configuration options by a lot.

I don't like negatives like "NO" in config options because it makes it harder to understand when it's in logic like !MICROPY_HW_UART1_NO_PULL. Much prefer to have positive options than negative options.

So, it would be MICROPY_HW_UARTx_RX_PULL and MICROPY_HW_UARTx_CTS_PULL, defaulting to GPIO_PULLUP. Unless you can think of a cleaner way to do it.

@iabdalkader
Copy link
Contributor Author

iabdalkader commented Sep 19, 2024

Looking at the original code again, it seems to be configuring a pull-up on all pins not just RX/CTS, am I right ? If so then MICROPY_HW_UARTx_PULL should be used.

    for (uint i = 0; i < 4; i++) {
        if (pins[i] != NULL) {
            // Configure pull-up on RX and CTS (the input pins).
            uint32_t pull = (i & 1) ? MP_HAL_PIN_PULL_UP : MP_HAL_PIN_PULL_NONE;
            bool ret = mp_hal_pin_config_alt(pins[i], mode, pull, uart_fn, uart_unit);
            if (!ret) {
                return false;
            }
        }
    }

EDIT ah I see the (i &1).

@iabdalkader
Copy link
Contributor Author

So, it would be MICROPY_HW_UARTx_RX_PULL and MICROPY_HW_UARTx_CTS_PULL, defaulting to GPIO_PULLUP. Unless you can think of a cleaner way to do it.

This will just complicate the logic above, not sure if fine-grained control over this is really needed: you either need the pull-ups or not, but I can try.

@iabdalkader
Copy link
Contributor Author

Unless you can think of a cleaner way to do it.

Okay, how about now with an array solution ?

@dpgeorge
Copy link
Member

Okay, how about now with an array solution ?

Yes, that looks good!

Does it work for the OPTA?

@iabdalkader
Copy link
Contributor Author

Does it work for the OPTA?

I'm confident it will, we just needed to disable the RX pull on UART3. I can't test it right now, but will ask someone else to test it after this and the Opta support are merged.

The UART driver enables a pull-up on RX/CTS pins by default.  This can
cause UART to fail to receive in certain situations, eg with RS485
transceivers.

This commit adds compile-time configuration options to set the pull mode on
the RX and CTS pins of each UART.

Signed-off-by: iabdalkader <i.abdalkader@gmail.com>
@dpgeorge dpgeorge merged commit eec5eb4 into micropython:master Sep 24, 2024
10 checks passed
@iabdalkader iabdalkader deleted the uart_rx_pull branch September 24, 2024 07:29
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