Don't expose registers as global static variables
Opened this issue · 16 comments
In the current state, registers are globals so it's easy to perform unsynchronized accesses to these registers from the main thread and from an interrupt.
Here's one solution:
// src/lib.rs
#[no_mangle]
pub fn start() -> ! {
extern {
fn main(peripherals: Peripherals) -> !;
}
let peripherals = unsafe {
Peripherals { gpioa: &mut GPIOA, gpiob: &mut GPIOB, .. }
};
main(peripherals)
}// src/bin/foo.rs
#[no_mangle]
pub fn main(p: Peripherals) -> ! {
do_something(p.gpioa, p.tim7);
loop {}
}Unresolved questions:
- How can we make sure the library user uses the right signature for the
mainfunction? - With this it's unclear how to let interrupts access the registers.
Here are three options.
First, we can expose register operation by global functions wrappers. In principle, calling these functions arbitrary is not safe, so we should mark these functions unsafe to transfer safety responsibility to user. Apparently, this is not expected.
Second, we can expose functions wrappers in trait impl block. But here comes another problem, we must take a &mut reference before we call them. And registers are still in global, this not work.
Third, we wrap all the unsafe code for a specific peripheral into a module with more safety check at runtime, like what RefCell do.
So, I have three alternatives in mind to expose registers in interrupt context:
- Add a unsafe function that returns an alias to the register. Basically:
// src/lib.rs
/// Returns an alias to the TIM7 register block
/// - Unsafety: It's up to the caller to ensure that the TIM7 register block is not being used in
/// other execution context.
pub unsafe alias_tim7<'a>() -> &'a mut gpio::Block {
&mut TIM7
}// src/bin/foo.rs
#[no_mangle]
pub fn main(p: AllPeripherals) -> ! {
// ..
}
// Interrupt handler
#[no_mangle]
pub fn tim7() {
// User must assert "This specific register is not aliased in other execution context"
unsafe {
// Clear interrupt flag
tim7().sr = 0;
}
// ..
}Downsides:
- Nothing stops you from aliasing memory.
unsafeis just a deterrent.
- Expose all registers behind a
RwLock
// src/lib.rs
static SAFE_GPIOA: RwLock<gpio::Block> = RwLock::new(GPIOA);// reset handler
#[no_mangle]
pub fn start() {
let gpioa = SAFE_GPIOA.write().unwrap();
// ..
}
// Interrupt handler
#[no_mangle]
pub fn tim7() {
let tim7 = SAFE_TIM7.read().unwrap();
}Downsides:
- runtime cost
unwrap- Not fine grained enough. Locks on whole register blocks. Would be better to be able to lock on a register level.
- It's unfortunate that one must acquire locks during the "setup phase" where interrupts are known to be disabled.
- Macro magic to statically "assign" registers to the
mainfunction and to each interrupt. (Something along the lines of Zinc's "platformtree")
// src/bin/foo.rs
// This macro asserts that the same register doesn't appear in two (or more) execution contexts
assign! {
// Syntax: EXECUTION_CONTEXT: REGISTER, OR_BLOCK, ..
main: GPIOA, UART,
tim7: TIM7->SR
}
fn setup(p: AllPeripherals) {
// All the registers are available here
init(p.GPIOA);
more_init(p.UART);
}
fn main(p: MainPeripherals) -> ! {
// Only GPIOA and UART are available here
let gpioa = p.GPIOA;
// ...
)
fn tim7(p: Tim7Peripherals) {
// Only TIM7->SR is available here
// Clear interrupt flag
p.tim7_sr = 0;
}
// The `assign!` macro auto generates all this code
struct AllPeripherals { ... }
struct MainPeripherals { ... }
struct Tim7Peripherals { ... }
// Reset handler
#[no_mangle]
pub fn start() -> ! {
unsafe {
setup(AllPeripherals { gpioa: &mut GPIOA, ... });
main(MainPeripherals { gpioa: &mut GPIOA, uart1: &mut UART1 });
}
}
// Interrupt handler
#[no_mangle]
pub fn __tim7() {
unsafe {
tim7(Tim7Peripherals { tim7_sr: &mut TIM7.SR })
}
}Downsides:
- MAGIC! No idea whether this can be implemented or not. Or how to implement it.
@goandylok
My alternative (1) is like your point (1), and my alternative (3) is like your point (3). I think that your point (2) is about adding unsafe methods to e.g. gpio::Block to allow modification of it via a &- reference?
Yes, that is what I mean.
And your alternative (3) is possible. To do that, we must go through ::syntax module and make new wheels of parser, seeking register aliasing in abstract syntax tree. It's very difficult to do right, because we are changing the language rules, just like we are making a new part of rustc. More over, nightly ::syntax are always changing, making zinc's author tired.
At some point I was tired of tracking rust-nightly every so often. There was a rewrite of ioreg concept that didn't use compiler macros and it's actually functional but never got merged because I still can't manage the time properly (I'm not actively working on zinc for like half a year now).
In my opinion, programming on bare metal requires highest speed, which acquires complete control on hardware. So the programmer should be more ‘cleaver’ and more '
knowledgeable' than the time they are writing Javascript.
If there are no way to perform compile-time check, I prefer the option (1)
We need not make whole program into unsafe.
Only unsafe when we fetch a register is also working.
pub unsafe tim7<'a>() -> &'a mut gpio::Block {
&mut TIM7
}let tim7 = unsafe { tim7() }
tim7.sr = 0;I have two little questions.
- Are we making a book to guide people building up the whole kingdom or to make and document a library?
If we use magical macro, there are no way to explain bit by bit. - Is it necessary to prevent user from using same register in
main()andinterrupt_handler()? As we may useBITBANDorSETTER
And your alternative (3) is possible. To do that, we must go through ::syntax module
Right, I definitely don't want to write a syntax extension. If it can't be done with macro_rules then I'm not going to implement it.
In my opinion, programming on bare metal requires highest speed, which acquires complete control on hardware.
I tend to agree. I personally like to program at the register level because I can clearly see what's going on and have maximum control (and the programs I write will usually run on a single device), but we can't deny that framework like Arduino are nice to use because of the high level device-agnostic APIs they provide even if they are not high performance or ultra flexible. I think it would be a good exercise for the book to implement high level device-agnostic APIs like those.
If there are no way to perform compile-time check, I prefer the option (1)
Yeah, I prefer option (1) too because it's simple and the unsafe blocks will highlight the areas of code that need to be audited.
Are we making a book to guide people building up the whole kingdom or to make and document a library?
I want to guide the reader through the process of building a "framework" from scratch as a way to explain the concepts related to microcontrollers: peripherals, interrupts, event-driven programming, data races, critical sections, schedulers, etc.
If we use magical macro, there are no way to explain bit by bit.
Right, I want to keep the framework we are going to build relatively simple. One can do crazy stuff with the type system and syntax extensions to make programs bullet-proof, but I don't want to go there. Perhaps I'll mention in the book about those possibilities but I won't be implementing them.
Is it necessary to prevent user from using same register in main() and interrupt_handler()? As we may use BITBAND or SETTER
BITBAND doesn't protect you from race conditions, you still need to synchronize operations if you are modifying the same register from main and the interrupt handler. Example below:
// NOTE omitting `unsafe` for brevity
fn main() {
// ..
let some_reg = TIM7.some_reg;
// <- tim7 interrupt kicks here!
// Toggle second bit
let modified_reg = some_reg ^ 0b10;
// Modification of the first bit never ocurred?! :scream_cat:
TIM7.some_reg = modified_reg;
// ..
}
fn tim7() {
// Toggle first bit
TIM7.bb().some_reg.first_bit ^= 1;
// ..
}This race condition can be avoided if you use a critical section to order the operations:
// NOTE omitting `unsafe` for brevity
fn main() {
// ..
// <- tim7 could occur here or before
// Critical section: disable interrupts, execute this closure and then re-enable the interrupts
nointerrupt(|| {
// tim7 can't occur in this block
let some_reg = TIM7.some_reg;
// Toggle second bit
let modified_reg = some_reg ^ 0b10;
TIM7.some_reg = modified_reg;
});
// <- tim7 could occur here or after
// ..
}
fn tim7() {
// Toggle first bit
TIM7.bb().some_reg.first_bit ^= 1;
// ..
}The scenario where you won't get a race condition is where both main and the interrupt handler access the same register via the bitband region. This is why the cu crate offers a bitband view of a register as a static global and that view can safely be modified from any context because modifications are done via methods that take &- references; on the other hand, plain registers are static mut globals and operations on them are unsafe -- they must synchronized somehow.
I'm not quite sure what SETTER is?
BITBAND doesn't protect you from race conditions, you still need to synchronize operations if you are modifying the same register from main and the interrupt handler.
What you mentioned are different from my understanding of BITBAND. As far as I know, Cortex-M3 cores expand some special register, especially GPIOs, into 32 times larger memory address.
For example, if I wanna to set high PA8, I could do it in normal way.
fn main() {
// ..
// directly access IO register
let reg = PortA.reg;
// <- interrupt may occurs here, so bad
let modified_reg = reg | 1<<8;
TIM7.reg = modified_reg;
// ..
}In this case, race conditions may happen, because we need to do three operations in assembly (read,bit-or,write), which made it not atomic.
Yet, BITBAND operation is atomic.
fn main() {
// ..
// access IO through BITBAND register
let bitband = PortA.Pin(8).BITBAND;
// <- whenever interrupt occur, we can't be in race condition, Horray!!
bitband = 1;
// ..
}Cortex M3 mapped each bit of PortA into 16-byte-long space(BITBAND). In these space, only the first bit of the bytes make sense. So. I'm not quite understand what you means that we must synchronize them.
I'm not quite sure what SETTER is?
It's another atomic register provided by STM32 chip.
The register is named BRR(write only) and BSRR(write only), and the normal-way register is called ODR(read/write)
BSRR is used to set a or some bits 1
GPIOB->BSRR = 0x80 // Set PB7 high, regardless whatever other bits is
GPIOB->BSRR = 0b110 // Set PB1 and PB2 high, regardless whatever other bits isBRR is used to set 0
GPIOC->BRR = 0x80 // Set PC7 low, regardless whatever other bits is
GPIOC->BRR = 0b110 // Set PC1 and PC2 low, regardless whatever other bits isWhat's more, using BSRR can be tricky.
When we wanna set PC0 low and PC1 high
GPIOC->BSRR = 0b10
GPIOC->BRR = 0b1The operation is separated and not synchronized(but it is not serious on safety)
Because high 16 bits of BSRR can also set 0, we can do it like this
GPIOC->BSRR = 0x10002 // 0b000000000000001_0000000000000010
BRR-High↑ BSRR-Low↑As high-BSRR behaves just like BRR, I never uses BRR.
I prefer BSRR rather than BITBAND, because I need to work out the BITBAND address for a bit by a complex formula.
AliasAddr=0x22000000+((A-0x20000000)*8+n)*4=0x22000000+(A-0x20000000)*32+n*4
But, I think we should use it, not the BSRR. As BITBAND is common.
For others case, we can stop interrupt more elegantly.
pub struct NoInterrupts {
#[allow(dead_code)]
contents: ()
}
impl NoInterrupts {
/// Start a new critical section
pub fn new() -> NoInterrupts {
unsafe {
disable_irqs();
}
NoInterrupts { contents: () }
}
}
impl Drop for NoInterrupts {
fn drop(&mut self) {
unsafe {
enable_irqs();
}
}
}fn main() {
// ..
// usually, codes are wrraped in some blocks, which we can reuse
match x {
TASK_1 => {
NoInterrupt::new();
// interrupt stopped in this block
let some_reg = TIM7.some_reg;
// Toggle second bit
let modified_reg = some_reg ^ 0b10;
TIM7.some_reg = modified_reg;
}
}
// <- tim7 could occur here or after
// ..
}
fn tim7() {
// Toggle first bit
TIM7.bb().some_reg.first_bit ^= 1;
// ..
}However, stopping interruption every time we touch register is not a good solution. Although this could work perfectly in a blinking demo, it could not lead to a robust productive project.
What you mentioned are different from my understanding of BITBAND.
Oh no, I think we are on the same page about BITBAND. Yes, they are atomic bit level operations.
If all the context of executions (main, interrupt handlers) use BITBAND operations you are safe from race conditions. But, if all least one of them (e.g. main) does the "normal' read-modify-write operation, you are exposed to race conditions -- that's the scenario I was describing in my previous comment (perhaps my example was not clear?). I guess the point was that BITBAND is a necessary condition to achieve data race freedom but its not sufficient on its own, you still need to think about read-modify-write operations (on the same register) from other contexts of execution.
I should also note that exclusively using BITBAND operations can quickly increase the program size because each operation can only modify one bit so you may end up needing a bunch of them. BITBAND can also be slower (require more instructions) than read-modify-write if you need to modify several bits of a register.
The register is named BRR(write only) and BSRR(write only)
I see what you are referring to now. Those registers only work with GPIO pins though; they are not a general mechanism like BITBAND.
For others case, we can stop interrupt more elegantly.
This struct NoInterrupt is the same as the nointerrupt(/* closure */) function from my previous comment. NoInterrupt uses a implicit drop guard, whereas nointerrupt explicitly delimits the scope of the critical section.
And actually your critical section example is wrong because you dropped the NoInterrupt guard too early:
{
NoInterrupt::new(); // WRONG! Guard dropped here (too early)
// These statements are unprotected :scream_cat:
let some_reg = TIM7.some_reg;
let modified_reg = some_reg ^ 0b10;
TIM7.some_reg = modified_reg
}Should have been:
{
let guard = NoInterrupt::new();
let some_reg = TIM7.some_reg;
let modified_reg = some_reg ^ 0b10;
TIM7.some_reg = modified_reg
} // OK! `guard` dropped hereThis is another reason why I prefer the explicit closure scope!
However, stopping interruption every time we touch register is not a good solution.
Agree. Critical sections should be used sparingly -- only when they are necessary.
I think the best way his to go step by step, level of abstraction by level of abstraction. Starting with raw unsafe access to registers (with struct generated from svd file, I plan to do that during next week) and when we have more experiences/ideas, provide better safer abstraction.
Concerning BITBAND, it's not available on all cortex-m microcontroller, especially the M7. So, I think that relying on it is not the best idea.
I think the best way his to go step by step, level of abstraction by level of abstraction.
Yes, this incremental approach is a core foundation of the Copper book.
with struct generated from svd file, I plan to do that during next week
Nice! Let us know how it goes.
I considered exploring this idea before but was turned off by the quality of st's SVD files (they are not as great as this SVD File Example, for instance none of the ones I've seen use the enumeratedValue tag). At that time I wanted to create statically (no assert!s) unbreakable register structs but that wasn't possible with st's SVD files. In retrospective, the result turned out to be annoying to use (e.g. Pin::_9 instead of just 9). Nowadays, I'm OK with liberally using assert!s as they usually get optimized away when compiling with LTO.
Concerning BITBAND, it's not available on all cortex-m microcontroller, especially the M7. So, I think that relying on it is not the best idea.
Good point. Noted.
Yap, I agree we should build up abstractions bit by bit. However, the bottom unsafe layer holds full responsibility on safety, which means the second and third layer need not care about it. So, when we started to write unsafe code, we must consider every possible situations. I think the race condition problem can not be postponed.
@dzamloI Generating structs from svd sounds great! Good luck. I had an idea with macro_rules on it, and I am going to try it this week.
---- Update ---------------------------------------------------
@japaric You are right. I eat my word. The quality of ST's svd file surprised me.
RustyGecko provided a solution.
pub struct GpioPin {
pub port: gpio::Port,
pub pin: u8,
}
impl GpioPin {
pub fn new(port: gpio::Port, pin: u8) -> GpioPin {
GpioPin {
port: port,
pin: pin,
}
}
}
pub trait Button {
fn init(&self);
fn on_click(&self, func: gpioint::IrqCallback);
}
pub trait Led {
fn init(&self);
fn on(&self);
fn off(&self);
fn toggle(&self);
}
impl Button for GpioPin {
fn init(&self) {
cmu::clock_enable(cmu::Clock::GPIO, true);
gpio::pin_mode_set(self.port, self.pin as u32, gpio::Mode::Input, 0);
gpio::int_config(self.port, self.pin as u32, false, true, true);
gpioint::init();
}
fn on_click(&self, func: gpioint::IrqCallback) {
gpioint::register(self.pin, func)
}
}
impl Led for GpioPin {
fn init(&self) {
cmu::clock_enable(cmu::Clock::GPIO, true);
gpio::pin_mode_set(self.port, self.pin as u32, gpio::Mode::PushPull, 0);
}
fn on(&self) {
gpio::pin_out_set(self.port, self.pin as u32);
}
fn off(&self) {
gpio::pin_out_clear(self.port, self.pin as u32);
}
fn toggle(&self) {
gpio::pin_out_toggle(self.port, self.pin as u32);
}
}use emlib::gpio::Port;
use modules::{GpioPin, Button, Led};
const PB0: &'static Button = &GpioPin { port: Port::B, pin: 9 };
const PB1: &'static Button = &GpioPin { port: Port::B, pin: 10 };
const LED0: &'static Led = &GpioPin { port: Port::E, pin: 2 };
const LED1: &'static Led = &GpioPin { port: Port::E, pin: 3 };
fn blink_led(pin: u8) {
match pin {
9 => LED0.toggle(),
10 => LED1.toggle(),
_ => ()
}
}
#[no_mangle]
pub extern fn main() {
PB0.init();
PB1.init();
LED0.init();
LED1.init();
PB0.on_click(blink_led);
PB1.on_click(blink_led);
loop {}
}It hide different logic behind static distribution. And more important, there is no unsafe, and no read-modify-write. We can ensure all operation on register is in a single file (they use C library here).
RustyGecko - modules
I have another idea. We could implement rust concurrency system for mcu.
For most time, I send and recieve data to buffer in interruption handler and do business in main, what exactly channel and sync suit for. To do that we can spawn() an interruption, move register or a module shown above into it and send data via channel.
@goandylok That's a nice looking high level API!
I have to admit that making the peripherals globally available to main and all the interrupt handlers makes me uneasy. Probably RustyGecko's runtime makes it safe through some implicit synchronization mechanism but I'd have to check their code.
I have another idea. We could implement rust concurrency system for mcu.
I'd like to hear more details but that belong to another thread so I'm going to open another issue to discuss that. See #35
I am not saying this pattern could make everything safe. We must make sure by ourselves, digging into its library,
Nice! Let us know how it goes.
The current result of my experimentation with SVD is at https://github.com/dzamlo/svd.