Migrate crate over to embedded-time #183
Conversation
|
|
||
| /// Wait for the given time. | ||
| /// | ||
| /// Note that durations above `u32::MAX` microseconds will be clamped at `u32::MAX`. |
There was a problem hiding this comment.
I'm not particularly happy with this behaviour but I can't think of a better solution right now. Thoughts appreciated!
There was a problem hiding this comment.
I'm not happy about that either, but can't think of a better solution right now. A fallible interface isn't great, panicking isn't great either. Your solution here at least should work for the common case, and the pitfall is documented.
| pub fn delay<T>(&mut self, delay: T) | ||
| where | ||
| T: Into<MicroSeconds>, | ||
| T: TryInto<Microseconds>, |
There was a problem hiding this comment.
This is so we can use e.g. 1.second() or 300.milliseconds() in calling code. We could also use this pattern in other hertz-accepting places. There's at least one demo that replaces 1.khz() with 100_000.Hz() which I don't like.
| @@ -24,7 +24,7 @@ fn main() -> ! { | |||
| let gpioa = dp.GPIOA.split(&mut rcc); | |||
|
|
|||
| // Initialize TIM2 for PWM | |||
| let pwm = pwm::Timer::new(dp.TIM2, 10.khz(), &mut rcc); | |||
| let pwm = pwm::Timer::new(dp.TIM2, 10_000.Hz(), &mut rcc); | |||
There was a problem hiding this comment.
This (and IIRC the SPI interface require this change unless I use TryInto in the APIs - embedded-time can convert from e.g. Khz<u32> to Hz<u64> without failing, but needs the fallible conversions if we want to go from Khz<u32> to Hz<u32>. Realistically, confining to u32 everywhere isn't an issue (IMO) so I'd be up for using TryInto and clamping to u32::MAX like I do with the delay() API. Thoughts?
jamwaffles
force-pushed
the
embedded-time
branch
5 times, most recently
from
a642284
to
a4745ff
Compare
There was a problem hiding this comment.
Thank you, @jamwaffles! I found one issue (see comment), but otherwise this looks great!
|
|
||
| /// Wait for the given time. | ||
| /// | ||
| /// Note that durations above `u32::MAX` microseconds will be clamped at `u32::MAX`. |
There was a problem hiding this comment.
I'm not happy about that either, but can't think of a better solution right now. A fallible interface isn't great, panicking isn't great either. Your solution here at least should work for the common case, and the pitfall is documented.
src/serial.rs
Outdated
| @@ -93,14 +91,14 @@ pub enum StopBits { | |||
| } | |||
|
|
|||
| pub struct Config { | |||
| pub baudrate: Bps, | |||
| pub baudrate: BitsPerSecond, | |||
There was a problem hiding this comment.
This isn't right, is it? I think we need BytesPerSecond.
(I don't think it makes a difference in functionality, but it's the wrong type semantically. Could lead to confusion.)
There was a problem hiding this comment.
Turns out there's a Baud struct which is far better suited lol. I force pushed to the branch with a fix for BitsPerSecond to Baud.
jamwaffles
force-pushed
the
embedded-time
branch
2 times, most recently
from
a5676aa
to
260d8ad
Compare
jamwaffles
force-pushed
the
embedded-time
branch
from
260d8ad
to
2c9e21c
Compare
IIRC I did test the examples when working on stm32-rs#183, but this error shows up when using the PLL which none of them do. I found this when testing with my own project code. At any rate, here's a fix for embedded-time comparisons when using the PLL.
IIRC I did test the examples when working on stm32-rs#183, but this error shows up when using the PLL which none of them do. I found this when testing with my own project code. At any rate, here's a fix for embedded-time comparisons when using the PLL.
No description provided.