Unable to trigger either TIMER0_COMPA or TIMER1_COMPA interrupts on an attiny85

[johnnynotsolucky]

I have an attiny85 with a RGB LED's red leg connected to PB3, and I'm trying to toggle it using a timer interrupt. I cannot work out why I can't get TIMER0_COMPA or TIMER1_COMPA to fire, I can only get them working in Arduino. I think I've ported the code to rust exactly how I have it in arduino.

The working Arduino code:

const byte red_led = 3;

volatile byte counter = 0;
volatile bool toggle = false;

void useTimer0() {
  //  TCCR0A = 0b10000010;
  TCCR0A = (1 << WGM01) | (1 << COM0A1); // CTC mode
  //  TCCR0B = 0b00000101;
  TCCR0B = (1 << CS02) | (1 << CS00); // 1024 prescaler
  OCR0A = 250;
  //  TIMSK = 0b00010000;
  TIMSK = (1 << OCIE0A);
}

void useTimer1() {
  //  TCCR1 = 0b10011100;
  TCCR1 = (1 << CTC1) | (1 << COM1A0) | (12 << CS10); // CTC, toggle, 2048 prescaler
  //  GTCCR = 0b00010000;
  GTCCR = (1 << COM1B0);
  //  TIMSK = 0b01000000;
  TIMSK = (1 << OCIE1A);
  OCR1C = 255;
}

void setup() {
  pinMode(red_led, OUTPUT);
  //  useTimer1();
  useTimer0();
  sei();
}

void count() {
  counter = counter + 1;
  if (counter > 30) {
    toggle = !toggle;
    counter = 0;
  }
}

ISR(TIMER0_COMPA_vect)
{
  count();
}

ISR(TIMER1_COMPA_vect) {
  count();
}

void loop() {
  digitalWrite(red_led, toggle);
}

The code I've ported to rust:

use core::{cell::Cell, sync::atomic::AtomicBool};

use arduino_hal::pac::{TC0, TC1};
use avr_device::interrupt::Mutex;
use core::sync::atomic::Ordering;
use panic_halt as _;

static COUNTER: Mutex<Cell<u8>> = Mutex::new(Cell::new(0));
static TOGGLE: AtomicBool = AtomicBool::new(false);

fn count() {
    avr_device::interrupt::free(|cs| {
        let counter = COUNTER.borrow(cs);
        let value = counter.get();
        if value > 30 {
            counter.set(0);
            TOGGLE.store(!TOGGLE.load(Ordering::SeqCst), Ordering::SeqCst);
        } else {
            counter.set(value + 1);
        }
    });
}
#[avr_device::interrupt(attiny85)]
fn TIMER0_COMPA() {
    count();
}

/// Use TIMER0_COMPA to toggle an LED on/off
/// Does not work.
fn timer0(tc0: &TC0) {
    tc0.tccr0a.write(|w| unsafe { w.bits(0b10000010) });
    tc0.tccr0b.write(|w| unsafe { w.bits(0b00000101) });
    tc0.ocr0a.write(|w| unsafe { w.bits(250) });
    tc0.timsk.write(|w| unsafe { w.bits(0b00010000) });
}

#[avr_device::interrupt(attiny85)]
fn TIMER1_COMPA() {
    count();
}

/// Use TIMER1_COMPA to toggle an LED on/off
/// This works exactly as it does with the c code used in arduino ide.
fn timer1(tc1: &TC1) {
    tc1.tccr1.write(|w| unsafe { w.bits(0b10011100) });
    tc1.gtccr.write(|w| unsafe { w.bits(0b00010000) });
    tc1.timsk.write(|w| unsafe { w.bits(0b01000000) }); // (1<<OCIE1A)
    tc1.ocr1c.write(|w| unsafe { w.bits(255) });
}

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

    let mut red = pins.d4.into_output_high();

    let tc0 = dp.TC0;
    // timer0(&tc0); // Doesn't seem to trigger TIMER0_COMPA
    let tc1 = dp.TC1;
    timer1(&tc1); // Doesn't seem to trigger TIMER1_COMPA

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

    loop {
        let is_on = avr_device::interrupt::free(|_cs| TOGGLE.load(Ordering::SeqCst));

        // I'm using an RGB LED with a common anode, so set_low() turns the light on.
        if is_on {
            red.set_low();
        } else {
            red.set_high();
        }
    }
}

