Skip to content

revise peripheral API #69

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

Merged
merged 3 commits into from
Jan 11, 2018
Merged

revise peripheral API #69

merged 3 commits into from
Jan 11, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Dec 22, 2017

This PR changes the signature of many of the high level methods available on peripherals like
NVIC::get_priority. Additionally some instance methods have been turned into static methods. The
following guidelines have been used to apply the changes:

  • If the method body contains a single, atomic read operation with no side effects (e.g. the read
    operation clears one of the bits of the register): the signature changed to make the method
    static, i.e. &self was removed from the signature.

  • If the method involves writing to or a RMW operation on a register: the signature changed to take
    the singleton by &mut self reference.

  • If the method involves only read operations where at least one of them modifies the value
    of a register: the signature changed to take the singleton by &mut self reference.

The rationale for this last guideline is that using &self, instead of &mut self, lets the user
(unintentionally) break abstractions in the presence of generators. Example below:

let peripherals = Peripherals::take().unwrap();
let syst = &peripherals.SYST;

// tasks
let mut a = || {
    loop {
        // yielding "busy wait"
        while !a.has_wrapped() {
            yield;
        }

        // do stuff
    }
};

let mut b = || {
    // ..

    // *NOTE* the problem is in the line below: this `is_counter_enabled` method reads the CSR
    // register and that read operation clears the COUNTFLAG bit of the register (if set), which is
    // the bit the `has_wrapped` method checks for.
    if syst.is_counter_enabled() {
        // ..
    }

    // ..
};

One more guideline was considered but the required conditions didn't apply to any of the existing
methods:

  • If the method involves only non side effectful, non necessarily atomic read operations: the
    signature of the method should remain as &self.

The rationale for this guideline is that a static method (no self argument) wouldn't be
appropriate because that can result in a torn read if the read operation can be preempted by some
context that modifies the register.

In any case, this last guideline doesn't seem to apply well to the peripherals structs exposed by
this crate because they deref to a RegisterBlock that allows mutation through a &self
reference. When these two properties (the guideline and Deref<Target=RegisterBlock>) are mixed
the user can potentially break abstractions using generators (as shown in the syst example).

cc @hannobraun
closes #67

@japaric japaric added this to the v0.4.0 milestone Dec 22, 2017
@homunkulus
Copy link
Contributor

☔ The latest upstream changes (presumably #71) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have time for a full review. I only glanced over the code changes and didn't double-check your unsafe notes against the technicals manuals. Other than that and the broken examples, this looks good to me!

//! let peripherals = Peripherals::take();
//! peripherals.dwt.enable_cycle_counter();
//! }
//! ```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is broken. Fixed version:

//! ``` no_run
//! use cortex_m::peripheral::Peripherals;
//!
//! fn main() {
//!     let mut peripherals = Peripherals::take().unwrap();
//!     peripherals.DWT.enable_cycle_counter();
//! }
//! ```

//! // but this method can be called without a DWT instance
//! let cyccnt = DWT::cyccnt();
//! }
//! ```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is also broken. Fixed version:

//! ``` no_run
//! use cortex_m::peripheral::{
//!     DWT,
//!     Peripherals,
//! };
//!
//! fn main() {
//!     {
//!         let mut peripherals = Peripherals::take().unwrap();
//!         peripherals.DWT.enable_cycle_counter();
//!     } // all the peripheral singletons are destroyed here
//!
//!     // but this method can be called without a DWT instance
//!     let cyccnt = DWT::get_cycle_count();
//! }
//! ```

//! // actually safe because this is an atomic read with no side effects
//! let cyccnt = unsafe { (*DWT::ptr()).cyccnt.read() };
//! }
//! ```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also broken. Fixed version:

//! ``` no_run
//! use cortex_m::peripheral::{
//!     DWT,
//!     Peripherals,
//! };
//!
//! fn main() {
//!     {
//!         let mut peripherals = Peripherals::take().unwrap();
//!         peripherals.DWT.enable_cycle_counter();
//!     } // all the peripheral singletons are destroyed here
//!
//!     // actually safe because this is an atomic read with no side effects
//!     let cyccnt = unsafe { (*DWT::ptr()).cyccnt.read() };
//! }
//! ```

/// value (e.g. `16`) has higher priority than a larger value (e.g. `32`).
pub fn get_priority<I>(&self, interrupt: I) -> u8
/// *NOTE* NVIC encodes priority in the highest bits of a byte so values like `1` and `2` map
/// the same priority. Also for NVIC priorities, a lower value (e.g. `16`) has higher priority
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "map to the same priority".

@japaric
Copy link
Member Author

japaric commented Jan 11, 2018

@hannobraun Thanks for the review!

@homunkulus r+

@homunkulus
Copy link
Contributor

📌 Commit 5a19c39 has been approved by japaric

japaric pushed a commit that referenced this pull request Jan 11, 2018
revise peripheral API

This PR changes the signature of many of the high level methods available on peripherals like
`NVIC::get_priority`. Additionally some instance methods have been turned into static methods. The
following guidelines have been used to apply the changes:

- If the method body contains a single, atomic read operation with *no* side effects (e.g. the read
  operation clears one of the bits of the register): the signature changed to make the method
  static, i.e. `&self` was removed from the signature.

- If the method involves writing to or a RMW operation on a register: the signature changed to take
  the singleton by `&mut self` reference.

- If the method involves only read operations where at least one of them modifies the value
  of a register: the signature changed to take the singleton by `&mut self` reference.

The rationale for this last guideline is that using `&self`, instead of `&mut self`, lets the user
(unintentionally) break abstractions in the presence of generators. Example below:

``` rust
let peripherals = Peripherals::take().unwrap();
let syst = &peripherals.SYST;

// tasks
let mut a = || {
    loop {
        // yielding "busy wait"
        while !a.has_wrapped() {
            yield;
        }

        // do stuff
    }
};

let mut b = || {
    // ..

    // *NOTE* the problem is in the line below: this `is_counter_enabled` method reads the CSR
    // register and that read operation clears the COUNTFLAG bit of the register (if set), which is
    // the bit the `has_wrapped` method checks for.
    if syst.is_counter_enabled() {
        // ..
    }

    // ..
};
```

One more guideline was considered but the required conditions didn't apply to any of the existing
methods:

- If the method involves only non side effectful, non necessarily atomic read operations: the
  signature of the method should remain as `&self`.

The rationale for this guideline is that a static method (no `self` argument) wouldn't be
appropriate because that can result in a torn read if the read operation can be preempted by some
context that modifies the register.

In any case, this last guideline doesn't seem to apply well to the peripherals structs exposed by
this crate because they *deref* to a `RegisterBlock` that allows mutation through a `&self`
reference. When these two properties (the guideline and `Deref<Target=RegisterBlock>`) are mixed
the user can potentially break abstractions using generators (as shown in the `syst` example).

cc @hannobraun
closes #67
@homunkulus
Copy link
Contributor

⌛ Testing commit 5a19c39 with merge 08c1320...

@homunkulus
Copy link
Contributor

💔 Test failed - status-travis

@japaric
Copy link
Member Author

japaric commented Jan 11, 2018

@homunkulus r+

@homunkulus
Copy link
Contributor

📌 Commit 47623ce has been approved by japaric

@homunkulus
Copy link
Contributor

⌛ Testing commit 47623ce with merge 024527b...

japaric pushed a commit that referenced this pull request Jan 11, 2018
revise peripheral API

This PR changes the signature of many of the high level methods available on peripherals like
`NVIC::get_priority`. Additionally some instance methods have been turned into static methods. The
following guidelines have been used to apply the changes:

- If the method body contains a single, atomic read operation with *no* side effects (e.g. the read
  operation clears one of the bits of the register): the signature changed to make the method
  static, i.e. `&self` was removed from the signature.

- If the method involves writing to or a RMW operation on a register: the signature changed to take
  the singleton by `&mut self` reference.

- If the method involves only read operations where at least one of them modifies the value
  of a register: the signature changed to take the singleton by `&mut self` reference.

The rationale for this last guideline is that using `&self`, instead of `&mut self`, lets the user
(unintentionally) break abstractions in the presence of generators. Example below:

``` rust
let peripherals = Peripherals::take().unwrap();
let syst = &peripherals.SYST;

// tasks
let mut a = || {
    loop {
        // yielding "busy wait"
        while !a.has_wrapped() {
            yield;
        }

        // do stuff
    }
};

let mut b = || {
    // ..

    // *NOTE* the problem is in the line below: this `is_counter_enabled` method reads the CSR
    // register and that read operation clears the COUNTFLAG bit of the register (if set), which is
    // the bit the `has_wrapped` method checks for.
    if syst.is_counter_enabled() {
        // ..
    }

    // ..
};
```

One more guideline was considered but the required conditions didn't apply to any of the existing
methods:

- If the method involves only non side effectful, non necessarily atomic read operations: the
  signature of the method should remain as `&self`.

The rationale for this guideline is that a static method (no `self` argument) wouldn't be
appropriate because that can result in a torn read if the read operation can be preempted by some
context that modifies the register.

In any case, this last guideline doesn't seem to apply well to the peripherals structs exposed by
this crate because they *deref* to a `RegisterBlock` that allows mutation through a `&self`
reference. When these two properties (the guideline and `Deref<Target=RegisterBlock>`) are mixed
the user can potentially break abstractions using generators (as shown in the `syst` example).

cc @hannobraun
closes #67
@homunkulus
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: japaric
Pushing 024527b to master...

@homunkulus homunkulus merged commit 47623ce into master Jan 11, 2018
@japaric japaric deleted the revise-api branch January 11, 2018 18:22
adamgreig pushed a commit that referenced this pull request Jan 12, 2022
69: compile on stable r=japaric a=japaric

with these changes this crate compiles on stable

Co-authored-by: Jorge Aparicio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update method signatures to better match scoped singletons
3 participants