Pulling Serial1 RX low causes Artemis to hang with v2.1 of the core

[PaulZC]

Hi gang,

Another interesting Apollo3 v2.1 quirk:

Pulling the Serial1 RX pin low causes the Artemis to hang with v2.1 of the core...

Steps to reproduce:

Redboard Artemis ATP
Run the following sketch
Use a jumper wire to pull the RX1 pin to GND
With v2.1.0 and v2.1.1 of the core, the Artemis hangs permanently - even when the jumper wire is removed
With v1.2.1, the code keeps on running

I haven't yet figured out why this happens, or exactly where the code hangs. I'm hoping @paulvha may have some insight on this (thank you in advance - as always).

I know that holding a serial RX pin low is 'illegal' (manifesting as a continuous start bit?) but folks need to know about this just in case the thing they have attached to the RX pin is (e.g.) powered down and pulls the pin low.

I'm kicking myself, because I actually noticed this a few weeks ago when I was upgrading OpenLog Artemis. I noticed that Serial1 RX would hang after deep sleep if nothing was connected to the RX pin. I eventually figured out that the (weak?) pull-up on the RX pin is removed after deep sleep. If the pin then floats low, the code hangs. I worked around this at the time by manually re-enabling the pull-up on the RX pin after deep sleep. I guess this might explain some of the behavior seen in #411?

Best wishes,
Paul (ZC)

// RedBoard Artemis ATP - Serial1 Rx test
//
// Let the code run for a few seconds,
// then use a jumper wire to pull Serial1 RX (RX1) low
//
// With v2.1.0 & v2.1.1 of Apollo3, pulling RX1 low causes the Artemis to hang...
// With v1.2.1, the Artemis keeps on running...

void setup() {
  Serial.begin(115200);
  Serial1.begin(115200);
}

void loop() {
  Serial.print(F("Serial1.available is "));
  Serial.println(Serial1.available());
  delay(500);
}

[PaulZC]

Running the following sketch shows that holding the RX1 pin low for ~9.5 bit periods causes the hang. I.e. half way through the stop bit. (1 start bit, eight data bits, no parity, 1 stop bit). So I guess there must be some subtle difference in the way the UART is configured in v2.1 compared with v1.2?

// RedBoard Artemis ATP - Serial1 Rx test
//
// Use a jumper wire to link RX1 to pin 14

int delayBy = 900;

void setup() {
  pinMode(14, OUTPUT);
  digitalWrite(14, HIGH);
  
  Serial.begin(115200);
  
  Serial1.begin(9600);
}

void loop() {
  delayBy++;
  
  digitalWrite(14, LOW);
  delayMicroseconds(delayBy);
  digitalWrite(14, HIGH);
  
  delay(50);
  
  Serial.print(F("delayBy is "));
  Serial.print(delayBy);
  Serial.print(F("  Serial1.available is "));
  Serial.println(Serial1.available());
  Serial.flush();
}

At 9600 baud, the code hangs when delayBy reaches 983 microseconds = 9.4 bits:

At 4800 baud and starting with delayBy = 1900, the code hangs when delayBy reaches 1981 microseconds = 9.5 bits:

[paulvha]

Looked at this issue before, it is described + solution in this issue : #349

regards,
Paulvha

[PaulZC]

Thank you Paul (@paulvha ),

I think this issue might be the cause of problems we've been seeing on the Artemis Global Tracker. I upgraded the firmware to use v2.1.0 of the core recently and some customers have reported the new code hanging. We have the Iridium modem connected to Serial1. Power for the modem is switched by a separate circuit controlled by a GPIO pin. I think the modem may be pulling the RX pin low briefly as it powers up causing the code to hang. I suspect that adding a pull-up resistor in this case may not work but I will give it a try!

Thanks again,
Paul

[PaulZC]

Some progress... Disabling the RX pin seems to avoid the issue.

This sketch keeps running on v2.1.0:

// RedBoard Artemis ATP - Serial1 Rx test
//
// Use a jumper wire to link RX1 to pin 14

void setup() {
  pinMode(14, OUTPUT);
  digitalWrite(14, HIGH);
  
  Serial.begin(115200);
}

void loop() {
  configureSerial1TxRx(D24, AM_HAL_PIN_24_UART1TX, D25, AM_HAL_PIN_25_UART1RX); // Select the standard ATP pins for UART1 TX and RX
  Serial1.begin(115200);

  delay(100);
  
  Serial.print(F("Serial1.available is "));
  Serial.println(Serial1.available());
  Serial.flush();

  am_hal_gpio_pinconfig(PinName(D25), g_AM_HAL_GPIO_DISABLE); // Disable the RX pin
  
  delay(100);

  digitalWrite(14, LOW);

  delay(100);

  digitalWrite(14, HIGH);

  delay(100);
}

void configureSerial1TxRx(PinName txPin, uint8_t txPinFuncSel, PinName rxPin, uint8_t rxPinFuncSel) // Configure pins for UART1 TX and RX
{
  am_hal_gpio_pincfg_t pinConfigTx = g_AM_BSP_GPIO_COM_UART_TX;
  pinConfigTx.uFuncSel = txPinFuncSel;
  pin_config(txPin, pinConfigTx);
  
  am_hal_gpio_pincfg_t pinConfigRx = g_AM_BSP_GPIO_COM_UART_RX;
  pinConfigRx.uFuncSel = rxPinFuncSel;
  pinConfigRx.ePullup = AM_HAL_GPIO_PIN_PULLUP_WEAK; // Put a weak pull-up on the Rx pin
  pin_config(rxPin, pinConfigRx);
}

[PaulZC]

I think we're in business!

This sketch:

// RedBoard Artemis ATP - Serial1 Rx test
//
// Use a jumper wire to link RX1 to pin 14

void setup() {
  pinMode(14, OUTPUT);
  digitalWrite(14, HIGH);
  
  Serial.begin(115200);
  Serial1.begin(100);
}

void loop() {
  configureSerial1TxRx(D24, AM_HAL_PIN_24_UART1TX, D25, AM_HAL_PIN_25_UART1RX); // Select the standard ATP pins for UART1 TX and RX

  delay(100);

  // Just for fun, bit-bang the ASCII character "0" (0x30, LSB-first = 0b00001100) on pin 14
  digitalWrite(14, LOW); delay(10); // Start bit. Bit period at 100 baud is 10 milliseconds
  digitalWrite(14, LOW); delay(40); // Four bits
  digitalWrite(14, HIGH); delay(20); // Two bits
  digitalWrite(14, LOW); delay(20); // Two bits
  digitalWrite(14, HIGH); delay(10); // Stop bit. Bit period at 100 baud is 10 milliseconds

  delay(100);
  
  Serial.print(F("Serial1.available is "));
  Serial.print(Serial1.available());
  Serial.print(F("   Serial1.read returned \""));
  Serial.write(Serial1.read());
  Serial.println(F("\""));
  Serial.flush();

  am_hal_gpio_pinconfig(PinName(D25), g_AM_HAL_GPIO_DISABLE); // Disable the RX pin
  
  delay(100);

  digitalWrite(14, LOW);

  delay(2000); // Wait >> 9.5 bit periods

  digitalWrite(14, HIGH);

  delay(100);
}

void configureSerial1TxRx(PinName txPin, uint8_t txPinFuncSel, PinName rxPin, uint8_t rxPinFuncSel) // Configure pins for UART1 TX and RX
{
  am_hal_gpio_pincfg_t pinConfigTx = g_AM_BSP_GPIO_COM_UART_TX;
  pinConfigTx.uFuncSel = txPinFuncSel;
  pin_config(txPin, pinConfigTx);
  
  am_hal_gpio_pincfg_t pinConfigRx = g_AM_BSP_GPIO_COM_UART_RX;
  pinConfigRx.uFuncSel = rxPinFuncSel;
  pinConfigRx.ePullup = AM_HAL_GPIO_PIN_PULLUP_WEAK; // Put a weak pull-up on the Rx pin
  pin_config(rxPin, pinConfigRx);
}

produces:

[PaulZC]

Could this be the explanation?

In am_hal_uart.c buffer_configure both the RX and RX_TMOUT interrupts are enabled:

