[RFC] Enable/disable functions

[hannes-hochreiner]

Background: I am working on a battery powered sensor based on the nRF52810. Hence, I am interested in having the chip consume the lowest amount of energy possible while it is sleeping.

Most peripherals start off disabled (in the nRF52810 all but the radio) and are enabled when the HAL instances are initialized. However, there is no way of (temporarily) disabling the function. Even when the structure is dropped, the peripheral is not disabled. I see three different options on how to move forward on this topic:

• enable peripheral on initialization, but add functions for disabling and re-enabling a peripheral (e.g. Enabling and disabling the TWIM instance #266 for TWIM)
• leave peripheral disabled on initialization and add "enable" and "disable" functions
• leave everything as is

Bonus questions:

• should the pin state also be changed?
• should the peripheral be disabled when the instance is dropped?

I'd be happy to work on these improvements and I would like to get some guidance how you would like to handle these topics (if at all). Please let me know what you think.

[jonas-schievink]

enable peripheral on initialization, but add functions for disabling and re-enabling a peripheral (e.g. #266 for TWIM)

I'd be in favor of this.

I think we shouldn't require the user to call enable, it's too easy to forget that.

should the pin state also be changed?

Since the purpose of the feature is reducing current draw, this seems like a good idea (but should be documented).

[hannes-hochreiner]

Review of the peripherals:

peripheral	ctor name	enabled in ctor	fn enable	fn disable	pins	fn free
ADC	new	true	false	false	configured in read	false
CCM	init	true	false	false	n/a	false
CLOCKS	new	special case	special case	special case	n/a	false
COMP	new	false	true	true	several fns for configuration	true
DELAY	new	n/a	n/a	n/a	n/a	true
ECB	init	n/a	n/a	n/a	n/a	false
GPIO	new	n/a	n/a	n/a	n/a	false
GPIOTE	new	n/a	n/a	n/a	n/a	true
I2S	new, new_controller, new_peripheral	true	true	true	provided in configured state	true (calls disable; pins untouched)
LPCOMP	new	false	true	true	several fns for configuration	true
PWM	new	false	true	true	set_output_pin	true
QDEC	new	false	true	true	configured in new	true
RNG	new	n/a	n/a	n/a	n/a	false
RTC	new	n/a	n/a	n/a	n/a	release
SAADC	new	true	false	false	n/a	false
SPI	new	true	false	false	provided in configured state	true
SPIM	new	true	false	false	provided in configured state	true
SPIS	new	true	true	true	provided in configured state	true
TEMP	new	n/a	n/a	n/a	n/a	false
TIMER	new, one_shot, periodic	n/a	n/a	n/a	n/a	true
TWI	new	true	false	false	configured in new	true
TWIM	new	true	true	true	configured in new	true
TWIS	new	true	true	true	configured in new	true
UART	new	true	false	false	provided in configured state	true
UARTE	new	true	false	false	provided in configured state	true
UICR	new	n/a	n/a	n/a	n/a	true
WDT	try_new	n/a	handled through type parameters	handled through type parameters	n/a	release

At the moment, I don't see a preferred model.

[hannes-hochreiner]

Thinking about it for a while, there are two models I could see yielding a structure that is intuitive to me:

Model 1

The HAL peripheral instance is kept around for a long time and can be enabled and disabled. That would require to consistently implement enable and disable (where applicable). It would also require the peripheral to change the pin state for maximum energy savings.

PROS

• Peripherals can be configured once and then switched on and off easily.

CONS

• More logic required to make sure that the peripheral is in the right state for an action (e.g. avoid write when the peripheral is disabled). I think the idiomatic way of doing this would be to implement type parameters (e.g. TWIM) to allow checking at compile time.
• The peripheral has to manipulate the pins and has to make sure they are in the right state when free is called.

Model 2

The HAL peripheral instance is only kept for the time it is actually used (e.g. to communicate). In the case of a system that sleeps most of the time, this would mean that the instance is created when the system wakes up and dropped before the system goes back to sleep.

PROS

• This approach is from my understanding more in line with the embedded_hal way of thinking.
• Pins can be configured as needed, provided to the instance, and after dropping the instance they can be reconfigured. I.e. the peripheral instance does not have to worry about the pins.
• enable and disable functions are not required. The instance in enabled on construction and disabled when free or drop are called.

CONS

• Instances need to be configured each time the get enabled.

Any thoughts?

[timokroeger]

Thank you for the table, that’s a really nice overview!
I’m also developing a battery powered device and struggling with the same issues.

I agree with all your points and would like to extend on some :)

Model 1 is the most intuitive one to me. Good usability (without typestate) and you can get results quickly.
The big downside is, that it is easy to misuse a peripheral when it is disabled.
As you proposed, we can leverage typestate to prevent misuse. Unfortunately typestate does not play well with RTIC which is commonly used with embedded rust (there are workarounds though: rtic-rs/rtic#345)

Model 2 has the same shortcomings when used with RTIC because resources are only available as mutable borrow. From usability perspective Model 2 is usually more verbose (new() and free() everywhere).

Because I prefer easy to use APIs, I would go with Model 1 with assertions instead of typestate. Typestate tends to confuse newcomers.

Not in the scope of nrf-hal but might be of interest: Model 3: Async where peripherals enable and disable themselves as required (currently exploring that at: https://github.com/akiles/embassy/pull/14, https://github.com/akiles/embassy/blob/master/embassy-nrf/src/uarte.rs)

[hannobraun]

I prefer model 1, because believe that HALs should provide a single Peripherals struct with all peripheral APIs in it, like the PAC does (see explanation and practical example). If we did this, we obviously can't enable anything by default in a constructor.

(I always planned to take the HAL in that direction, but didn't get that far during my Oxidize 2019 refactoring, and never again found the time to pick it back up.)

I also prefer using type state to track enabling/disabling (and other configuration, as appropriate), because it allows for APIs that eliminate the potential for user error, and because it's potentially zero-cost (unlike any form of runtime checking). I acknowledge that this is harder to learn and doesn't play well with RTIC, but I believe type state is such a useful tool (in this case, and in general) that making the API slightly harder to learn is a worthwhile trade-off (and a good investment for those learning).

As for RTIC, as @timokroeger mentioned, there are (ugly and cumbersome) workarounds for right now. For the future, I think it would be great to find solutions that make RTIC play better with type state. I think indirectly putting some pressure on it by insisting on writing APIs based on type state, rather than preemptively accepting that type state is not viable, might be a better strategy to assist in achieving that 🙂

[hannes-hochreiner]

Thank you all for your comments. I think it is fair to say that there is a consensus to go with Model 1. As for the implementation, it seems that we have two main options: runtime checks or type states.

I prefer the type state solution for the reasons pointed out by @hannobraun. However, I can see that an API based on type states might be harder to learn for people new to Rust and that some parts of the ecosystem are not ready (yet).

@jonas-schievink and @hannobraun: As members of the nRF Rust organization, could you share some insight on what is planned for the future of this crate or how the future direction of the API is decided? I feel that going with type states would require a considerable rework of the crate. Hence, I would only make a push in that direction, if there is some agreement among the stewards of this crate that this is the desired future direction.

[jonas-schievink]

I think we don't really need either.

Disabling peripherals is something I don't expect too many people to do (only if you care about power usage), so always paying for the runtime checks is undesirable.

Typestate complicates both the implementation and usage of most types provided by this crate, and I don't think it's fair to pressure the RTIC developers to making it easier to use by using it so aggressively (encoding state in the typesystem fundamentally makes it impossible to store a value in a variable and change the type-level state later on, so I don't really see what they could do here – if typestate can change dynamically you must lift it to a runtime value, for example by putting the type into an enum).

The simplest possible solution would be to add enable and disable methods to all peripherals where they make sense, and maybe make them put GPIOs in the least energy-intensive state if needed. It seems reasonable to make them "advanced" APIs and to make it the user's responsibility to call enable before using the peripheral again after they called disable. If this turns out to be too difficult to use in practice, we can opt for a more complicated implementation (starting, for example, by adding runtime checks behind #[cfg(debug_assertions)]).

[hannobraun]

Before I answer the comments, I'd like to clarify my role here: Yes, I'm a member of this org, yes I'm officially a maintainer here, but I haven't really done anything in forever. Please don't put any value in my opinions, unless you are convinced by my arguments. I certainly won't try to veto any decisions made here, even if I disagree with them.

As for future plans, I've shared some of the plans I had for this HAL in my comment above, but since I never implemented them am unlikely to do so going forward, they are not any better than the plans anyone else here might come up with.

@hannes-hochreiner

I feel that going with type states would require a considerable rework of the crate. Hence, I would only make a push in that direction, if there is some agreement among the stewards of this crate that this is the desired future direction.

I'd say being able to enable and disable peripherals consistently would be an improvement, no matter what future vision we might or might not agree on. I certainly wouldn't object to a PR that introduced this capability, even if it doesn't follow my preferred style.

@jonas-schievink

Typestate complicates both the implementation and usage of most types provided by this crate, and I don't think it's fair to pressure the RTIC developers to making it easier to use by using it so aggressively

Okay, first off I'd like to clarify my use of the word "pressure" since that might be misunderstood. There are certainly many ways to pressure the RTIC developers that would be unfair, rude, or plain wrong. But I'm not talking about calling them at home every night, or hounding them on their issue tracker.

We are certainly allowed to make the technical decisions that we think are right, even if those decisions don't play perfectly well with RTIC. Just like the RTIC developers were fully within their rights to make technical decisions that don't play well with type state, which now puts pressure on us not to use it (and there's certainly nothing unfair about that).

And I object to the word "aggressively" in this context. You might reasonably disagree with the use of type state for certain things, but there's nothing inherently aggressive about using it to track peripheral state.

(encoding state in the typesystem fundamentally makes it impossible to store a value in a variable and change the type-level state later on, so I don't really see what they could do here – if typestate can change dynamically you must lift it to a runtime value, for example by putting the type into an enum).

Storing types using type state in a variable and changing them is not fundamentally impossible; you mention enums in the same sentence 🙂

And if we agreed that using type state to track peripheral state were desirable, we could even provide these enums as part of the peripheral API. (And it might even be possible to use macro magic to generate them automatically, but I'm trailing off topic.)

As for what RTIC could do, I made some suggestions in the issue that @timokroeger linked above (rtic-rs/rtic#345).

The simplest possible solution would be to add enable and disable methods to all peripherals where they make sense, and maybe make them put GPIOs in the least energy-intensive state if needed. It seems reasonable to make them "advanced" APIs and to make it the user's responsibility to call enable before using the peripheral again after they called disable. If this turns out to be too difficult to use in practice, we can opt for a more complicated implementation (starting, for example, by adding runtime checks behind #[cfg(debug_assertions)]).

As I said above, I won't object to anyone adding those enable/disable methods (even without type state or checks), as I think having this capability would be an improvement either way. But I definitely think "let the user worry about it" is not a good solution. I've spent way too much time in my life tracking down problems that were caused by really simple mistakes like forgetting to enable something in the right place.

I also object to relying on debug assertions. That's fine for relatively simple programs, but as soon as your program gets non-trivial, it suddenly becomes likely to miss a bug in a rarely called branch, causing an issue in production that could have easily been prevented at compile-time, if type state were used.

I fully acknowledge that everything is a trade-off, and we can reasonably come to different conclusions. Due to my experience being stupid and making simple mistakes that took me way too much time to find, I'm firmly in the "make it reliable, and be willing to add (compile-time) complexity to do that" camp.

[Dirbaio]

I do object to adding enable/disable methods. Enabling/disabling a peripheral at runtime is already possible like this:

struct Resources {
    uarte: UARTE0,
    pins: uarte::Pins,
}

struct Foo {
    resources: Option<Resources>
}

impl Foo{
    fn do_something_with_uart(&mut self, ...) {
        // Take the resources, replacing with None
        // unwrap will panic if resources already taken
        let resources = self.resources.take().unwrap(); 

        // Construct the uart
        let mut uarte = uarte::Uarte::new(resources.uarte, resources.pins, Self::PARITY, Self::BAUDRATE);

        // Use it
        uarte.send(...).await;

        // Deconstruct it
        let (uarte, pins) = uarte.free();

        // Put back the resources
        self.resources = Some(Resources { uarte, pins }));
    }
}

The overhead from this is quite small. If it's unacceptable to the user, they can unsafely conjure the uart and pins from thin air with unsafe each time they want to use the uart too.

What is missing is having free properly deinitialize and disable the peripherals, which is something we should do anyway.

[jonas-schievink]

The use case supported by enable/disable is to shut down peripherals for saving power. This does not work well if it requires ownership of the peripheral (same issue as with typestate).

I think it is a good idea to make free undo everything new does though, so 👍 to making it disable the peripheral if new enables it.

[Dirbaio]

If user wants to store the peripheral somewhere such as a RTIC resource, they could do this:

enum DisableableUart {
    Disabled(UARTE0, uarte::Pins),
    Enabled(Uarte<UARTE0, ..>),
}

IMO this is much nicer than adding enable/disable.

• Makes it very clear to the user it's their responsibility to check whether it's enabled/disabled on use.
• Makes it impossible to call eg read() if uart is disabled.
• Adds no extra runtime complexity to the HAL for users that don't need it.

[hannobraun]

@Dirbaio

What is missing is having free properly deinitialize and disable the peripherals, which is something we should do anyway.

I always imagined that one use case for free-type methods is to initialize the peripheral using the HAL API, then use the PAC API as an escape hatch to use a feature that the HAL doesn't yet support. No idea if anyone is actually doing that.

[timokroeger]

I always imagined that one use case for free-type methods is to initialize the peripheral using the HAL API, then use the PAC API as an escape hatch to use a feature that the HAL doesn't yet support. No idea if anyone is actually doing that.

I certainly have done that and I think we should have a way to access the PAC. Maybe with a different name and marked as unsafe: unsafe fn raw_parts() -> (UARTE0, uarte::Pins).

[Dirbaio]

I am guilty of that too, hehe.

However if you're willing to use unsafe you can already do this with Peripherals::steal() or with let u: UARTE0 = mem::transmute(());. I'm not sure if we should add a new method to do that.

[hannes-hochreiner]

If user wants to store the peripheral somewhere such as a RTIC resource, they could do this:

enum DisableableUart {
    Disabled(UARTE0, uarte::Pins),
    Enabled(Uarte<UARTE0, ..>),
}

IMO this is much nicer than adding enable/disable.

• Makes it very clear to the user it's their responsibility to check whether it's enabled/disabled on use.
• Makes it impossible to call eg read() if uart is disabled.
• Adds no extra runtime complexity to the HAL for users that don't need it.

@Dirbaio: Could you please expand on this some more? I find it intriguing, but I have tried to implement it and could not get it to work.

[pietgeursen]

Disabling peripherals is something I don't expect too many people to do (only if you care about power usage)

There's another use-case that I just hit:

The SPIM instances share the same address space with instances of SPIS, SPI, TWIM, TWIS, and TWI. You need to make sure that conflicting instances are disabled before using Spim. See product specification, section 15.2.

At the moment I'm stuck because I can't disable the SPI0 so that I can use the TWI0.

[hannes-hochreiner]

The SPIM instances share the same address space with instances of SPIS, SPI, TWIM, TWIS, and TWI. You need to make sure that conflicting instances are disabled before using Spim. See product specification, section 15.2.

At the moment I'm stuck because I can't disable the SPI0 so that I can use the TWI0.

I have not considered this use case until now. Thank you for bringing it to my attention.

I have done some experiments in the last few weeks with typestates and async functions to get a better understanding of the problems. My current thinking is that for peripherals like SPI and TWI there is no reason to enable them in the constructor. They can be enabled when the communication is started and disabled right afterwards. In blocking functions, this would give the same behavior we see now, but with the additional benefit that less power is used. @pietgeursen, I would think that that would also cover your use case.

[taruti]

Fwiw needed a way to enable/disable saadc. No strong opinions on the API.