"Safe" pin manipulation not actually safe

[robomancer-or]

So this might be me setting something up wrong, but if so I really don't see what it is... here's how to recreate the bug:

I'm testing this on an STM32F407 with LEDs on the obvious pins

`fn main() {
let p = stm32f40x::Peripherals::take().unwrap();
p.RCC.ahb1enr.write(|w| w.gpioeen().enabled());
unsafe {
p.GPIOE.moder.modify(|r, w| w.bits(r.bits() | 0x0550_0000));
p.GPIOE.otyper.modify(|r, w| w.bits(r.bits() & 0b11_0000_1111_1111_11));
}
p.GPIOE.odr.write(|w| w.odr10().set_bit());
p.GPIOE.odr.write(|w| w.odr11().set_bit());
p.GPIOE.odr.write(|w| w.odr12().set_bit());
p.GPIOE.odr.write(|w| w.odr13().set_bit());

loop {
asm::nop();
}
}
`

Expected behavior: all 4 LEDs off (they're low side switched).

Observed behavior: only the LED connected to GPIOE_13 is off. You can modulate which LED is off by commenting out odrX set_bit calls that come after the target LED in the code.

My guess: the compiler is dicking around because some "don't mess with this" flag is missing

[japaric]

p.GPIOE.odr.write(|w| w.odr10().set_bit());

write is a single store operation; not a load-modify-store operation so the old value is being discarded.

Same thing with these other 3 calls:

p.GPIOE.odr.write(|w| w.odr11().set_bit());
p.GPIOE.odr.write(|w| w.odr12().set_bit());
p.GPIOE.odr.write(|w| w.odr13().set_bit());

So the final value of ODR will be the last write operation: ODR = 1 << 13.

I think you meant to use modify instead of write.

[robomancer-or]

Ok, I agree that that would get me the expected results, but it still seems wrong that calling a safe method called "set_bit" would change any other memory.

[kunerd]

@robomancer-or: I stumbled across the same at first, but it's all documented in the svd2rust docs:

The write method takes a closure with signature (&mut W) -> &mut W. If the "identity closure", |w| w, is passed then the write method will set the CR2 register to its reset value. Otherwise, the closure specifies how the reset value will be modified before it's written to CR2.

It doesn't really change any other memory, because write is a method on the whole register itself and as stated in the docs, it starts by using the reset_value as a base, which can then be modified in the closure. I think this is basically needed for write-only registers, which does not allow to read their current value and modify it afterwards.

[robomancer-or]

Lol, it's documented, and therefore a feature and definitely not a bug!

I still challenge this design choice for several reasons, but I'm very new to the language, so if it seems like I'm missing some important idea about safety I almost certainly am (please let me know):

1. It seems wrong that calling a safe method called "set_bit" would change any other memory
2. It means that the lambda |HIDDEN| w.odr11().set_bit() does different things depending on what is behind HIDDEN, which subtracts code clarity, having an ignored input shouldn't change behavior.
   2.5. To say # 2 in a different way, it's odd that the same method on the same struct does different things based on hidden state. If my lambda were to pass w to some other function for a more complicated decision process I'd have to include a second parameter to track what w will actually do.
3. Though that behavior is documented in svd2rust, it doesn't seem to be documented in the crates people actually use. svd2rust is nowhere in my Cargo.lock file...
4. If write really means "a reset followed by a modify" it suggests that the api is not particularly orthogonal. You could easily argue that having write is nice a convenience, but it should be descriptively named (perhaps reset_with?) and there should be some other descriptively named convenience for people who just want to set a bit (which I would guess is the far more common use case).

[kunerd]

I would like if the discussion could stay a bit more objective, I never said that everything is fine and not a bug if it's documented somewhere. I just tried to describe you the functionality of write with the help of the documentation part I cited and my additional thoughts. That didn't pay off very well, sorry for that. I'm not a native English speaker, so I may have missed an important point.

I still challenge this design choice for several reasons

That's your good right, and better solutions (naming included) are always welcome.

It means that the lambda |HIDDEN| w.odr11().set_bit() does different things depending on what is behind HIDDEN, which subtracts code clarity, having an ignored input shouldn't change behavior.

That's not true at all, calling p.GPIOE.odr.write(|w| w.odr11().set_bit()); will always produce the same result. It will always set odr11 on GPIOE.odr.

You could easily argue that having write is nice a convenience, but it should be descriptively named (perhaps reset_with?)

That's discussable and as I said above, improvements are always welcome.

and there should be some other descriptively named convenience for people who just want to set a bit

There is, it's called modify and does exactly what it is named after: it modifies a register.

Sorry if I can't help you further. Maybe you can explain what you understand under safety and how it could be applied in your example.

[robomancer-or]

Oh, I'm sorry, I was trying to be playful. There's a somewhat widely known saying (at least in the US) that if it's documented it's a feature rather than a bug. I didn't mean anything by it.

What I was saying is that HIDDEN could have contained r, w (or even _, w) instead of just w. I'm highlighting that the meaning of a bit of code changes significantly based on factors elsewhere in the code. Item 2.5 says the same thing differently.

My understanding of safety is that it entails different parts of the code being unable to step on each others feet (one example of which being mutating state that somewhere else thinks it owns). If GPIOA 1-5 are being used for the Ethernet MAC, and I want to add support for USB OTG on GPIOA 9-12, the two drivers shouldn't be able to clobber one another. In the present state of things this can't happen only because we're operating at the register level, but do we really want a situation where using Ethernet MAC on 5 of the pins means I can't use the other 11 pins for anything?

[Emilgardis]

In the present state of things this can't happen only because we're operating at the register level, but do we really want a situation where using Ethernet MAC on 5 of the pins means I can't use the other 11 pins for anything?

It is possible (but not very easy with modify/write). However you can solve it easily, but just like with wrapping C in rust you will have to make a higher-level abstraction, see for example https://github.com/japaric/stm32f103xx-hal/blob/master/src/gpio.rs#L78. This may be hard to parse but it enables zen-main165 which then leads to zen-main274 and finally passing it off (safely) to zen-main297.
This means that we can separately process the first and second motor without interfering with any other bits on GPIOB.

[kunerd]

What I was saying is that HIDDEN could have contained r, w (or even _, w) instead of just w. I'm highlighting that the meaning of a bit of code changes significantly based on factors elsewhere in the code. Item 2.5 says the same thing differently.

If the signature of the closure changes, your code wouldn't compile anymore, or I don't get your point here.

My understanding of safety is that it entails different parts of the code being unable to step on each others feet (one example of which being mutating state that somewhere else thinks it owns). If GPIOA 1-5 are being used for the Ethernet MAC, and I want to add support for USB OTG on GPIOA 9-12, the two drivers shouldn't be able to clobber one another. In the present state of things this can't happen only because we're operating at the register level, but do we really want a situation where using Ethernet MAC on 5 of the pins means I can't use the other 11 pins for anything?

This is a different story and it was discussed in other places, too. I think you are expecting too much from the automatically generated code here, it's only the first (low-level) hardware abstraction layer and it provides some nice features that you won't get (easily) with C, like implicit validity checking of register values. All other safety features can only be achieved in higher level-abstraction, like @Emilgardis suggested above.

[robomancer-or]

Yeah, it definitely wouldn't compile, but when I'm reading code I'm not JIT compiling it in my head, I glance at a thing and expect its meaning to be evident. In this case the meaning isn't evident because it depends on factors elsewhere in the code (worse, due to the naming it seems evident what the code does, so you have to learn what not to do the hard way). I'm talking about clarity and doing what the consumer expects, not about theoretical correctness.

it provides some nice features [...] like implicit validity checking of register values

Doesn't that assume MCUs will only ever have reset values that are both valid, and any one register swap away from valid?

[kunerd]

Yeah, it definitely wouldn't compile, but when I'm reading code I'm not JIT compiling it in my head, I glance at a thing and expect its meaning to be evident. In this case the meaning isn't evident because it depends on factors elsewhere in the code (worse, due to the naming it seems evident what the code does, so you have to learn what not to do the hard way). I'm talking about clarity and doing what the consumer expects, not about theoretical correctness.

Following this argumentation, every closure or callback driven interface ẁill be unsafe and/or not intuitively usable. From my point of view, you need (at least) to know the specification of an interface, to use it. But this isn't the right place to discuss this further, so if you want you can ping me on the usual channels on IRC (#rust, or better #rust-embedded).

By the way, this issue might be of interest for you.

[japaric]

1. It seems wrong that calling a safe method called "set_bit" would change any other memory

Are you suggesting marking the method as unsafe? That would be plain wrong. Rust is very clear about its "safety" being memory safety (no pointer / iteration invalidation, no data races, etc.). Even though writing to BSRR modifies ODR (because that's how the hardware works) that's not memory unsafe: you can not grab the BSRR register, sent it to other thread, write to it there and cause a data race in the execution context that hold the ODR register because it's not possible to split GPIOx proxy struct into registers.

2.5. To say # 2 in a different way, it's odd that the same method on the same struct does different things based on hidden state.

If you are referring to w.odr11().set_bit() where w: &mut _ODRW. It always does the same thing: it sets the ODR11 bit of the ODR register value (not the register itself).

It's the closure that sets the context of how w will be used. if you have any suggestion to make the write method name more clear about what it does let me know.

3. Though that behavior is documented in svd2rust, it doesn't seem to be documented in the crates people actually use. svd2rust is nowhere in my Cargo.lock file...

The second line of the crate documentation of all device crates generated using svd2rust have a link to svd2rust register-level API documentation.

4. If write really means "a reset followed by a modify"

But it doesn't; it literally means write this w value into the register -- that's a single store operation. reset and modify would be 3 operations: (1) write the reset value to the register, (2) read the register, modify that value and (3) write the modified value into the register.

[robomancer-or]

I'm not suggesting anything beyond what I said; it seems wrong that a function called set_bit would change any other part of memory (unless that bit has a functional purpose, obviously). I understand (now) that it's really write that is changing the other memory. It's a statement about aesthetics/learnability, not technical correctness. You're right that it's not a safety concern. ... And you're also right to call me out for not having a better suggestion... I'll keep this in mind and let you know if I think of anything.

[bergus]

I do like the reset_with suggestion! Another approach that could help would be using

p.GPIOE.odr.write(|reset| reset.odr10().set_bit());
p.GPIOE.odr.write(|reset| reset.odr11().set_bit());
p.GPIOE.odr.write(|reset| reset.odr12().set_bit());
p.GPIOE.odr.write(|reset| reset.odr13().set_bit());

(or something similar that shows that the closure argument is the reset value) in all places, especially the examples in the docs.