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

Cloneable timer #614

Merged
merged 10 commits into from
May 20, 2023
Merged

Cloneable timer #614

merged 10 commits into from
May 20, 2023

Conversation

jannic
Copy link
Member

@jannic jannic commented May 18, 2023

Make Timer a cloneable type:

We don't support de-initializing a Timer anyways. And once it's initialized, all operations on it are either read-only, or already wrapped in the separate alarm structures.

Therefore, we can safely allow for Timer to be cloned, and implement the delay functions directly on it. That makes the API more ergonomic.

Co-authored-by: Wilfried Chauveau wilfried.chauveau@ithinuel.me

rp2040-hal/src/timer.rs Outdated Show resolved Hide resolved
rp2040-hal/src/timer.rs Outdated Show resolved Hide resolved
jannic and others added 3 commits May 19, 2023 09:06
Thanks to @ithinuel for the idea

Co-authored-by: Wilfried Chauveau <wilfried.chauveau@ithinuel.me>
@jannic jannic marked this pull request as ready for review May 19, 2023 15:08
@jannic jannic added the enhancement New feature or request label May 19, 2023
@jannic jannic added this to the 0.9.0 milestone May 19, 2023
rp2040-hal/src/timer.rs Show resolved Hide resolved
rp2040-hal/src/timer.rs Outdated Show resolved Hide resolved
Having inherent and trait methods with the same name requires ugly disambiguation tricks
Comment on lines 147 to 150
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.

@jannic jannic merged commit 49d0460 into rp-rs:main May 20, 2023
8 checks passed
@jannic jannic deleted the cloneable-timer branch May 20, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants