From 6d5135632443a7a4305eb6d05702209bd11e387c Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Sun, 19 Feb 2023 19:26:55 -0500 Subject: [PATCH 1/2] Fix pad assignments on atmel-samd UART --- ports/atmel-samd/common-hal/busio/UART.c | 135 +++++++++++++++-------- ports/mimxrt10xx/common-hal/busio/UART.c | 2 +- ports/stm/common-hal/busio/UART.c | 2 +- shared-bindings/busio/UART.c | 28 +++-- 4 files changed, 114 insertions(+), 53 deletions(-) diff --git a/ports/atmel-samd/common-hal/busio/UART.c b/ports/atmel-samd/common-hal/busio/UART.c index 6027b6a5fa..280e8d10b6 100644 --- a/ports/atmel-samd/common-hal/busio/UART.c +++ b/ports/atmel-samd/common-hal/busio/UART.c @@ -58,6 +58,8 @@ static void usart_async_rxc_callback(const struct usart_async_descriptor *const // Nothing needs to be done by us. } +// shared-bindings validates that the tx and rx are not both missing, +// and that the pins are distinct. void common_hal_busio_uart_construct(busio_uart_obj_t *self, const mcu_pin_obj_t *tx, const mcu_pin_obj_t *rx, const mcu_pin_obj_t *rts, const mcu_pin_obj_t *cts, @@ -92,10 +94,6 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, bool have_rts = rts != NULL; bool have_cts = cts != NULL; - if (!have_tx && !have_rx) { - mp_raise_ValueError(translate("tx and rx cannot both be None")); - } - if (have_rx && receiver_buffer_size > 0 && (receiver_buffer_size & (receiver_buffer_size - 1)) != 0) { mp_raise_ValueError_varg(translate("%q must be power of 2"), MP_QSTR_receiver_buffer_size); } @@ -107,6 +105,20 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, // This assignment is only here because the usart_async routines take a *const argument. struct usart_async_descriptor *const usart_desc_p = (struct usart_async_descriptor *const)&self->usart_desc; + // Allowed pads for USART. See the SAMD21 and SAMx5x datasheets. + // TXPO: + // (both) 0x0: TX pad 0; no RTS/CTS + // (SAMD21) 0x1: TX pad 2; no RTS/CTS + // (SAMx5x) 0x1: reserved + // (both) 0x2: TX pad 0; RTS: pad 2, CTS: pad 3 + // (SAMD21) 0x3: reserved + // (SAMx5x) 0x3: TX pad 0; RTS: pad 2; no CTS + // RXPO: + // 0x0: RX pad 0 + // 0x1: RX pad 1 + // 0x2: RX pad 2 + // 0x3: RX pad 3 + for (int i = 0; i < NUM_SERCOMS_PER_PIN; i++) { Sercom *potential_sercom = NULL; if (have_tx) { @@ -115,29 +127,71 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, continue; } potential_sercom = sercom_insts[sercom_index]; + + // SAMD21 and SAMx5x have different requirements. + #ifdef SAMD21 - if (potential_sercom->USART.CTRLA.bit.ENABLE != 0 || - !(tx->sercom[i].pad == 0 || - tx->sercom[i].pad == 2)) { + if (potential_sercom->USART.CTRLA.bit.ENABLE != 0) { + // In use. continue; } + if (tx->sercom[i].pad != 0 && + tx->sercom[i].pad != 2) { + // TX must be on pad 0 or 2. + continue; + } + if (have_rts) { + if (rts->sercom[i].pad != 2 || + tx->sercom[i].pad == 2) { + // RTS pin must be on pad 2, so if TX is also on pad 2, not possible + continue; + } + } + if (have_cts) { + if (cts->sercom[i].pad != 3 || + (have_rx && rx->sercom[i].pad == 3)) { + // CTS pin must be on pad 3, so if RX is also on pad 3, not possible + continue; + } + } #endif + #ifdef SAM_D5X_E5X - if (potential_sercom->USART.CTRLA.bit.ENABLE != 0 || - !(tx->sercom[i].pad == 0)) { + if (potential_sercom->USART.CTRLA.bit.ENABLE != 0) { + // In use. continue; } + if (tx->sercom[i].pad != 0) { + // TX must be on pad 0 + continue; + } + + if (have_rts && rts->sercom[i].pad != 2) { + // RTS pin must be on pad 2 + continue; + } + if (have_cts) { + if (cts->sercom[i].pad != 3 || + (have_rx && rx->sercom[i].pad == 3)) { + // CTS pin must be on pad 3, so if RX is also on pad 3, not possible + continue; + } + } #endif + tx_pinmux = PINMUX(tx->number, (i == 0) ? MUX_C : MUX_D); tx_pad = tx->sercom[i].pad; if (have_rts) { rts_pinmux = PINMUX(rts->number, (i == 0) ? MUX_C : MUX_D); } - if (rx == NULL) { + if (!have_rx) { + // TX only, so don't need to look further. sercom = potential_sercom; break; } } + + // Have TX, now look for RX match. We know have_rx is true at this point. for (int j = 0; j < NUM_SERCOMS_PER_PIN; j++) { if (((!have_tx && rx->sercom[j].index < SERCOM_INST_NUM && sercom_insts[rx->sercom[j].index]->USART.CTRLA.bit.ENABLE == 0) || @@ -160,20 +214,10 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, if (sercom == NULL) { raise_ValueError_invalid_pins(); } - if (!have_tx) { - tx_pad = 0; - if (rx_pad == 0) { - tx_pad = 2; - } - } - if (!have_rx) { - rx_pad = (tx_pad + 1) % 4; - } - // Set up clocks on SERCOM. samd_peripherals_sercom_clock_init(sercom, sercom_index); - if (rx && receiver_buffer_size > 0) { + if (have_rx && receiver_buffer_size > 0) { self->buffer_length = receiver_buffer_size; if (NULL != receiver_buffer) { self->buffer = receiver_buffer; @@ -204,36 +248,41 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, // which don't necessarily match what we need. After calling it, set the values // specific to this instantiation of UART. - // Set pads computed for this SERCOM. Refer to the datasheet for details on pads. - // TXPO: - // 0x0: TX pad 0; no RTS/CTS - // 0x1: reserved - // 0x2: TX pad 0; RTS: pad 2, CTS: pad 3 - // 0x3: TX pad 0; RTS: pad 2; no CTS - // RXPO: - // 0x0: RX pad 0 - // 0x1: RX pad 1 - // 0x2: RX pad 2 - // 0x3: RX pad 3 + // See the TXPO/RXPO table above for how RXPO and TXPO are chosen below. - // Default to TXPO with no RTS/CTS - uint8_t computed_txpo = 0; - // If we have both CTS (with or without RTS), use second pinout - if (have_cts) { - computed_txpo = 2; - } - // If we have RTS only, use the third pinout - if (have_rts && !have_cts) { - computed_txpo = 3; + // rxpo maps directly to rx_pad. + // Set to 0x0 if no RX, but it doesn't matter because RX will not be enabled. + const uint8_t rxpo = have_rx ? rx_pad : 0x0; + + #ifdef SAMD21 + // SAMD21 has only one txpo value when using either CTS or RTS or both. + // TX is on pad 0 or 2, or there is no TX. + // 0x0 for pad 0, 0x1 for pad 2. + uint8_t txpo; + if (tx_pad == 2) { + txpo = 0x1; + } else { + txpo = (have_cts || have_rts) ? 0x2 : 0x0; } + #endif + + #ifdef SAM_D5X_E5X + // SAMx5x has two different possibilities, per the chart above. + // We already know TX is on pad 0, or there is no TX. + + // Without RTS or CTS, txpo can be 0x0. + // It's not clear if 0x2 would cover all our cases, but this is known to be safe. + uint8_t txpo = (have_rts || have_cts) ? 0x2: 0x0; + #endif // Doing a group mask and set of the registers saves 60 bytes over setting the bitfields individually. sercom->USART.CTRLA.reg &= ~(SERCOM_USART_CTRLA_TXPO_Msk | SERCOM_USART_CTRLA_RXPO_Msk | SERCOM_USART_CTRLA_FORM_Msk); - sercom->USART.CTRLA.reg |= SERCOM_USART_CTRLA_TXPO(computed_txpo) | - SERCOM_USART_CTRLA_RXPO(rx_pad) | + // See chart above for TXPO values and RXPO values. + sercom->USART.CTRLA.reg |= SERCOM_USART_CTRLA_TXPO(txpo) | + SERCOM_USART_CTRLA_RXPO(rxpo) | (parity == BUSIO_UART_PARITY_NONE ? 0 : SERCOM_USART_CTRLA_FORM(1)); // Enable tx and/or rx based on whether the pins were specified. diff --git a/ports/mimxrt10xx/common-hal/busio/UART.c b/ports/mimxrt10xx/common-hal/busio/UART.c index 871d57648d..a61b86bee3 100644 --- a/ports/mimxrt10xx/common-hal/busio/UART.c +++ b/ports/mimxrt10xx/common-hal/busio/UART.c @@ -179,7 +179,7 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, break; } } else { - mp_raise_ValueError(translate("Supply at least one UART pin")); + mp_raise_ValueError(translate("tx and rx cannot both be None")); } if (rx && !rx_config) { diff --git a/ports/stm/common-hal/busio/UART.c b/ports/stm/common-hal/busio/UART.c index 48894db5f9..e8284713d1 100644 --- a/ports/stm/common-hal/busio/UART.c +++ b/ports/stm/common-hal/busio/UART.c @@ -160,7 +160,7 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, periph_index, uart_taken); } else { // both pins cannot be empty - mp_raise_ValueError(translate("Supply at least one UART pin")); + mp_raise_ValueError(translate("tx and rx cannot both be None")); } // Other errors diff --git a/shared-bindings/busio/UART.c b/shared-bindings/busio/UART.c index aada49a051..77d7036c2e 100644 --- a/shared-bindings/busio/UART.c +++ b/shared-bindings/busio/UART.c @@ -48,9 +48,13 @@ //| //| def __init__( //| self, -//| tx: microcontroller.Pin, -//| rx: microcontroller.Pin, +//| tx: Optional[microcontroller.Pin] = None, +//| rx: Optional[microcontroller.Pin] = None, //| *, +//| rts: Optional[microcontroller.Pin] = None, +//| cts: Optional[microcontroller.Pin] = None, +//| rs485_dir: Optional[microcontroller.Pin] = None, +//| rs485_invert: bool = False, //| baudrate: int = 9600, //| bits: int = 8, //| parity: Optional[Parity] = None, @@ -74,11 +78,13 @@ //| :param float timeout: the timeout in seconds to wait for the first character and between subsequent characters when reading. Raises ``ValueError`` if timeout >100 seconds. //| :param int receiver_buffer_size: the character length of the read buffer (0 to disable). (When a character is 9 bits the buffer will be 2 * receiver_buffer_size bytes.) //| +//| ``tx`` and ``rx`` cannot both be ``None``. +//| //| *New in CircuitPython 4.0:* ``timeout`` has incompatibly changed units from milliseconds to seconds. //| The new upper limit on ``timeout`` is meant to catch mistaken use of milliseconds. //| //| **Limitations:** RS485 is not supported on SAMD, nRF, Broadcom, Spresense, or STM. -//| On i.MX and Raspberry Pi RP2040 support is implemented in software: +//| On i.MX and Raspberry Pi RP2040, RS485 support is implemented in software: //| The timing for the ``rs485_dir`` pin signal is done on a best-effort basis, and may not meet //| RS485 specifications intermittently. //| """ @@ -118,11 +124,21 @@ STATIC mp_obj_t busio_uart_make_new(const mp_obj_type_t *type, size_t n_args, si const mcu_pin_obj_t *rx = validate_obj_is_free_pin_or_none(args[ARG_rx].u_obj, MP_QSTR_rx); const mcu_pin_obj_t *tx = validate_obj_is_free_pin_or_none(args[ARG_tx].u_obj, MP_QSTR_tx); - + const mcu_pin_obj_t *rts = validate_obj_is_free_pin_or_none(args[ARG_rts].u_obj, MP_QSTR_rts); + const mcu_pin_obj_t *cts = validate_obj_is_free_pin_or_none(args[ARG_cts].u_obj, MP_QSTR_cts); + const mcu_pin_obj_t *rs485_dir = validate_obj_is_free_pin_or_none(args[ARG_rs485_dir].u_obj, MP_QSTR_rs485_dir); if ((tx == NULL) && (rx == NULL)) { mp_raise_ValueError(translate("tx and rx cannot both be None")); } + // Pins must be distinct. + if ((tx != NULL && (tx == rx || tx == rts || tx == cts || tx == rs485_dir)) || + (rx != NULL && (rx == rts || rx == cts || rx == rs485_dir)) || + (rts != NULL && (rts == cts || rts == rs485_dir)) || + (cts != NULL && (cts == rs485_dir))) { + raise_ValueError_invalid_pins(); + } + uint8_t bits = (uint8_t)mp_arg_validate_int_range(args[ARG_bits].u_int, 5, 9, MP_QSTR_bits); busio_uart_parity_t parity = BUSIO_UART_PARITY_NONE; @@ -137,10 +153,6 @@ STATIC mp_obj_t busio_uart_make_new(const mp_obj_type_t *type, size_t n_args, si mp_float_t timeout = mp_obj_get_float(args[ARG_timeout].u_obj); validate_timeout(timeout); - const mcu_pin_obj_t *rts = validate_obj_is_free_pin_or_none(args[ARG_rts].u_obj, MP_QSTR_rts); - const mcu_pin_obj_t *cts = validate_obj_is_free_pin_or_none(args[ARG_cts].u_obj, MP_QSTR_cts); - const mcu_pin_obj_t *rs485_dir = validate_obj_is_free_pin_or_none(args[ARG_rs485_dir].u_obj, MP_QSTR_rs485_dir); - const bool rs485_invert = args[ARG_rs485_invert].u_bool; // Always initially allocate the UART object within the long-lived heap. From 2684aeb838ff75985d83083007e0e4131c100aab Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Mon, 20 Feb 2023 17:02:46 -0500 Subject: [PATCH 2/2] don't check for RX and TX both none in ports: now checked in shared-bindings --- locale/circuitpython.pot | 6 ------ ports/espressif/common-hal/busio/UART.c | 5 ++--- ports/mimxrt10xx/common-hal/busio/UART.c | 3 ++- ports/nrf/common-hal/busio/UART.c | 4 +--- ports/stm/common-hal/busio/UART.c | 6 +++--- 5 files changed, 8 insertions(+), 16 deletions(-) diff --git a/locale/circuitpython.pot b/locale/circuitpython.pot index 72ee60cc0c..24b5b541dc 100644 --- a/locale/circuitpython.pot +++ b/locale/circuitpython.pot @@ -1987,10 +1987,6 @@ msgstr "" msgid "Stopping AP is not supported." msgstr "" -#: ports/mimxrt10xx/common-hal/busio/UART.c ports/stm/common-hal/busio/UART.c -msgid "Supply at least one UART pin" -msgstr "" - #: shared-bindings/alarm/time/TimeAlarm.c msgid "Supply one of monotonic_time or epoch_time" msgstr "" @@ -4116,8 +4112,6 @@ msgstr "" msgid "twai_start returned esp-idf error #%d" msgstr "" -#: ports/atmel-samd/common-hal/busio/UART.c -#: ports/espressif/common-hal/busio/UART.c ports/nrf/common-hal/busio/UART.c #: shared-bindings/busio/UART.c shared-bindings/canio/CAN.c msgid "tx and rx cannot both be None" msgstr "" diff --git a/ports/espressif/common-hal/busio/UART.c b/ports/espressif/common-hal/busio/UART.c index f025c15b3e..c13bdedac0 100644 --- a/ports/espressif/common-hal/busio/UART.c +++ b/ports/espressif/common-hal/busio/UART.c @@ -112,9 +112,8 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, uart_config_t uart_config = {0}; bool have_rs485_dir = rs485_dir != NULL; - if (!have_tx && !have_rx) { - mp_raise_ValueError(translate("tx and rx cannot both be None")); - } + + // shared-bindings checks that TX and RX are not both None, so we don't need to check here. // Filter for sane settings for RS485 if (have_rs485_dir) { diff --git a/ports/mimxrt10xx/common-hal/busio/UART.c b/ports/mimxrt10xx/common-hal/busio/UART.c index a61b86bee3..086b8dee87 100644 --- a/ports/mimxrt10xx/common-hal/busio/UART.c +++ b/ports/mimxrt10xx/common-hal/busio/UART.c @@ -179,7 +179,8 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, break; } } else { - mp_raise_ValueError(translate("tx and rx cannot both be None")); + // TX and RX are both None. But this is already handled in shared-bindings, so + // we won't get here. } if (rx && !rx_config) { diff --git a/ports/nrf/common-hal/busio/UART.c b/ports/nrf/common-hal/busio/UART.c index df4a485999..10a34e09df 100644 --- a/ports/nrf/common-hal/busio/UART.c +++ b/ports/nrf/common-hal/busio/UART.c @@ -183,9 +183,7 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, mp_raise_ValueError(translate("All UART peripherals are in use")); } - if ((tx == NULL) && (rx == NULL)) { - mp_raise_ValueError(translate("tx and rx cannot both be None")); - } + // shared-bindings checks that TX and RX are not both None, so we don't need to check here. mp_arg_validate_int_min(receiver_buffer_size, 1, MP_QSTR_receiver_buffer_size); diff --git a/ports/stm/common-hal/busio/UART.c b/ports/stm/common-hal/busio/UART.c index e8284713d1..cdace31639 100644 --- a/ports/stm/common-hal/busio/UART.c +++ b/ports/stm/common-hal/busio/UART.c @@ -85,7 +85,7 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, bool sigint_enabled) { // match pins to UART objects - USART_TypeDef *USARTx; + USART_TypeDef *USARTx = NULL; uint8_t tx_len = MP_ARRAY_SIZE(mcu_uart_tx_list); uint8_t rx_len = MP_ARRAY_SIZE(mcu_uart_rx_list); @@ -159,8 +159,8 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, USARTx = assign_uart_or_throw(self, (self->tx != NULL), periph_index, uart_taken); } else { - // both pins cannot be empty - mp_raise_ValueError(translate("tx and rx cannot both be None")); + // TX and RX are both None. But this is already handled in shared-bindings, so + // we won't get here. } // Other errors