-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure to also run a current version of rustfmt
. Do you have an example for this functionality?
src/timer.rs
Outdated
|
||
fn start<T>(&mut self, count: T) where T: Into<Self::Time> { | ||
let duration = count.into(); | ||
debug_assert!(duration.as_secs() < ((u32::MAX as u64 - duration.subsec_micros) / 1_000_000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like debug_assert!()
calls in embedded because the chances are high that the code will be never exercised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error[E0615]: attempted to take value of method `subsec_micros` on type `core::time::Duration`
--> src/timer.rs:27:73
|
27 | debug_assert!(duration.as_secs() < ((u32::MAX as u64 - duration.subsec_micros) / 1_000_000));
| ^^^^^^^^^^^^^
|
= help: maybe a `()` to call it is missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I dislike the debug_assert
call too. The alternatives I see are:
assert
and hope it gets optimised out- I feel like this isn't too bad a choice, since using
Time = Duration
already relies onCountDown::start
being inlined and optimised it's highly likely this will be ok.
- I feel like this isn't too bad a choice, since using
- don't assert at all, just have it as a documentation item
- since this is likely to be passed into places that aren't
nrf51
specific they won't see this documentation and it's likely to go wrong at some point
- since this is likely to be passed into places that aren't
- Figure out some way to do constrain it at compile time
- I don't think even once const parameters are stable that Rust will be capable of doing this ergonomically
So, if you're happy with it I'll change this to a normal assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A normal assert
is fine.
Fixed compile error (I had switched a constant 1 second into the I'll try and push an example tomorrow, unfortunately it'll be going through a |
I've pushed an example at https://github.com/Nemo157/nrf51-blinker-rs, that uses this + some even more experimental GPIOTE edge detection to wait for a timer to expire or button to be pressed non-blockingly. |
Okay, that example is rather weird but it works. :-D I'm somewhat curious where you implemented the GPIOTE stuff you're talking about... |
NB: I've just pushed out a new version 0.4.2 to crates.io. Thanks a lot for your contributions. |
Nemo157/nrf51-hal@56d7cc6...input-edges I really don’t like the API I added to embedded-hal for it though; I’m going to try and come up with a better design before I’ll be opening PRs to add it. Also want to come up with a compile time checked API for assigning the event channels to avoid the assert. |
As I mentioned in rust-embedded/embedded-hal#59 the
Duration
struct gets compiled out fully in my test code, but I'm still not certain using it is the best idea in case it doesn't.