https://github.com/sparkfun/mbed-os-ambiq-apollo3/blob/90230bb74485d774d5dba5550297d2804724fb7e/targets/TARGET_Ambiq_Micro/TARGET_Apollo3/sdk/mcu/apollo3/hal/am_hal_uart.c#L592-L593

But in serial_api.c serial_irq_set only the RX interrupt is enabled:

https://github.com/sparkfun/mbed-os-ambiq-apollo3/blob/90230bb74485d774d5dba5550297d2804724fb7e/targets/TARGET_Ambiq_Micro/TARGET_Apollo3/device/serial_api.c#L229

If pulling the RX pin low causes an RX_TMOUT, does this mean the UART will hang because the interrupt is not enabled and not being serviced?

I suspect we need to change line 229:

MBED_ASSERT(am_hal_uart_interrupt_enable(obj->serial.uart_control->handle, AM_HAL_UART_INT_RX) == AM_HAL_STATUS_SUCCESS);

to:

MBED_ASSERT(am_hal_uart_interrupt_enable(obj->serial.uart_control->handle, AM_HAL_UART_INT_RX | AM_HAL_UART_INT_RX_TMOUT) == AM_HAL_STATUS_SUCCESS);

And in line 242, change:

MBED_ASSERT(am_hal_uart_interrupt_disable(obj->serial.uart_control->handle, AM_HAL_UART_INT_RX) == AM_HAL_STATUS_SUCCESS);

to:

MBED_ASSERT(am_hal_uart_interrupt_disable(obj->serial.uart_control->handle, AM_HAL_UART_INT_RX | AM_HAL_UART_INT_RX_TMOUT) == AM_HAL_STATUS_SUCCESS);

Thoughts please...

[Wenn0101]

@PaulZC I'll be back working on Monday, I'll take a look first thing!

[paulvha]

Same here...

[PaulZC]

Thank you both! No rush...
Very best wishes,
Paul

[Wenn0101]

Alright. Here is what I found so far.

It looks like when the RX line is going low we are hanging in the UART::rxISR.

    char c;
    while(UnbufferedSerial::readable()) {
        UnbufferedSerial::read(&c, 1);
        _rxbuf.store_char(c);
    }

More specifically the UnbufferedSerial::readable() is returning true (erroneously), and we are getting stuck in UnbufferedSerial::read(&c, 1);. The read function is a blocking function that does not exit until we have read atleast 1 byte, since the line is just being held low there is no data to read and we just sit there.

Digging into the mbed driver the read is utilizing the serial_getc implementation. which is hanging in this loop:

 do {
    am_hal_uart_transfer(obj->serial.uart_control->handle, &am_hal_uart_xfer_read_single);
  } while (bytes_read == 0);

We don't have much control over this behavior since we can see above:

 * # Defined behavior:
 * ...
 * serial_getc is a blocking call (waits for the character).

The problem really seems to be that the call before this ( UnbufferedSerial::readable() which thru mbed will arrive at our driver implementation of serial_readable) is returning true.
This code is:

int serial_readable(serial_t *obj) {
  MBED_ASSERT(obj->serial.uart_control != NULL);
  return !(UARTn(obj->serial.uart_control->inst)->FR_b.RXFE);
}

So RXFE (according to the datasheet "This bit holds the receive FIFO empty indicator") is changing to false. indicating that the FIFO of the hardware serial is not empty... then we are attempting to read the FIFO, finding it empty and waiting for it to not be empty...

I have confirmed this is the problem by commenting out the body of the UART::rxISR and then running this sketch.

void setup() {
  Serial.begin(115200);
  Serial1.begin(115200);
}

void loop() {
  Serial.print("Uart 0 RXFE ");
  Serial.println(UARTn(0)->FR_b.RXFE);
  Serial.print("Uart 1 RXFE ");
  Serial.println(UARTn(1)->FR_b.RXFE);
  delay(500);
}

When I jump RX to ground the RXFE register swaps to 0.

So I have found what I believe to be the root cause and now I'm working on figuring out a solution.

[PaulZC]

