Add an unsafe fn SCB::steal

[jonas-schievink]

This allows safely creating an SCB instance from nothing, which is required when combining rtfm 0.4.x with APIs that make use of SCB/&mut SCB (rtfm retains ownership of the SCB).

Before, it was already possible to unsafely access the SCB register block via SCB::ptr(). However, it was impossible to obtain an SCB instance itself.

[rust-highfive]

r? @adamgreig

(rust_highfive has picked a reviewer for you, use r? to override)

[adamgreig]

I think this is a good idea and we should consider doing it for the other peripherals, but the unsafety comment should probably be more explicit than currently. Maybe say that users must synchronise SCB access with all other SCB instances to prevent race conditions, what do you think?

[jonas-schievink]

@adamgreig Yeah that's a good point. I've updated the docs.

[m-ou-se]

Peripherals::steal().SCB already works. Is there an advantage to a separate function that does this?

[jonas-schievink]

@m-ou-se Oh, you're right. I thought that still had the debug_assert! in it, but it was removed.

I guess having a separate function is still a little bit more convenient, but I can definitely see not providing more unsafe methods than necessary.

[m-ou-se]

It still has the CORE_PERIPHERALS = true; part though, so using it would make Peripherals::take() return None.

Maybe only take() should set that boolean, not steal(). (Might even save a byte of memory in case the user only uses steal() and not take().)

[jonas-schievink]

Yeah, I noticed that too. Fortunately most uses will probably not mind that too much since they'll only call steal after take, but in case LLVM isn't able to optimize the boolean away (when using only steal it is only ever written to), saving the byte would be nice.

[jonas-schievink]

Maybe only take() should set that boolean, not steal(). (Might even save a byte of memory in case the user only uses steal() and not take().)

One issue with this is that RTFM relies on this behavior for soundness. If this was changed, RTFM would call steal behind the scenes, but calling take() in the user code would then also return the peripherals instead of None, so you could acquire 2 instances of everything in safe code.

[m-ou-se]

One issue with this is that RTFM relies on this behavior for soundness. If this was changed, RTFM would call steal behind the scenes, but calling take() in the user code would then also return the peripherals instead of None, so you could acquire 2 instances of everything in safe code.

That would mean that RTFM's soundness is based on undocumented behaviour. The documentation of cortex-m's take and steal functions don't specify that steal would cause take to return None. (Or did I miss some documentation somewhere?)

Should this be the actual (documented) behaviour of steal()? Or should RTFM be updated call take() to make sure later calls to take() will return None?

[jonas-schievink]

Yeah you're right. I think it makes sense to change the documentation for steal(), since it allows writing this RTFM-like abstraction soundly without having to pay for the extra check take() is doing.

I could also see adding another function that will just set the flag and not do anything else, and then making steal() not set the flag implicitly anymore. Not sure what's best, or what the use cases for the additional method are. I guess it might be useful to be able to call steal() without affecting any other code, and it would also behave more similar to SCB::steal() I added here.

[m-ou-se]

adding another function that will just set the flag and not do anything else

That function already exists: That's just take() (just ignore its return value). It's a bit less efficient than it could be for that case though, because it temporarily disables interrupts. Maybe that can be avoided with a slightly different implementation.

[m-ou-se]

Maybe it'd even make sense to have a Cargo feature that RTFM can enable on cortex-m that would disable/remove the take function. When using RTFM, take is always going to return None anyway, so any usage is probably a mistake.

[jonas-schievink]

Maybe it'd even make sense to have a Cargo feature that RTFM can enable on cortex-m that would disable/remove the take function. When using RTFM, take is always going to return None anyway, so any usage is probably a mistake.

Interesting idea! I think this would be a pretty big change, so it might warrant some more discussion in an RFC issue.

[therealprof]

Blech, adding random features is a huge stumbling block for newcomers. We want to have less of those rather than more.

[m-ou-se]

A function that effectively always returns None also sounds like a stumbling block. Newcomers wouldn't have to touch the feature. It should be disabled by default, and using RTFM will enable it. It could even replace take() by a function with a #[depracated] marker or something that warns the user: "You're using a framework that handles the core peripherals, so take() will always return None"

[therealprof]

@m-ou-se I doubt it that a random user would stumble across a very special and specific function and be shied away by slightly unexpected semantics while on the other hand a feature gate always affects all users and thus provides a much larger problem surface.

Documenting it seems like a good idea though.

[hannobraun]

while on the other hand a feature gate always affects all users and thus provides a much larger problem surface

How does an on-by-default feature gate affect users negatively?

Edit: The suggestion here is to add a feature that removes the method, so in this case it would be an off-by-default feature gate. I still wonder what's so bad about feature gates though, so my question stands :-)

[therealprof]

@hannobraun On-by-default is even worse because every crate will actively have to opt out.

But even for regular features, the additive (global) nature of feature gates is a common source of troubles if some dependency crates decides to enable them (for whatever reason) and they really should be disabled in the application.

[jonas-schievink]

The feature would be opt-in, and only frameworks like RTFM that manage peripherals on their own would enable it, so there shouldn't be problems where the feature gets accidentally enabled in some application. (ie. it wouldn't have the same problems as a default-enabled std feature does)

It could even replace take() by a function with a #[depracated] marker or something that warns the user: "You're using a framework that handles the core peripherals, so take() will always return None"

This actually sounds pretty neat. However then we'd "pay" with adding a Cargo feature, only to show a warning to the user, not sure if this is a worthwhile tradeoff. Are there any other crates that do something like this? (I realize that we already do way more exotic stuff than this in eg. cortex-m-rt, but still)

[hannobraun]

@therealprof

But even for regular features, the additive (global) nature of feature gates is a common source of troubles if some dependency crates decides to enable them (for whatever reason) and they really should be disabled in the application.

Ah, okay. I figured you'd be talking about something more exotic. I probably misread your original comment (you said "always affects all users", which I understood to mean "always affects all users negatively", which I don't think is what you meant).

Thanks for explaining!

[jonas-schievink]

Closing this for now. It only provides a tiny utility function, and the future design and naming of take/steal-style APIs is not settled yet.