Remove debug assert from Peripherals::steal

[thenewwazoo]

Very simple PR! :)

This permits actually-unsafe use of the steal method, and avoids divergent behavior between debug and release modes.

[jamesmunns]

So, not the final word, but my $0.02,

This could maaaaaaybe be considered a breaking change, if someone was relying on this assert to trigger in debug mode (it's a long shot, I know).

I would say that either of the following would be a no-brainer approve from me:

• Add a #[cfg(not(feature = "unchecked-steal"))] block around this assert, so users can opt-out of the assert.
  • Downside: svd2rust doesn't currently generate Cargo.toml, so the user would have to define the feature themselves. Not a great UX
• Add a new method, something like unsafe fn steal_unchecked(), which doesn't have the assert.
  • That being said, "steal" is basically supposed to be take_unchecked, so this seems excessive.

I don't have strong feelings either way, but I would appreciate more feedback from anyone on @rust-embedded/tools

[therealprof]

@jamesmunns Back when @japaric introduced the debug_assert! I was arguing against its divergent behaviour in debug vs. release but people seemed to be in favour of doing it that way. I don't think circumstances have changed since then so it would be great to hear from the previous proponents whether their opinion has changed.

I still see this as an ergonomics only issue since it is possible to access the peripherals anyway and a crippled steal is only making that mildly more complicated...

[jamesmunns]

@therealprof there are some things you cannot do, I actually ran in to it recently. For example, you can use SCB::ptr(), however that derefs to a RegisterBlock, not an SCB. There is no public method to create an SCB.

This means you cannot easily call a member function of SCB, like disable_icache(). See here: https://github.com/rust-embedded/cortex-m/blob/e3b7357002f0fa739253c78623f825e0e8a7f1ab/src/peripheral/scb.rs#L330

I will admit I have done this before, and it seems to work, but seems incredibly unsound:

// AJM - unsure if this is sound
let cpuid = &mut *(CPUID::ptr() as *mut CPUID);
let scb = &mut *(SCB::ptr() as *mut SCB);
// Disable icache
scb.disable_icache();

// Disable dcache
scb.disable_dcache(cpuid);

[jamesmunns]

For context, this was in a #[pre_init] function, which is why I couldn't take the peripherals here.

Though, it might be nice to be able to "give back" the peripherals to allow them to be taken later, but that really would only help in the case of pre-init, not in an interrupt context or anything, so probably not really worth it.

[thenewwazoo]

One of the most useful aspects of steal is that it gives you an owned copy which, yes, while horribly unsafe is useful from time to time. Casting pointers (clever though that is!) doesn't accomplish the same thing, unfortunately. Aside from patching crates, I still haven't found a workaround, and I'd love to ship some code. :)

[thenewwazoo]

I've pushed and published a manual update to the crates relevant to me, but only because I was able to take ownership of them. So while I've got a workaround, it's not ideal. :) I'd love to get this merged in so I can de-monkey-ify things.

[DoumanAsh]

I think, considering unsafe trait of this function, debug assertion is actually wrong here as it actually makes this function no longer unsafe.

I'd argue that removing this assert makes sense and it should be up to user to control the usage of Peripherals instances

[thenewwazoo]

@DoumanAsh I'm not sure I understand what you're saying in your second paragraph. Are you suggesting to leave the debug_assert, but remove the unsafe? My goal is to be able to control owned Peripherals instances, even in cases where I've already taken them, which is not currently possible.

[DoumanAsh]

@thenewwazoo No, what I'm saying is that any sort of assertion to prevent calling this method twice, makes it safe.
Instead the assertion should be removed and usage of such unsafe method should be up to user's choice

UPD: Actually noticed I made mistake in my original comment 😄

[starblue]

I would go further and in addition to removing the dbg_assert!() do the following:

• Move setting the flag into take(): without this the pre-init use mentioned by @jamesmunns would still prevent take() in the main initialization.

• Rename the flag to PERIPHERALS_TAKEN to make the meaning clearer.

[therealprof]

To move things forward I'm going out on a limb here and will approve this on the basis that it never made sense to me and @DoumanAsh has quite a point in saying that this function is marked unsafe anyway so people using it will need to reason about the use of the "stolen" peripherals.

[therealprof]

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push