Reading between the lines here, I think the AM_HAL_UART_INT_RX_TMOUT interrupt must enabled because the ISR is being called after the RX pin has been pulled low for ~9.5 bit periods (see above). When we hold the pin low beyond the 9th bit period (1 start bit, 8 data bits) we go into an 'illegal' state, the RX line should be high (1 stop bit) but it isn't and I'm guessing an RX_TMOUT interrupt is generated.

Again, reading between the lines, I'm guessing that v1.2 was OK with the RX pin being pulled low because the ISR checked the AM_HAL_UART_INT_RX interrupt flag was set before performing the am_hal_uart_transfer. If the AM_HAL_UART_INT_RX interrupt flag was not set (but presumably AM_HAL_UART_INT_RX_TMOUT was?) then all was well as the interrupt was still being cleared and serviced, and am_hal_uart_transfer was not called:

Arduino_Apollo3/cores/arduino/ard_sup/uart/ap3_uart.cpp

Line 553 in 87a33ce

if (ui32Status & AM_HAL_UART_INT_RX)

The code never got as far as calling uart_fifo_read where the RXFE flag was checked:

Arduino_Apollo3/cores/arduino/am_sdk_ap3/mcu/apollo3/hal/am_hal_uart.c

Line 632 in 87a33ce

if ( UARTn(ui32Module)->FR_b.RXFE )

Can we change serial_readable so it checks the RX_TMOUT flag and/or the RX flag before checking RXFE?

As a nasty thought experiment, what would happen if a valid serial byte was received (with a proper stop bit) followed immediately by the RX pin being pulled low for 10+ bits? Maybe, if that happens, whatever is in the FIFO should be discarded?

Thanks for digging into this...!

[paulvha]

I have extended your sketch to include reading the DR-status bits. Turns out an error has occurred ( parity, framing, overrun or break) when connecting a wire to GND.

  Serial.print("Uart 1 DR- FEDATA ");
  Serial.println(UARTn(1)->DR_b.FEDATA);
  Serial.print("Uart 1 DR- PEDATA ");
  Serial.println(UARTn(1)->DR_b.PEDATA);
  Serial.print("Uart 1 DR- BEDATA ");
  Serial.println(UARTn(1)->DR_b.BEDATA);
  Serial.print("Uart 1 DR- OEDATA ");
  Serial.println(UARTn(1)->DR_b.OEDATA);

I had included the serial.print() in UART: rxISR and in case of an error when we try to read the character it will "hang".

The reason is that thru MBED, Serial_getc(in Serial_API.c), am_hal_uart_transfer (in am_hal_uart.c), read_timeout(in am_hal_uart.c), read_nonblocking (in am_hal_uart.c) we end up in uart_fifo_read ((in am_hal_uart.c). Here it performs a check on the error bits and if detected it will return an error AND will NOT increase the number of bytes read. The returned error code is neglected in Serial_getc it will only check that data has been received :

  do {
    am_hal_uart_transfer(obj->serial.uart_control->handle, &am_hal_uart_xfer_read_single);
  } while (bytes_read == 0);

Now you got your "hang".

Unfortunately, the error code can NOT be passed back thru MBED, only the read character is passed back. Passing back a 0x00 or 0xFF can not be done as that might be valid data.

Potential solution
Hence we should extend the while loop in UART:rxISR to something like

    while(UnbufferedSerial::readable()) {

        if (UARTn(ui32Module)->DR_b.FEDATA | UARTn(ui32Module)->DR_b.PEDATA |
            UARTn(ui32Module)->DR_b.BEDATA | UARTn(ui32Module)->DR_b.OEDATA)
        {
            // ignore the character in error
            UARTn(ui32Module)->DR);
        }
        else {
            UnbufferedSerial::read(&c, 1);
            _rxbuf.store_char(c);
        }
    }

We need to define the UART module in HardwareSerial.h, setting default as 1 as the UART1 might be mapped to different pins:
uint32_t ui32Module = 1;

We need to add a line to determine that UART module 0 is used:

UART::UART( void ) :
    UART(STDIO_UART_TX, STDIO_UART_RX)
{
  ui32Module = 0
}

regards,
Paul