How can I properly modify a global variable from an interrupt service routine?

embedded
1

I am using the avr-hal crate to write firmware for an atmega328p. I'm trying to modify a register value from an interrupt service routine, but I'm having issues with borrowing when trying to do so that I'm not really sure how to properly solve. I've read that mutex's are probably a way to do this, and avr-hal does provide them with avr_device::interrupt::mutex, but I'm not exactly sure how to go about that, or if there's a better method. In general, I'm not familiar with how interrupts should be handled in rust. For example:

#![no_std]
#![no_main]
#![feature(abi_avr_interrupt)]

use atmega_hal as hal;
use panic_halt as _;

#[hal::entry]
fn main() -> {
    let peripherals = hal::Peripherals::take().unwrap();
    let pins = hal::pins!(peripherals)

    let mut test_pin = pins.pb0.into_output();
    let mut test_output = pins.pc3.into_pull_up_input();

    peripherals.EXINT.pcicr.modify(|_, w| w.pcie().bits(1 << 2));
    peripherals.EXINT.pcmsk1.modify(|_, w| w.pcint().bits(1 << 3));

    unsafe { avr_device::interrupt::enable(); }

    loop {
        test_pin.toggle();
    }
}

#[avr_device::interrupt(atmega328p)]
fn PCINT1() {
    // I want to use peripherals here to be able to modify registers. For example, something like
    // peripherals.TC1.ocr1a.write(|w| w.bits(1000))
}

I thought adding something in the global scope like

static GLOBAL_PERIPHERALS: avr_device::interrupt::Mutex<RefCell<Option<hal::Peripherals>>> = avr_device::interrupt::Mutex::new(RefCell::new(None));

And then modifying things in the critical sections with something like:

avr_device::interrupt::free(|cs| {
    let local_peripherals = GLOBAL_PERIPHERALS.borrow(cs).borrow()
});

but I'm really not sure how or where to properly do that. Or if there's other ways, or better ways to handle interrupts.

2

don't share all the resources, only put the specific pheripheral into global state, which should be initialized in main. for example, only share the TC1 resource for the timer interrupt:

static TIMER1: Mutex<RefCell<Option<pac::TC1>>> = Mutex::new(RefCell::new(None));

fn main() {
    let peripherals = Peripherals::take().unwrap();
    // initialize the timer in critical section, i.e. with irq disabled:
    interrupt::free(move |cs| {
        let tc1 = peripherals.TC1;
        // configure the timer
        todo!();
        // update the global variable
        TIMER1.replace(cs, Some(tc1));
    });
}

#[avr_device::interrupt(atmega328p)]
fn PCINT1() {
    // access the peripheral in critical section
    interrupt::free(|cs| {
        let register = TIMER1.borrow_ref_mut(cs).as_mut().expect("not initialized yet");
        register.ocr1a.write(|w| w.bits(1000));
    });
}

as for the memory and runtime overhead, this is not ideal, but it is the best you can do without unsafe.

the critical_section::Mutex wrapper has no runtime overhead, the overhead of RefCell is a borrow counter, which is implemented using isize, and Option<T> is essentially equivalent to a boolean flag when T is stateless.

in this particular example, the RefCell is unnecessary, but you cannot replace it with Cell, because the TC1 type is not Copy, despite being a stateless type. in order to eliminate the RefCell overhead, you must use unsafe, e.g. by using UnsafeCell directly.

since TC1 is stateless, Option<TC1> is just an boolean flag indicating whether the peripheral has been initialized or not. in theory, you can eliminate this too if you really really really want to, e.g. by replacing it with MaybeUninit<TC1>, but this is extremely unsafe, I wouldn't recommend it, even if you are an expert of unsafe rust.

side note:

it's not good practice to use the peripheral resource types from pac directly, better to use the abstractions from hal layer instead.

also, this kind of resource management is exactly what rtic is designed for. for example, if the timer is only accessed in the interrupt handler, you can declare it as a "local" resource of the interrupt handler task, which can be initialized in the init task and then passed the hardware task, all without any runtime overhead, without writing unsafe code yourself. see:

5 Likes
3

Do you mean TIMER1 instead of TC1?

How do I pass pins to the interrupt service routine? For example, what if I wanted to use test_pin in the ISR? Would it just be

static GLOBAL_PIN = Mutex<RefCell<Option<atmega_hal::Pins>>> = Mutex::new(RefCell::new(None))

with the same access in a critical section as before.

Hm, I think I would need to write it as, for example:

    avr_device::interrupt::free(|cs| {
        let local_peripherals = GLOBAL_PERIPHERALS.borrow(cs).borrow();
        local_peripherals
            .as_ref()
            .unwrap()
            .TC1
            .ocr1a
            .write(|w| w.bits(1000))
    });

How come you're borrowing as a mut? Does it need to be a mut to modify a register? Also how come you don't have unwrap()?

4

yes, it was a typo, edited.

each pin (more precisely, each pin and its configuration) is represented by a distinct type. for instance, Pin<Output, PB0> is a type different from Pin<Input<PullUp>, PB0>, different from Pin<Output, PB1> either. once you understand this concept, it's should be clear what type you should use for the shared states (global variables) between the main program and interrupt service routine.

the embedded rust ecosystem makes heavy use of rust's advanced type system, particularly, hardware resources are modeled as "linear" types (here I use the term "linearity" loosely to refer to the properties like singleton, move only, cannot be copied or cloned, conversions commonly named into_xxx() will "consume" value of the old type to produce the a value of the new type), and data race is eliminated either at compile time by the borrow checker, or at runtime using proper abstraction like the critical section token, and so on.

this approach may seem complicated to traditional embedded developers using C, but once you get familiar with it, it feels so natural, it changes how you think and even how you write C code.

the Pins struct represents all the hardware pins, test_pin is a single IO pin defined by your board, you can just share the specific pin.

each hal crate have their own design, I notice the atmega-hal crate uses a generic Pin type with an type level parameter for the mode and pin id. take the PB0 pin configured as Output as an example for your test_pin, you can do something like:

type TestPin<Mode> = Pin<Mode, PB0>;
type TestMode = Output;

static TEST_PIN: Mutex<RefCell<Option<TestPin<TestMode>>>> = Mutex::new(RefCell::new(None));

fn main() {
    let peripherals = hal::Peripherals::take().unwrap();
    let pins = hal::pins!(peripherals)

    let mut test_pin = pins.pb0.into_output();
    interrupt::free(move |cs| {
        TEST_PIN.replace(cs, Some(test_pin));
    });
}

you don't need to call borrow() twice, there are some special methods for the common case of Mutex<RefCell<T>>, you can call borrow_ref() or borrow_ref_mut() directly. see:

I didn't check the documentation, I just assumed exclusive access was required. if the API only needs a shared reference, you can use borrow_ref() instead. nevertheless, borrow_ref_mut() should also work in this example, because there's actually no concurrent/shared access.

3 Likes
5

avr-hal implements its own version of Mutex. It lacks some of std::sync::Mutex's methods.

6

well, that's interesting to know.

I thought it was a re-export of critical_section::Mutex, but it is actually using the old bare-metal crate. they work the same though, only differs very slightly in the API.

fun fact: bare-metal was planned to be deprecated in favor of critical-section. however, because it already reached 1.0, the planned version (maybe 1.1 or 2.0, I don't really know what was the plan) with deprecation annotations was not published to crates-io.

1 Like
7

Related: Better Mutex · Issue #52 · Rahix/avr-device · GitHub

8

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.