Skip to content
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

[Gen 3] Fixes UART RX DMA transfer size issues #2264

Merged
merged 2 commits into from
Jan 19, 2021

Conversation

avtolstoy
Copy link
Member

@avtolstoy avtolstoy commented Jan 11, 2021

Problem

Originally reported in https://community.particle.io/t/serial1-rx-randomly-dies-on-boron-2-0-1-red-sos-crash-with-second-serial1-begin/58888/10

There is a problem in how we commit received bytes while RX DMA transfer is still ongoing (https://github.com/particle-iot/device-os/blob/develop/hal/src/nRF52840/usart_hal.cpp#L279), causing ring buffer to get into an invalid state and subsequent RX DMA transfers to be configured incorrectly with very large sizes, which causes out of bounds writes.

Solution

  1. The timer that counts the received bytes with PPI can count more than what's actually been put into the receive buffer by DMA, so we'll simply limit by that. This is the main fix. https://github.com/particle-iot/device-os/compare/fix/ch70753-gen3-uart-out-of-bounds?expand=1#diff-7b4e007cfccb7b35c77f62bc690413bf701398529aca65632392e125634271ccR287
  2. Make sure that whenever TX or RX interrupt is disabled we are not processing TX/RX events in the interrupt handler whenever some other event causes an interrupt
  3. Minor fix in Usart::read() to re-start RX DMA transfer under an RX lock. Just in case.
  4. C++ standard version for unit tests have been reverted for now to C++14, as we need to update host compiler and cmake on CI.

Steps to Test

Use the following application along with a python script writing into Serial1 through an USB-UART adapter. Watch the USB Serial.

#include "Particle.h"
#include <functional>

SYSTEM_MODE(AUTOMATIC);

const size_t BUFF_SIZE = 50; 
const size_t READ_SIZE = BUFF_SIZE;
static volatile uint32_t marker1 = 0xdeadbeef;
static char buff[BUFF_SIZE];
static volatile uint32_t marker = 0xdeadbeef;
static uint32_t loop_count = 0;
static uint32_t total_read = 0;
static volatile uint32_t marker2 = 0xdeadbeef;

void setup() {
    Serial.begin();
    Serial1.begin(230400);
    Serial.printlnf("-- BEGIN --");
}

void loop() {
    int nread = Serial1.readBytes(buff, READ_SIZE);
    volatile uint32_t marker3 = 0xdeadbeef;
    if (nread > 0) {
        total_read += nread;
        if (nread != READ_SIZE) {
            Serial.printlnf("nread: %d ", nread);
        }
    }
    loop_count += 1;
    Serial.printlnf("loop %lu read: %lu marker: %x %x %x %x", loop_count, total_read, marker, marker1, marker2, marker3);
}
import serial

path = "/dev/ttyUSB0"
ser = serial.Serial(path, 230400)
msg = "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
raw_bytes = msg.encode()
la_len = len(raw_bytes)
total_sent = 0
for x in range(2500):
        ser.write(raw_bytes)
        total_sent += la_len
ser.close()

print('total_sent {}.'.format(total_sent))

Before this PR, the markers will eventually change value from 0xdeadbeef to something else indicating an out of bounds write.

References


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@avtolstoy avtolstoy added the bug label Jan 11, 2021
@avtolstoy avtolstoy added this to the 2.0.1 milestone Jan 11, 2021
@avtolstoy avtolstoy modified the milestones: 2.0.1, 2.0.2 Jan 11, 2021
@avtolstoy avtolstoy force-pushed the fix/ch70753-gen3-uart-out-of-bounds branch from d7f0935 to 4b4608a Compare January 19, 2021 15:57
@avtolstoy avtolstoy merged commit d6259fe into develop Jan 19, 2021
@avtolstoy avtolstoy deleted the fix/ch70753-gen3-uart-out-of-bounds branch January 19, 2021 15:58
@jack-particle
Copy link

Use the following application along with a python script writing into Serial1 through an USB-UART adapter. Watch the USB Serial.

Hey @avtolstoy ,how long does the testing example need to reproduce the issue?Still randomly?

@avtolstoy
Copy link
Member Author

@jack-particle Usually a few minutes is enough, but yes, it's generally an 'undefined' amount of time. Eventually affected releases should showcase this problem.

@jack-particle
Copy link

@jack-particle Usually a few minutes is enough, but yes, it's generally an 'undefined' amount of time. Eventually affected releases should showcase this problem.

@avtolstoy Do you think the time is related to the baud rate? The Zoomo project has a hardFault exception every few hours(actually randomly) that triggers the system panic. I found out before the system panic reset the serial can not provide the data from the gps module.And then the device was reset.The baud rate is 115200 now. Your testing sample code is  230400.
image

@avtolstoy
Copy link
Member Author

@jack-particle Baudrate should be irrelevant, but 230400 was showcasing the problem quicker.

@jack-particle
Copy link

@jack-particle Baudrate should be irrelevant, but 230400 was showcasing the problem quicker.

Thanks.You are right.I have used 460800 for testing. And it did not help for reproducing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants