From 9f28c7ab8d96d19a010246318dee1de73b3ed4ee Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timo=20Kr=C3=B6ger?= <timokroeger93@gmail.com>
Date: Sat, 2 Jan 2021 22:40:36 +0100
Subject: [PATCH 1/7] uarte: Do not spin when stopping a receive future
Spinning on drop() is still required when the future has not been
stopped so that DMA finishes before the buffer is released.
---
embassy-nrf/src/uarte.rs | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs
index 648298b84..494b92df3 100644
--- a/embassy-nrf/src/uarte.rs
+++ b/embassy-nrf/src/uarte.rs
@@ -313,7 +313,7 @@ where
{
fn drop(self: &mut Self) {
if self.uarte.rx_started() {
- trace!("stoprx");
+ trace!("stoprx (drop)");
self.uarte.instance.events_rxstarted.reset();
self.uarte
@@ -364,9 +364,20 @@ where
T: Instance,
{
/// Stops the ongoing reception and returns the number of bytes received.
- pub async fn stop(self) -> usize {
- drop(self);
- let len = T::state().rx_done.wait().await;
+ pub async fn stop(mut self) -> usize {
+ let len = if self.uarte.rx_started() {
+ trace!("stoprx (stop)");
+
+ self.uarte.instance.events_rxstarted.reset();
+ self.uarte
+ .instance
+ .tasks_stoprx
+ .write(|w| unsafe { w.bits(1) });
+ T::state().rx_done.wait().await
+ } else {
+ // Transfer was stopped before it even started. No bytes were sent.
+ 0
+ };
len as _
}
}
From a7c03e4cb6a9d39d0139cb88d224a0f80fd21820 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timo=20Kr=C3=B6ger?= <timokroeger93@gmail.com>
Date: Sun, 3 Jan 2021 11:12:11 +0100
Subject: [PATCH 2/7] uarte: Only stop RX forcefully when a reception is
running
The STOPRX task always triggers a timeout of ~55bit times until the
RXTO event is generated. Before we disabled the receiver only after
the timeout.
With this change the receiver is stopped right after reception has ended
because the DMA buffer is full.
For forced RX aborts like `stop()` or on drop still need to wait for the
RXTO event before disabling the receiver.
---
embassy-nrf/src/uarte.rs | 43 ++++++++++++++++++++++++----------------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs
index 494b92df3..895ac11c3 100644
--- a/embassy-nrf/src/uarte.rs
+++ b/embassy-nrf/src/uarte.rs
@@ -170,6 +170,13 @@ where
trace!("endrx");
let len = uarte.rxd.amount.read().bits();
compiler_fence(Ordering::SeqCst);
+
+ if uarte.events_rxstarted.read().bits() != 0 {
+ // The ENDRX was signal triggered because DMA buffer is full.
+ uarte.events_rxstarted.reset();
+ try_disable = true;
+ }
+
T::state().rx_done.signal(len);
}
@@ -227,6 +234,7 @@ impl<T: Instance> embassy::uart::Uart for Uarte<T> {
// `mem::forget()` on a previous future after polling it once.
assert!(!self.rx_started());
+ T::state().rx_done.reset();
self.enable();
ReceiveFuture {
@@ -334,27 +342,28 @@ where
fn poll(self: core::pin::Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
let Self { uarte, buf } = unsafe { self.get_unchecked_mut() };
- if !uarte.rx_started() {
- let uarte = &uarte.instance;
+ match T::state().rx_done.poll_wait(cx) {
+ Poll::Pending if !uarte.rx_started() => {
+ let uarte = &uarte.instance;
- T::state().rx_done.reset();
+ let ptr = buf.as_ptr();
+ let len = buf.len();
+ assert!(len <= EASY_DMA_SIZE);
- let ptr = buf.as_ptr();
- let len = buf.len();
- assert!(len <= EASY_DMA_SIZE);
+ compiler_fence(Ordering::SeqCst);
+ uarte.rxd.ptr.write(|w| unsafe { w.ptr().bits(ptr as u32) });
+ uarte
+ .rxd
+ .maxcnt
+ .write(|w| unsafe { w.maxcnt().bits(len as _) });
- compiler_fence(Ordering::SeqCst);
- uarte.rxd.ptr.write(|w| unsafe { w.ptr().bits(ptr as u32) });
- uarte
- .rxd
- .maxcnt
- .write(|w| unsafe { w.maxcnt().bits(len as _) });
-
- trace!("startrx");
- uarte.tasks_startrx.write(|w| unsafe { w.bits(1) });
+ trace!("startrx");
+ uarte.tasks_startrx.write(|w| unsafe { w.bits(1) });
+ Poll::Pending
+ }
+ Poll::Pending => Poll::Pending,
+ Poll::Ready(_) => Poll::Ready(Ok(())),
}
-
- T::state().rx_done.poll_wait(cx).map(|_| Ok(()))
}
}
From a3b3305b8e9afbedde58e6c44c112335b25fa176 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timo=20Kr=C3=B6ger?= <timokroeger93@gmail.com>
Date: Sun, 3 Jan 2021 12:02:03 +0100
Subject: [PATCH 3/7] uarte: Only stop TX forcefully when a transmissions is
running
This comes with insignificant power consumption improvements but makes
the code of the RX and TX case symmetric.
---
embassy-nrf/src/uarte.rs | 46 +++++++++++++++++++++++-----------------
1 file changed, 27 insertions(+), 19 deletions(-)
diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs
index 895ac11c3..80c31b671 100644
--- a/embassy-nrf/src/uarte.rs
+++ b/embassy-nrf/src/uarte.rs
@@ -156,6 +156,13 @@ where
uarte.events_endtx.reset();
trace!("endtx");
compiler_fence(Ordering::SeqCst);
+
+ if uarte.events_txstarted.read().bits() != 0 {
+ // The ENDTX was signal triggered because DMA has finished.
+ uarte.events_txstarted.reset();
+ try_disable = true;
+ }
+
T::state().tx_done.signal(());
}
@@ -211,6 +218,7 @@ impl<T: Instance> embassy::uart::Uart for Uarte<T> {
// `mem::forget()` on a previous future after polling it once.
assert!(!self.tx_started());
+ T::state().tx_done.reset();
self.enable();
SendFuture {
@@ -281,28 +289,28 @@ where
fn poll(self: core::pin::Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
let Self { uarte, buf } = unsafe { self.get_unchecked_mut() };
- if !uarte.tx_started() {
- let uarte = &uarte.instance;
+ match T::state().tx_done.poll_wait(cx) {
+ Poll::Pending if !uarte.tx_started() => {
+ let uarte = &uarte.instance;
+ let ptr = buf.as_ptr();
+ let len = buf.len();
+ assert!(len <= EASY_DMA_SIZE);
+ // TODO: panic if buffer is not in SRAM
- T::state().tx_done.reset();
+ compiler_fence(Ordering::SeqCst);
+ uarte.txd.ptr.write(|w| unsafe { w.ptr().bits(ptr as u32) });
+ uarte
+ .txd
+ .maxcnt
+ .write(|w| unsafe { w.maxcnt().bits(len as _) });
- let ptr = buf.as_ptr();
- let len = buf.len();
- assert!(len <= EASY_DMA_SIZE);
- // TODO: panic if buffer is not in SRAM
-
- compiler_fence(Ordering::SeqCst);
- uarte.txd.ptr.write(|w| unsafe { w.ptr().bits(ptr as u32) });
- uarte
- .txd
- .maxcnt
- .write(|w| unsafe { w.maxcnt().bits(len as _) });
-
- trace!("starttx");
- uarte.tasks_starttx.write(|w| unsafe { w.bits(1) });
+ trace!("starttx");
+ uarte.tasks_starttx.write(|w| unsafe { w.bits(1) });
+ Poll::Pending
+ }
+ Poll::Pending => Poll::Pending,
+ Poll::Ready(_) => Poll::Ready(Ok(())),
}
-
- T::state().tx_done.poll_wait(cx).map(|()| Ok(()))
}
}
From 85ec9dd16fb5649900722228e46e9c68370901a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timo=20Kr=C3=B6ger?= <timokroeger93@gmail.com>
Date: Sun, 3 Jan 2021 12:09:51 +0100
Subject: [PATCH 4/7] uarte: Be on safe side with potentially racy code
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The PS does not specify how many cycles it takes for a STARTXX task to
generate a XXSTARTED event. I think it is instantaneous but let’s be on
the safe side for the following sequence:
1. poll() starttx
2. drop() txstarted not yet set, but future gets dropped
3. txstarted set by hardware, peripheral enabled after it was dropped
---
embassy-nrf/src/uarte.rs | 55 +++++++++++++++++++++++-----------------
1 file changed, 32 insertions(+), 23 deletions(-)
diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs
index 80c31b671..156dbbefd 100644
--- a/embassy-nrf/src/uarte.rs
+++ b/embassy-nrf/src/uarte.rs
@@ -289,27 +289,31 @@ where
fn poll(self: core::pin::Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
let Self { uarte, buf } = unsafe { self.get_unchecked_mut() };
- match T::state().tx_done.poll_wait(cx) {
- Poll::Pending if !uarte.tx_started() => {
- let uarte = &uarte.instance;
- let ptr = buf.as_ptr();
- let len = buf.len();
- assert!(len <= EASY_DMA_SIZE);
- // TODO: panic if buffer is not in SRAM
+ if T::state().tx_done.poll_wait(cx).is_pending() {
+ let ptr = buf.as_ptr();
+ let len = buf.len();
+ assert!(len <= EASY_DMA_SIZE);
+ // TODO: panic if buffer is not in SRAM
- compiler_fence(Ordering::SeqCst);
- uarte.txd.ptr.write(|w| unsafe { w.ptr().bits(ptr as u32) });
- uarte
- .txd
- .maxcnt
- .write(|w| unsafe { w.maxcnt().bits(len as _) });
+ compiler_fence(Ordering::SeqCst);
+ uarte
+ .instance
+ .txd
+ .ptr
+ .write(|w| unsafe { w.ptr().bits(ptr as u32) });
+ uarte
+ .instance
+ .txd
+ .maxcnt
+ .write(|w| unsafe { w.maxcnt().bits(len as _) });
- trace!("starttx");
- uarte.tasks_starttx.write(|w| unsafe { w.bits(1) });
- Poll::Pending
- }
- Poll::Pending => Poll::Pending,
- Poll::Ready(_) => Poll::Ready(Ok(())),
+ trace!("starttx");
+ uarte.instance.tasks_starttx.write(|w| unsafe { w.bits(1) });
+ while !uarte.tx_started() {} // Make sure transmission has started
+
+ Poll::Pending
+ } else {
+ Poll::Ready(Ok(()))
}
}
}
@@ -352,21 +356,26 @@ where
match T::state().rx_done.poll_wait(cx) {
Poll::Pending if !uarte.rx_started() => {
- let uarte = &uarte.instance;
-
let ptr = buf.as_ptr();
let len = buf.len();
assert!(len <= EASY_DMA_SIZE);
compiler_fence(Ordering::SeqCst);
- uarte.rxd.ptr.write(|w| unsafe { w.ptr().bits(ptr as u32) });
uarte
+ .instance
+ .rxd
+ .ptr
+ .write(|w| unsafe { w.ptr().bits(ptr as u32) });
+ uarte
+ .instance
.rxd
.maxcnt
.write(|w| unsafe { w.maxcnt().bits(len as _) });
trace!("startrx");
- uarte.tasks_startrx.write(|w| unsafe { w.bits(1) });
+ uarte.instance.tasks_startrx.write(|w| unsafe { w.bits(1) });
+ while !uarte.rx_started() {} // Make sure reception has started
+
Poll::Pending
}
Poll::Pending => Poll::Pending,
From 9b1f7b8a17bd1ebd61e68c5d2a2116794d31cfa7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timo=20Kr=C3=B6ger?= <timokroeger93@gmail.com>
Date: Sun, 3 Jan 2021 13:31:33 +0100
Subject: [PATCH 5/7] uarte: Enable peripheral with first poll
This fixes a lockup when a future is dropped before it was polled.
---
embassy-nrf/src/uarte.rs | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs
index 156dbbefd..aee91f803 100644
--- a/embassy-nrf/src/uarte.rs
+++ b/embassy-nrf/src/uarte.rs
@@ -219,7 +219,6 @@ impl<T: Instance> embassy::uart::Uart for Uarte<T> {
assert!(!self.tx_started());
T::state().tx_done.reset();
- self.enable();
SendFuture {
uarte: self,
@@ -243,7 +242,6 @@ impl<T: Instance> embassy::uart::Uart for Uarte<T> {
assert!(!self.rx_started());
T::state().rx_done.reset();
- self.enable();
ReceiveFuture {
uarte: self,
@@ -257,7 +255,7 @@ pub struct SendFuture<'a, T>
where
T: Instance,
{
- uarte: &'a Uarte<T>,
+ uarte: &'a mut Uarte<T>,
buf: &'a [u8],
}
@@ -295,6 +293,8 @@ where
assert!(len <= EASY_DMA_SIZE);
// TODO: panic if buffer is not in SRAM
+ uarte.enable();
+
compiler_fence(Ordering::SeqCst);
uarte
.instance
@@ -323,7 +323,7 @@ pub struct ReceiveFuture<'a, T>
where
T: Instance,
{
- uarte: &'a Uarte<T>,
+ uarte: &'a mut Uarte<T>,
buf: &'a mut [u8],
}
@@ -359,6 +359,8 @@ where
let ptr = buf.as_ptr();
let len = buf.len();
assert!(len <= EASY_DMA_SIZE);
+
+ uarte.enable();
compiler_fence(Ordering::SeqCst);
uarte
From 93780fa31d3c563e05fac6cd6ff8f15e43dc80fa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timo=20Kr=C3=B6ger?= <timokroeger93@gmail.com>
Date: Sun, 3 Jan 2021 13:56:13 +0100
Subject: [PATCH 6/7] uarte: Wait for the peripheral to be disabled
Prevents a panic in the case of:
1. Abort a receive future
2. Free Uarte::free()
3. Uarte::new()
-> panicked at 'assertion failed: uarte.enable.read().enable().is_disabled()'
---
embassy-nrf/src/uarte.rs | 2 ++
1 file changed, 2 insertions(+)
diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs
index aee91f803..dc2093c38 100644
--- a/embassy-nrf/src/uarte.rs
+++ b/embassy-nrf/src/uarte.rs
@@ -131,6 +131,8 @@ where
}
pub fn free(self) -> (T, T::Interrupt, Pins) {
+ // Wait for the peripheral to be disabled from the ISR.
+ while self.instance.enable.read().enable().is_enabled() {}
(self.instance, self.irq, self.pins)
}
From 0631623b511d2906993bf2fa3799ea7eec3d7f4a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timo=20Kr=C3=B6ger?= <timokroeger93@gmail.com>
Date: Sun, 3 Jan 2021 17:05:04 +0100
Subject: [PATCH 7/7] uarte: Low power wait for RX drop
---
embassy-nrf/src/uarte.rs | 13 ++++++++-----
embassy-nrf/src/util/mod.rs | 10 ++++++++++
embassy/src/util/signal.rs | 9 ++-------
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs
index dc2093c38..bfebf5cab 100644
--- a/embassy-nrf/src/uarte.rs
+++ b/embassy-nrf/src/uarte.rs
@@ -17,8 +17,8 @@ use crate::hal::gpio::Port as GpioPort;
use crate::hal::pac;
use crate::hal::prelude::*;
use crate::hal::target_constants::EASY_DMA_SIZE;
-use crate::interrupt;
use crate::interrupt::OwnedInterrupt;
+use crate::{interrupt, util};
pub use crate::hal::uarte::Pins;
// Re-export SVD variants to allow user to directly set values.
@@ -275,7 +275,9 @@ where
.instance
.tasks_stoptx
.write(|w| unsafe { w.bits(1) });
- T::state().tx_done.blocking_wait();
+
+ // TX is stopped almost instantly, spinning is fine.
+ while !T::state().tx_done.signaled() {}
}
}
}
@@ -342,7 +344,8 @@ where
.instance
.tasks_stoprx
.write(|w| unsafe { w.bits(1) });
- T::state().rx_done.blocking_wait();
+
+ util::low_power_wait_until(|| T::state().rx_done.signaled())
}
}
}
@@ -361,7 +364,7 @@ where
let ptr = buf.as_ptr();
let len = buf.len();
assert!(len <= EASY_DMA_SIZE);
-
+
uarte.enable();
compiler_fence(Ordering::SeqCst);
@@ -394,7 +397,7 @@ where
T: Instance,
{
/// Stops the ongoing reception and returns the number of bytes received.
- pub async fn stop(mut self) -> usize {
+ pub async fn stop(self) -> usize {
let len = if self.uarte.rx_started() {
trace!("stoprx (stop)");
diff --git a/embassy-nrf/src/util/mod.rs b/embassy-nrf/src/util/mod.rs
index 2fd5453d3..cf3306545 100644
--- a/embassy-nrf/src/util/mod.rs
+++ b/embassy-nrf/src/util/mod.rs
@@ -1,2 +1,12 @@
pub mod peripheral;
pub mod ring_buffer;
+
+/// Low power blocking wait loop using WFE/SEV.
+pub fn low_power_wait_until(mut condition: impl FnMut() -> bool) {
+ while !condition() {
+ // WFE might "eat" an event that would have otherwise woken the executor.
+ cortex_m::asm::wfe();
+ }
+ // Retrigger an event to be transparent to the executor.
+ cortex_m::asm::sev();
+}
diff --git a/embassy/src/util/signal.rs b/embassy/src/util/signal.rs
index cb6410611..8e778d1e3 100644
--- a/embassy/src/util/signal.rs
+++ b/embassy/src/util/signal.rs
@@ -63,12 +63,7 @@ impl<T: Send> Signal<T> {
futures::future::poll_fn(move |cx| self.poll_wait(cx))
}
- /// Blocks until the signal has been received.
- ///
- /// Returns immediately when [`poll_wait()`] has not been called before.
- pub fn blocking_wait(&self) {
- while cortex_m::interrupt::free(|_| {
- matches!(unsafe { &*self.state.get() }, State::Waiting(_))
- }) {}
+ pub fn signaled(&self) -> bool {
+ cortex_m::interrupt::free(|_| matches!(unsafe { &*self.state.get() }, State::Signaled(_)))
}
}