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

[gen3] hal: fixes the issue that UARTE RX may loss data. #2698

Merged
merged 1 commit into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 45 additions & 13 deletions hal/src/nRF52840/usart_hal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,18 @@ class RxLock {
public:
RxLock(NRF_UARTE_Type* uarte)
: uarte_(uarte) {
taskENTER_CRITICAL();
nrf_uarte_int_disable(uarte, NRF_UARTE_INT_ENDRX_MASK);
__DSB();
__ISB();
}
~RxLock() {
nrf_uarte_int_enable(uarte_, NRF_UARTE_INT_ENDRX_MASK);
taskEXIT_CRITICAL();
__DSB();
__ISB();
}


private:
NRF_UARTE_Type* uarte_;
Expand Down Expand Up @@ -113,6 +120,7 @@ class Usart {
transmitting_(false),
receiving_(0),
rxConsumed_(0),
lastTimerValue_(0),
evGroup_(nullptr) {
evGroup_ = xEventGroupCreate();
SPARK_ASSERT(evGroup_);
Expand Down Expand Up @@ -271,10 +279,13 @@ class Usart {
return begin(config_);
}

ssize_t data() {
ssize_t data(size_t* consumable = nullptr) {
CHECK_TRUE(isEnabled(), SYSTEM_ERROR_INVALID_STATE);
RxLock lk(uarte_);
ssize_t d = rxBuffer_.data();

size_t d = CHECK(rxBuffer_.data());

// At the very least we already have `d` commited bytes in rx buffer

if (receiving_) {
// IMPORTANT: It seems that the timer value (which is a counter for every time UARTE generates RXDRDY event)
// may potentially get larger than the configured RX DMA transfer:
Expand All @@ -284,11 +295,22 @@ class Usart {
// </quote>
// We'll be extra careful here and make sure not to consume more than we can, otherwise
// we may put the ring buffer into an invalid state as there are not safety checks.
const ssize_t toConsume = std::min(rxBuffer_.acquirePending(), timerValue() - rxConsumed_);
if (toConsume > 0) {
rxBuffer_.acquireCommit(toConsume);
rxConsumed_ += toConsume;
d += toConsume;
uint16_t lastTimer = lastTimerValue_;
uint16_t rxConsumed = rxConsumed_;
if (rxConsumed == 0) {
// Re-read lastTimerValue_ to sync up
lastTimer = lastTimerValue_;
// Update d, as we might have received ENDRX in-between previous `d` calculation and now
d = CHECK(rxBuffer_.data());
if (!receiving_) {
return d;
}
}
uint16_t timerVal = timerValue() - lastTimer; // https://en.cppreference.com/w/c/language/conversion
const size_t canConsume = std::min<uint16_t>(timerVal - rxConsumed, rxBuffer_.acquirePending());
d += canConsume;
if (consumable) {
*consumable = canConsume;
}
}
return d;
Expand All @@ -302,10 +324,18 @@ class Usart {

ssize_t read(uint8_t* buffer, size_t size) {
CHECK_TRUE(isEnabled(), SYSTEM_ERROR_INVALID_STATE);
const ssize_t maxRead = CHECK(data());
size_t consumable = 0;
const ssize_t maxRead = CHECK(data(&consumable));
const size_t readSize = std::min((size_t)maxRead, size);
CHECK_TRUE(readSize > 0, SYSTEM_ERROR_NO_MEMORY);
RxLock lk(uarte_);
if (consumable > 0) {
CHECK(data(&consumable));
if (consumable > 0) {
rxBuffer_.acquireCommit(consumable);
rxConsumed_ += consumable;
}
}
ssize_t r = CHECK(rxBuffer_.get(buffer, readSize));
if (!receiving_) {
startReceiver();
Expand Down Expand Up @@ -398,7 +428,7 @@ class Usart {
nrf_uarte_event_clear(uarte_, NRF_UARTE_EVENT_ENDRX);
nrf_uarte_event_clear(uarte_, NRF_UARTE_EVENT_RXDRDY);

nrf_timer_task_trigger(timer_, NRF_TIMER_TASK_CLEAR);
lastTimerValue_ += (rxConsumed_ + (uint16_t)(rxBuffer_.acquirePending()));
rxConsumed_ = 0;

if (rxBuffer_.acquirePending() > 0) {
Expand Down Expand Up @@ -656,11 +686,12 @@ class Usart {
void disableTimer() {
nrf_timer_task_trigger(timer_, NRF_TIMER_TASK_CLEAR);
nrf_timer_task_trigger(timer_, NRF_TIMER_TASK_SHUTDOWN);
lastTimerValue_ = 0;
nrf_ppi_channel_disable(ppi_);
rxConsumed_ = 0;
}

size_t timerValue() {
uint16_t timerValue() {
nrf_timer_task_trigger(timer_, NRF_TIMER_TASK_CAPTURE0);
return nrf_timer_cc_read(timer_, NRF_TIMER_CC_CHANNEL0);
}
Expand Down Expand Up @@ -748,7 +779,8 @@ class Usart {

volatile bool transmitting_;
volatile uint8_t receiving_;
volatile size_t rxConsumed_;
volatile uint16_t rxConsumed_;
volatile uint16_t lastTimerValue_;

Config config_ = {};

Expand All @@ -760,7 +792,7 @@ class Usart {

constexpr const Usart::BaudrateMap Usart::baudrateMap_[];

const auto UARTE0_INTERRUPT_PRIORITY = APP_IRQ_PRIORITY_LOW;
const auto UARTE0_INTERRUPT_PRIORITY = (app_irq_priority_t)_PRIO_APP_HIGH; // Equal to configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY
// TODO: move this to hal_platform_config.h ?
const auto UARTE1_INTERRUPT_PRIORITY = (app_irq_priority_t)_PRIO_SD_LOWEST;

Expand Down
16 changes: 13 additions & 3 deletions services/inc/ringbuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -357,11 +357,21 @@ inline size_t RingBuffer<T>::curSpace() const {

template <typename T>
inline size_t RingBuffer<T>::curData() const {
if (head_ >= tail_ && !full_) {
return head_ - tail_;
// Tail is only modified by the consumer/reader, we don't care about it
// head_, curSize_ and full_ are the ones that are modified by producer and would
// usually be modified in ISR.
size_t head = head_;

if (head > tail_) {
return head - tail_;
} else if (head == tail_ && !full_) {
return 0;
} else {
return curSize_ + head_ - tail_;
// NOTE: this might not work well without locking for multiple concurrent acquirable regions
// For a single region, curSize_ cannot change if head is already behind tail.
return curSize_ + head - tail_;
}

}

template <typename T>
Expand Down