Merge #255

255: more monotonic timer docs r=nils-grepit a=japaric

covers

- initialization and configuration of the timer; this is now a responsibility of
  the application author
- correctness of `Monotonic::now()` in `#[init]`
- safety of `Monotonic::reset()`

closes #251
cc @jonas-schievink

(EDIT: yay, pull request number 0xFF)

Co-authored-by: Jorge Aparicio <jorge@japaric.io>

7 files changed, 54 insertions, 23 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 086bb0ef..5c858c56 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,7 +5,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
 
-## v0.5.0 - 2019-09-?? (currently in beta pre-release)
+## v0.5.0 - 2019-??-?? (currently in beta pre-release)
 
 ### Added
 
@@ -45,7 +45,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
   Cargo features are enabled.
 
 - [breaking-change][] the monotonic timer used to implement the `schedule` API
-  is now user configurable via the `#[app(monotonic = ..)]` argument.
+  is now user configurable via the `#[app(monotonic = ..)]` argument. IMPORTANT:
+  it is now the responsibility of the application author to configure and
+  initialize the chosen `monotonic` timer during the `#[init]` phase.
 
 - [breaking-change][] the `peripherals` field is not include in `init::Context`
   by default. One must opt-in using the `#[app(peripherals = ..)]` argument.
diff --git a/book/en/src/by-example/timer-queue.md b/book/en/src/by-example/timer-queue.md
index 7c8be382..94d22819 100644
--- a/book/en/src/by-example/timer-queue.md
+++ b/book/en/src/by-example/timer-queue.md
@@ -34,6 +34,10 @@ first appear in the `schedule` argument of the context attribute. When
 scheduling a task the (user-defined) `Instant` at which the task should be
 executed must be passed as the first argument of the `schedule` invocation.
 
+Additionally, the chosen `monotonic` timer must be configured and initialized
+during the `#[init]` phase. Note that this is *also* the case if you choose to
+use the `CYCCNT` provided by the `cortex-m-rtfm` crate.
+
 The example below schedules two tasks from `init`: `foo` and `bar`. `foo` is
 scheduled to run 8 million clock cycles in the future. Next, `bar` is scheduled
 to run 4 million clock cycles in the future. Thus `bar` runs before `foo` since
