Adds some simple methods to Timer. #52

thalesfragoso · 2019-03-28T19:01:11Z

Very simple methods but potentially useful, especially the one to clear the update interrupt flag.
I was having trouble using the timer with interrupts, because the Timer takes ownership of the registers and the tim field is private.
Sorry if I got something wrong, I am kinda new to rust.

therealprof · 2019-03-28T19:08:16Z

Looks okay to me but there's no need to release Clocks since it's a copyable resource.

thalesfragoso · 2019-03-28T19:27:44Z

Looks okay to me but there's no need to release Clocks since it's a copyable resource.

It makes sense, I did not realize that before. I have changed that bit in the new commit.

therealprof · 2019-03-28T19:37:39Z

src/timer.rs

+        self.tim.disable_counter();
+    }
+
+    /// Releases the SYST and clocks


Suggested change

/// Releases the SYST and clocks

/// Releases the SYST

therealprof · 2019-03-28T19:38:47Z

src/timer.rs

+
+                /// Releases the TIM Peripheral and clocks
+                pub fn release(self) -> $TIMX {
+                    self.tim.cr1.modify(|_, w| w.cen().clear_bit());


Can't we simply call stop() from above?

therealprof

Please also add an entry to CHANGELOG.md.

@TheZoq2 Any comments from your side?

TheZoq2 · 2019-03-28T19:48:29Z

Looks good to me

thalesfragoso · 2019-03-28T19:55:51Z

I updated the Changelog, don't know if I over-explained it, feel free to modify.

TheZoq2 · 2019-03-28T19:59:55Z

Looks good to me, though I did think of another improvement which would be nice. Could you replace the already existing calls to uif.clear_bit() with the new clear_uif function?

Also, I would argue that calling that function clear_update_interrupt might be more intuitive than clear_uif

thalesfragoso · 2019-03-28T20:03:01Z

Looks good to me, though I did think of another improvement which would be nice. Could you replace the already existing calls to uif.clear_bit() with the new clear_uif function?

Also, I would argue that calling that function clear_update_interrupt might be more intuitive than clear_uif

How about clear_update_interrupt_flag ? Flag is a very common word and would be the first one I would search for in a function to do this procedure.

TheZoq2 · 2019-03-28T20:03:44Z

Yea, that is probably even better!

TeXitoi

Great work, will use it as soon as it's published!

TeXitoi · 2019-03-28T22:39:43Z

CHANGELOG.md

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Add feature for using STM32F101 chip
 - Add gpio pins corresponding to LQFP-100 package
 - Implement `core::fmt::Write` for `serial::Tx`
+- Add methods `stop`, `release` and `clear_uif` to `Timer` (`clear_uif` only on `Timer<TIMX>`)


TeXitoi · 2019-03-28T22:41:39Z

src/timer.rs

+
+    /// Releases the SYST
+    pub fn release(mut self) -> SYST {
+        self.tim.disable_counter();


minor: better to self.stop() as any bug on stop must be propagated here.

thalesfragoso · 2019-03-29T01:00:09Z

I will modify the code to use the new methods as proposed and I think I will write an example for the Timer interrupt with rtfm and add it to the PR.
I will probably do it over the weekend.

thalesfragoso · 2019-03-29T15:24:24Z

I ended up getting some spare time today, so there you go. Sorry for the bunch of commits, but I guess you gonna squash them all anyways.
I tested the example on a bluepill and it worked as expected.

TheZoq2 · 2019-03-29T15:30:07Z

Thanks, looks good to me!

@therealprof What's our policy on adding dependencies for examples, #2 didn't really come to a conclusion. I believe that rtfm was originally removed because it didn't compile on stable, but that's solved now. I'm fine with having rtfm as a dependency since it's a pretty common crate in the embedded rust world, but at the same time, adding a bunch of dependencies might be a bad idea for compilation times and security.

thalesfragoso · 2019-03-29T15:35:02Z

I kinda understand the dependencies for examples thing, but in the present scenario I would think that it is somewhat bad to push newcomers to implement interrupts (with handlers and stuff) without rtfm.

therealprof · 2019-03-29T15:37:30Z

@TheZoq2 I'm generally not too hot on having examples in this crate (apart from in the docs). Ideally we should have BSP crates which go nuts on examples. I do have a a basic https://github.com/therealprof/nucleo-f103rb crate, if people are interested I could make this free for all, or just create a new blue pill crate.

However for now it's okay for me to just add back the RTFM dependency.

therealprof · 2019-03-29T15:40:26Z

@thalesfragoso The problem is that a universal HAL crate is somewhat tricky to use anyway, especially for beginners; so instead of dragging newcomers into examples in here we really should demonstrate how to get properly started with your own projects by having a proper BSP crate.

TheZoq2 · 2019-03-29T15:44:00Z

Yea, eventually we should probably take care of this example stuff, and a BSP with examples does sound like a good idea. I do agree that having a RTFM example for interrupts is much better than one without RTFM :)

I guess all that's left to do then is to merge this, thanks for the help! @thalesfragoso

TeXitoi · 2019-03-29T15:43:08Z

examples/timer-interrupt-rtfm.rs

+            resources.LED.set_high();
+            *resources.LED_STATE = false;
+        } else {
+            resources.LED.set_low();


You can call resources.LED.toggle(); and remove LED_STATE

Does PC13<Output<MODE>> have toggle ? I remember looking at the docs when I was writing other code and could not find it.

I've used it in the past https://github.com/TeXitoi/blue-pill-quickstart/blob/master/src/main.rs#L41

TeXitoi · 2019-03-29T15:43:30Z

examples/timer-interrupt-rtfm.rs

+
+        loop {
+            // Waits for interrupt
+            wfi();


You can just not define an idle() function.

Does it automatically call wfi() ?

https://japaric.github.io/cortex-m-rtfm/book/en/by-example/app.html#idle

When no idle function is declared, the runtime sets the SLEEPONEXIT bit and then sends the microcontroller to sleep after running init.

therealprof reviewed Mar 28, 2019

View reviewed changes

TeXitoi reviewed Mar 28, 2019

View reviewed changes

thalesfragoso and others added 7 commits March 28, 2019 22:13

Add more methods to Timer

4a33aa6

Methods also added to SYST

f77c2f9

Don't return Clocks on Timer release

9cd0178

Update timer.rs

8b96e16

Update CHANGELOG.md

f764cb8

Minor refactoring

1b37402

Add Timer interrupt example and minor changes

ba8717b

thalesfragoso force-pushed the timer-upgrade branch from 464309c to ba8717b Compare March 29, 2019 15:22

TheZoq2 merged commit 9f95056 into stm32-rs:master Mar 29, 2019

TeXitoi reviewed Mar 29, 2019

View reviewed changes

thalesfragoso deleted the timer-upgrade branch June 11, 2019 01:26

Adds some simple methods to Timer. #52

Adds some simple methods to Timer. #52

Uh oh!

Conversation

thalesfragoso commented Mar 28, 2019

Uh oh!

therealprof commented Mar 28, 2019

Uh oh!

thalesfragoso commented Mar 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

therealprof left a comment

Choose a reason for hiding this comment

Uh oh!

TheZoq2 commented Mar 28, 2019

Uh oh!

thalesfragoso commented Mar 28, 2019

Uh oh!

TheZoq2 commented Mar 28, 2019

Uh oh!

thalesfragoso commented Mar 28, 2019

Uh oh!

TheZoq2 commented Mar 28, 2019

Uh oh!

TeXitoi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thalesfragoso commented Mar 29, 2019

Uh oh!

thalesfragoso commented Mar 29, 2019

Uh oh!

TheZoq2 commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thalesfragoso commented Mar 29, 2019

Uh oh!

therealprof commented Mar 29, 2019

Uh oh!

therealprof commented Mar 29, 2019

Uh oh!

TheZoq2 commented Mar 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thalesfragoso Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheZoq2 commented Mar 29, 2019 •

edited

Loading

thalesfragoso Mar 29, 2019 •

edited

Loading