Skip to content

Cloneable timer #614

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
May 20, 2023
Merged

Cloneable timer #614

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
837486d
Make Timer cloneable
jannic May 18, 2023
a9e88b1
Update blinky example
jannic May 18, 2023
291baa3
Inline the three calls to timer()
jannic May 18, 2023
25dfeb8
Implement DelayUs<T>, DelayMs<T> for more values of T
jannic May 18, 2023
f69319b
Remove implementation details from doc comment
jannic May 19, 2023
6aef40d
Implement delay using 32bit arithmetic
jannic May 19, 2023
cd258f9
Fix clippy warning
jannic May 19, 2023
ae9e1f5
Fix comparison with u32::MAX for smaller types
jannic May 19, 2023
e9aee64
Rename internal delay function
jannic May 20, 2023
7fb1484
Remove delay implementations with u64 parameters
jannic May 20, 2023
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
12 changes: 5 additions & 7 deletions rp2040-hal/examples/blinky.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ use rp2040_hal as hal;
use hal::pac;

// Some traits we need
use embedded_hal::blocking::delay::DelayMs;
use embedded_hal::digital::v2::OutputPin;
use rp2040_hal::clocks::Clock;

/// The linker will place this boot block at the start of our program image. We
/// need this to help the ROM bootloader get our code up and running.
Expand All @@ -47,13 +47,12 @@ const XTAL_FREQ_HZ: u32 = 12_000_000u32;
fn main() -> ! {
// Grab our singleton objects
let mut pac = pac::Peripherals::take().unwrap();
let core = pac::CorePeripherals::take().unwrap();

// Set up the watchdog driver - needed by the clock setup code
let mut watchdog = hal::Watchdog::new(pac.WATCHDOG);

// Configure the clocks
let clocks = hal::clocks::init_clocks_and_plls(
let _clocks = hal::clocks::init_clocks_and_plls(
XTAL_FREQ_HZ,
pac.XOSC,
pac.CLOCKS,
Expand All @@ -65,7 +64,7 @@ fn main() -> ! {
.ok()
.unwrap();

let mut delay = cortex_m::delay::Delay::new(core.SYST, clocks.system_clock.freq().to_Hz());
let mut timer = rp2040_hal::Timer::new(pac.TIMER, &mut pac.RESETS);

// The single-cycle I/O block controls our GPIO pins
let sio = hal::Sio::new(pac.SIO);
Expand All @@ -82,10 +81,9 @@ fn main() -> ! {
let mut led_pin = pins.gpio25.into_push_pull_output();
loop {
led_pin.set_high().unwrap();
// TODO: Replace with proper 1s delays once we have clocks working
delay.delay_ms(500);
timer.delay_ms(500);
led_pin.set_low().unwrap();
delay.delay_ms(500);
timer.delay_ms(500);
}
}

Expand Down
139 changes: 100 additions & 39 deletions rp2040-hal/src/timer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use crate::atomic_register_access::{write_bitmask_clear, write_bitmask_set};
use crate::pac::{RESETS, TIMER};
use crate::resets::SubsystemReset;
use crate::typelevel::Sealed;
use core::marker::PhantomData;
use core::sync::atomic::{AtomicU8, Ordering};

/// Instant type used by the Timer & Alarm methods.
Expand All @@ -35,21 +34,20 @@ fn release_alarm(mask: u8) {
});
}

fn get_counter(timer: &crate::pac::timer::RegisterBlock) -> Instant {
let mut hi0 = timer.timerawh.read().bits();
let timestamp = loop {
let low = timer.timerawl.read().bits();
let hi1 = timer.timerawh.read().bits();
if hi0 == hi1 {
break (u64::from(hi0) << 32) | u64::from(low);
}
hi0 = hi1;
};
TimerInstantU64::from_ticks(timestamp)
}
/// Timer peripheral
//
// This struct logically wraps a `pac::TIMER`, but doesn't actually store it:
// As after initialization all accesses are read-only anyways, the `pac::TIMER` can
// be summoned unsafely instead. This allows timer to be cloned.
//
// (Alarms do use write operations, but they are local to the respective alarm, and
// those are still owned singletons.)
//
// As the timer peripheral needs to be started first, this struct can only be
// constructed by calling `Timer::new(...)`.
#[derive(Clone, Copy)]
pub struct Timer {
timer: TIMER,
_private: (),
}

impl Timer {
Expand All @@ -61,17 +59,29 @@ impl Timer {
pub fn new(timer: TIMER, resets: &mut RESETS) -> Self {
timer.reset_bring_down(resets);
timer.reset_bring_up(resets);
Self { timer }
Self { _private: () }
}

/// Get the current counter value.
pub fn get_counter(&self) -> Instant {
get_counter(&self.timer)
// Safety: Only used for reading current timer value
let timer = unsafe { &*pac::TIMER::PTR };
let mut hi0 = timer.timerawh.read().bits();
let timestamp = loop {
let low = timer.timerawl.read().bits();
let hi1 = timer.timerawh.read().bits();
if hi0 == hi1 {
break (u64::from(hi0) << 32) | u64::from(low);
}
hi0 = hi1;
};
TimerInstantU64::from_ticks(timestamp)
}

/// Get the value of the least significant word of the counter.
pub fn get_counter_low(&self) -> u32 {
self.timer.timerawl.read().bits()
// Safety: Only used for reading current timer value
unsafe { &*pac::TIMER::PTR }.timerawl.read().bits()
}

/// Initialized a Count Down instance without starting it.
Expand All @@ -84,27 +94,85 @@ impl Timer {
}
/// Retrieve a reference to alarm 0. Will only return a value the first time this is called
pub fn alarm_0(&mut self) -> Option<Alarm0> {
take_alarm(1 << 0).then_some(Alarm0(PhantomData))
take_alarm(1 << 0).then_some(Alarm0(*self))
}

/// Retrieve a reference to alarm 1. Will only return a value the first time this is called
pub fn alarm_1(&mut self) -> Option<Alarm1> {
take_alarm(1 << 1).then_some(Alarm1(PhantomData))
take_alarm(1 << 1).then_some(Alarm1(*self))
}

/// Retrieve a reference to alarm 2. Will only return a value the first time this is called
pub fn alarm_2(&mut self) -> Option<Alarm2> {
take_alarm(1 << 2).then_some(Alarm2(PhantomData))
take_alarm(1 << 2).then_some(Alarm2(*self))
}

/// Retrieve a reference to alarm 3. Will only return a value the first time this is called
pub fn alarm_3(&mut self) -> Option<Alarm3> {
take_alarm(1 << 3).then_some(Alarm3(PhantomData))
take_alarm(1 << 3).then_some(Alarm3(*self))
}

/// Pauses execution for at minimum `us` microseconds.
fn delay_us_internal(&self, mut us: u32) {
let mut start = self.get_counter_low();
// If we knew that the loop ran at least once per timer tick,
// this could be simplified to:
// ```
// while timer.timelr.read().bits().wrapping_sub(start) <= us {
// cortex_m::asm::nop();
// }
// ```
// However, due to interrupts, for `us == u32::MAX`, we could
// miss the moment where the loop should terminate if the loop skips
// a timer tick.
loop {
let now = self.get_counter_low();
let waited = now.wrapping_sub(start);
if waited >= us {
break;
}
start = now;
us -= waited;
}
}
}

// safety: all write operations are synchronised and all reads are atomic
unsafe impl Sync for Timer {}
macro_rules! impl_delay_traits {
($($t:ty),+) => {
$(
impl embedded_hal::blocking::delay::DelayUs<$t> for Timer {
fn delay_us(&mut self, mut us: $t) {
#![allow(unused_comparisons)]
assert!(us >= 0); // Only meaningful for i32
while <$t>::MAX as u64 > u32::MAX as u64 && us > u32::MAX as $t {
self.delay_us_internal(u32::MAX);
us -= u32::MAX as $t;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really find this convoluted for a loop that's only present when we're implementing support for u64.
I think it would be more readable and less error-prone to have this in its own impl block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair. It's one thing that the optimizer removes the loop, but that doesn't help readability. I'll implement the u64 version separately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should we skip the u64 implementation entirely? A blocking delay of more than an hour is usually just wrong. And if someone really needs it for some reason, it's not difficult to implement it locally:

 use embedded_hal::blocking::delay::DelayUs;                                                                                                                                                                     
                                                                                                                                                                                                                 
 pub struct DelayU64<T>(T);                                                                                                                                                                                      
                                                                                                                                                                                                                 
 impl<T> DelayU64<T> {                                                                                                                                                                                           
     pub fn new(delay: T) -> Self {                                                                                                                                                                              
         Self(delay)                                                                                                                                                                                             
     }                                                                                                                                                                                                           
 }                                                                                                                                                 
                                                                                                                                                                                                                 
 impl<T> DelayUs<u64> for DelayU64<T>                                                                                                                                                                            
 where                                                                                                                                                                                                           
     T: DelayUs<u32>,                                                                                                                                                                                            
 {                                                                                                                                                                                                               
     fn delay_us(&mut self, mut us: u64) {                                                                                                                                                                 
         while us > u32::MAX as u64 {                                                                                                                                                                
             self.0.delay_us(u32::MAX);                                                                                                                                                                          
             us -= u32::MAX as u64;                                                                                             
         }                                                                                                                      
         self.0.delay_us(us as u32);                                                                                            
     }                                                                                                                          
 }

And in case anyone really misses these functions, we can always add them later.

self.delay_us_internal(us as u32)
}
}
impl embedded_hal::blocking::delay::DelayMs<$t> for Timer {
fn delay_ms(&mut self, ms: $t) {
#![allow(unused_comparisons)]
assert!(ms >= 0); // Only meaningful for i32
for _ in 0..ms {
self.delay_us_internal(1000);
}
}
}
)*
}
}

// The implementation for i32 is a workaround to allow `delay_ms(42)` construction without specifying a type.
impl_delay_traits!(u8, u16, u32, u64, i32);

#[cfg(feature = "eh1_0_alpha")]
impl eh1_0_alpha::delay::DelayUs for Timer {
fn delay_us(&mut self, us: u32) {
self.delay_us_internal(us)
}
}

/// Implementation of the embedded_hal::Timer traits using rp2040_hal::timer counter
///
Expand Down Expand Up @@ -224,14 +292,12 @@ pub trait Alarm: Sealed {
macro_rules! impl_alarm {
($name:ident { rb: $timer_alarm:ident, int: $int_alarm:ident, int_name: $int_name:tt, armed_bit_mask: $armed_bit_mask: expr }) => {
/// An alarm that can be used to schedule events in the future. Alarms can also be configured to trigger interrupts.
pub struct $name(PhantomData<()>);
pub struct $name(Timer);
impl $name {
fn schedule_internal(
&mut self,
timer: &crate::pac::timer::RegisterBlock,
timestamp: Instant,
) -> Result<(), ScheduleAlarmError> {
fn schedule_internal(&mut self, timestamp: Instant) -> Result<(), ScheduleAlarmError> {
let timestamp_low = (timestamp.ticks() & 0xFFFF_FFFF) as u32;
// Safety: Only used to access bits belonging exclusively to this alarm
let timer = unsafe { &*pac::TIMER::PTR };

// This lock is for time-criticality
cortex_m::interrupt::free(|_| {
Expand All @@ -241,7 +307,7 @@ macro_rules! impl_alarm {
alarm.write(|w| unsafe { w.bits(timestamp_low) });

// If it is not set, it has already triggered.
let now = get_counter(timer);
let now = self.0.get_counter();
if now > timestamp && (timer.armed.read().bits() & $armed_bit_mask) != 0 {
// timestamp was set to a value in the past

Expand Down Expand Up @@ -318,11 +384,8 @@ macro_rules! impl_alarm {
///
/// [enable_interrupt]: #method.enable_interrupt
fn schedule(&mut self, countdown: MicrosDurationU32) -> Result<(), ScheduleAlarmError> {
// safety: Only read operations are made on the timer and they should not have any UB
let timer = unsafe { &*TIMER::ptr() };
let timestamp = get_counter(timer) + countdown;

self.schedule_internal(timer, timestamp)
let timestamp = self.0.get_counter() + countdown;
self.schedule_internal(timestamp)
}

/// Schedule the alarm to be finished at the given timestamp. If [enable_interrupt] is
Expand All @@ -335,15 +398,13 @@ macro_rules! impl_alarm {
///
/// [enable_interrupt]: #method.enable_interrupt
fn schedule_at(&mut self, timestamp: Instant) -> Result<(), ScheduleAlarmError> {
// safety: Only read operations are made on the timer and they should not have any UB
let timer = unsafe { &*TIMER::ptr() };
let now = get_counter(timer);
let now = self.0.get_counter();
let duration = timestamp.ticks().saturating_sub(now.ticks());
if duration > u32::max_value().into() {
return Err(ScheduleAlarmError::AlarmTooLate);
}

self.schedule_internal(timer, timestamp)
self.schedule_internal(timestamp)
}

/// Return true if this alarm is finished. The returned value is undefined if the alarm
Expand Down