diff --git a/examples/baseline.rs b/examples/baseline.rs
index b7144dd1..df0ff9a4 100644
--- a/examples/baseline.rs
+++ b/examples/baseline.rs
@@ -14,6 +14,8 @@ use panic_semihosting as _;
 const APP: () = {
     #[init(spawn = [foo])]
     fn init(cx: init::Context) {
+        // omitted: initialization of `CYCCNT`
+
         hprintln!("init(baseline = {:?})", cx.start).unwrap();
 
         // `foo` inherits the baseline of `init`: `Instant(0)`
diff --git a/examples/periodic.rs b/examples/periodic.rs
index ec110e11..dca0ad56 100644
--- a/examples/periodic.rs
+++ b/examples/periodic.rs
@@ -16,6 +16,8 @@ const PERIOD: u32 = 8_000_000;
 const APP: () = {
     #[init(schedule = [foo])]
     fn init(cx: init::Context) {
+        // omitted: initialization of `CYCCNT`
+
         cx.schedule.foo(Instant::now() + PERIOD.cycles()).unwrap();
     }
 
diff --git a/examples/schedule.rs b/examples/schedule.rs
index 27d3bd1f..97818e36 100644
--- a/examples/schedule.rs
+++ b/examples/schedule.rs
@@ -1,6 +1,5 @@
 //! examples/schedule.rs
 
-#![deny(unsafe_code)]
 #![deny(warnings)]
 #![no_main]
 #![no_std]
@@ -13,8 +12,15 @@ use rtfm::cyccnt::{Instant, U32Ext as _};
 #[rtfm::app(device = lm3s6965, monotonic = rtfm::cyccnt::CYCCNT)]
 const APP: () = {
     #[init(schedule = [foo, bar])]
-    fn init(cx: init::Context) {
-        let now = Instant::now();
+    fn init(mut cx: init::Context) {
+        // Initialize (enable) the monotonic timer (CYCCNT)
+        cx.core.DCB.enable_trace();
+        // required on devices that software lock the DWT (e.g. STM32F7)
+        unsafe { cx.core.DWT.lar.write(0xC5ACCE55) }
+        cx.core.DWT.enable_cycle_counter();
+
+        // semantically, the monotonic timer is frozen at time "zero" during `init`
+        let now = cx.start; // the start time of the system
 
         hprintln!("init @ {:?}", now).unwrap();
 
diff --git a/src/cyccnt.rs b/src/cyccnt.rs
index c8a1b7ee..86969cb1 100644
--- a/src/cyccnt.rs
+++ b/src/cyccnt.rs
@@ -1,11 +1,9 @@
-//! Data Watchpoint Trace (DWT) unit's CYCle CouNTer
+//! Data Watchpoint Trace (DWT) unit's CYCle CouNTer (CYCCNT)
 
 use core::{
     cmp::Ordering,
     convert::{Infallible, TryInto},
-    fmt,
-    marker::PhantomData,
-    ops,
+    fmt, ops,
 };
 
 use cortex_m::peripheral::DWT;
@@ -16,25 +14,25 @@ use crate::Fraction;
 ///
 /// This data type is only available on ARMv7-M
 ///
-/// Note that this value is tied to the CYCCNT of one core and that sending it a different core
-/// makes it lose its meaning -- each Cortex-M core has its own CYCCNT counter and these are usually
-/// unsynchronized and they may even be running at different frequencies.
+/// # Correctness
+///
+/// Adding or subtracting a `Duration` of more than `(1 << 31)` cycles to an `Instant` effectively
+/// makes it "wrap around" and creates an incorrect value. This is also true if the operation is
+/// done in steps, e.g. `(instant + dur) + dur` where `dur` is `(1 << 30)` ticks.
+///
+/// In multi-core contexts: this value is tied to the CYCCNT of *one* core so sending it a different
+/// core makes it lose its meaning -- each Cortex-M core has its own CYCCNT counter and these are
+/// usually unsynchronized and may even be running at different frequencies.
 #[derive(Clone, Copy, Eq, PartialEq)]
 pub struct Instant {
     inner: i32,
-    _not_send_or_sync: PhantomData<*mut ()>,
 }
 
-unsafe impl Sync for Instant {}
-
-unsafe impl Send for Instant {}
-
 impl Instant {
     /// Returns an instant corresponding to "now"
     pub fn now() -> Self {
         Instant {
             inner: DWT::get_cycle_count() as i32,
-            _not_send_or_sync: PhantomData,
         }
     }
 
@@ -61,6 +59,8 @@ impl fmt::Debug for Instant {
 
 impl ops::AddAssign<Duration> for Instant {
     fn add_assign(&mut self, dur: Duration) {
+        // NOTE this is a debug assertion because there's no foolproof way to detect a wrap around;
+        // the user may write `(instant + dur) + dur` where `dur` is `(1<<31)-1` ticks.
         debug_assert!(dur.inner < (1 << 31));
         self.inner = self.inner.wrapping_add(dur.inner as i32);
     }
@@ -77,7 +77,7 @@ impl ops::Add<Duration> for Instant {
 
 impl ops::SubAssign<Duration> for Instant {
     fn sub_assign(&mut self, dur: Duration) {
-        // XXX should this be a non-debug assertion?
+        // NOTE see the NOTE in `<Instant as AddAssign<Duration>>::add_assign`
         debug_assert!(dur.inner < (1 << 31));
         self.inner = self.inner.wrapping_sub(dur.inner as i32);
     }
@@ -115,6 +115,12 @@ impl PartialOrd for Instant {
 /// A `Duration` type to represent a span of time.
 ///
 /// This data type is only available on ARMv7-M
+///
+/// # Correctness
+///
+/// This type is *not* appropriate for representing time spans in the order of, or larger than,
+/// seconds because it can hold a maximum of `(1 << 31)` "ticks" where each tick is the inverse of
+/// the CPU frequency, which usually is dozens of MHz.
 #[derive(Clone, Copy, Default, Eq, Ord, PartialEq, PartialOrd)]
 pub struct Duration {
     inner: u32,
@@ -208,9 +214,6 @@ impl crate::Monotonic for CYCCNT {
     }
 
     fn zero() -> Instant {
-        Instant {
-            inner: 0,
-            _not_send_or_sync: PhantomData,
-        }
+        Instant { inner: 0 }
     }
 }
diff --git a/src/lib.rs b/src/lib.rs
index 959c9b7b..d661c54f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -138,9 +138,21 @@ pub trait Monotonic {
     fn ratio() -> Fraction;
 
     /// Returns the current time
+    ///
+    /// # Correctness
+    ///
+    /// This function is *allowed* to return nonsensical values if called before `reset` is invoked
+    /// by the runtime. Therefore application authors should *not* call this function during the
+    /// `#[init]` phase.
     fn now() -> Self::Instant;
 
     /// Resets the counter to *zero*
+    ///
+    /// # Safety
+    ///
+    /// This function will be called *exactly once* by the RTFM runtime after `#[init]` returns and
+    /// before tasks can start; this is also the case in multi-core applications. User code must
+    /// *never* call this function.
     unsafe fn reset();
 
     /// A `Self::Instant` that represents a count of *zero*