I've used the binary values for setting the registers so I can match them against the datasheet easily, and have tested those exact values in the arduino code too.

I'm pretty sure I'm missing something crucial, but I just cannot see what. Any pointers would be hugely appreciated!

P.S. The actual frequency the LED is toggled is irrelevant, as long as the light toggles

P.P.S This is an amazing library, thank you!

Update: Edited to show that I actually couldn't get TIMER1_COMPA working, I had set the TOIE1 bit instead of the OCIE1A bit. With the TOIE1 bit set, it pulses on PB4 based off what was written to the OCR1C register (I think).

[johnnynotsolucky]

Quite embarrassingly, I had the wrong avr-spec configured in config.toml.

I had duplicated my project from another arduino nano project, and seemingly changed everything but the build target.

[Rahix]

Glad you got it figured out! Btw, your code looks a bit odd:

First of all, in this line:

let is_on = avr_device::interrupt::free(|_cs| TOGGLE.load(Ordering::SeqCst));

The critical section is superfluous. "Atomics" do not need a critical section by their nature of only accessing the value atomically.

Then, looking at this:

static COUNTER: Mutex<Cell<u8>> = Mutex::new(Cell::new(0));
static TOGGLE: AtomicBool = AtomicBool::new(false);

fn count() {
    avr_device::interrupt::free(|cs| {
        let counter = COUNTER.borrow(cs);
        let value = counter.get();
        if value > 30 {
            counter.set(0);
            TOGGLE.store(!TOGGLE.load(Ordering::SeqCst), Ordering::SeqCst);
        } else {
            counter.set(value + 1);
        }
    });
}

You are using both "atomics" and a mutex at the same time. This does not really make sense and just produces unnecessary overhead. When there's already a mutex involved, just use mutexes for the rest as well:

static COUNTER: Mutex<Cell<u8>> = Mutex::new(Cell::new(0));
static TOGGLE: Mutex<Cell<bool>> = Mutex::new(Cell::new(false));

fn count() {
    avr_device::interrupt::free(|cs| {
        let counter = COUNTER.borrow(cs);
        let toggle = TOGGLE.borrow(cs);

        let value = counter.get();
        if value > 30 {
            counter.set(0);
            toggle.set(!toggle.get());
        } else {
            counter.set(value + 1);
        }
    });
}

There is one more reason why you should do this: "Atomics" do not actually exist on AVR. The AtomicBool type (and all the other ones) are just emulating atomics by using a critical section under the hood. So they produce the exact same machine code as the Mutex approach.

[johnnynotsolucky]

Thank you for pointing this out @Rahix! I had assumed that mutating anything from interrupt handler needed to be inside an explicit critical section. This makes perfect sense, and probably should've been quite obvious.

[Rahix]

Actually, now that you mention it, interrupt handlers are even more special... On AVR, interrupts will always be disabled while the interrupt handler is running. So technically the critical section there wouldn't actually be needed because the CPU already enforces it. We've been thinking about passing a cs token to all interrupt handlers by default to allow writing code based on this fact (see Rahix/avr-device#52).

[johnnynotsolucky]

That makes sense. Further, at least for the Attiny85, I think that interrupts will only fire in order of priority, so if at any point multiple interrupts are triggered, the CPU guarantees that PCINT0 will run before TIMER1_COMPA, for example.

Would explicitly having a critical section in an interrupt handler just be good practice to indicate that this handler is changing volatile data until #52 is added to avr-devices?

[Rahix]

I would add the critical section only when it is needed. After all, you can't safely get to the contents of a mutex without the cs token so as long as you're not using unsafe code, you only need the critical section where mutex access forces you to use one.

[johnnynotsolucky]

Perfect. Thank you for all your feedback ❤️