raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.65k stars 905 forks source link

hardware_alarm_irq_handler uses wrong value to determine `timer_num` when using timer 1. #1948

Open ajf58 opened 4 hours ago

ajf58 commented 4 hours ago

I think there's a logic error in here, which means TIMER1 doesn't work as expected.

Looking at this snippet:

static void hardware_alarm_irq_handler(void) {
    // Determine which timer this IRQ is for
    uint alarm_num = TIMER_ALARM_NUM_FROM_IRQ(__get_current_exception() - VTABLE_FIRST_IRQ);
    check_hardware_alarm_num_param(alarm_num);
    uint timer_num = TIMER_NUM_FROM_IRQ(alarm_num);

alarm_num is always a value 0-3. This is then used as the IRQ number on line 150:

    uint timer_num = TIMER_NUM_FROM_IRQ(alarm_num);

which is fine when using TIMER0, but won't work with TIMER1, because alarm_num is being used instead of irq_num. In contrast, https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/pico_time_adapter/include/pico/time_adapter.h#L31-L36 seems to do the correct thing:

static inline alarm_pool_timer_t *ta_from_current_irq(uint *alarm_num) {
    uint irq_num = __get_current_exception() - VTABLE_FIRST_IRQ;
    alarm_pool_timer_t *timer = timer_get_instance(TIMER_NUM_FROM_IRQ(irq_num));
    *alarm_num = TIMER_ALARM_NUM_FROM_IRQ(irq_num);
    return timer;
}

i.e. it calculates irq_num and uses that as part of TIMER_NUM_FROM_IRQ(irq_num).

If I rewrite this with the following snippet, my tests pass as expected:

void hardware_alarm_irq_handler(void* data) {
    // Determine which timer this IRQ is for
    uint irq_num = __get_current_exception() - VTABLE_FIRST_IRQ;     // Create new intermediate variable.
    uint alarm_num = TIMER_ALARM_NUM_FROM_IRQ(irq_num);     // Use here (behaviour unchanged).
    check_hardware_alarm_num_param(alarm_num);
    uint timer_num = TIMER_NUM_FROM_IRQ(irq_num);                  // And here, correcting the issue.
    timer_hw_t *timer = timer_get_instance(timer_num);
    hardware_alarm_callback_t callback = NULL;

This seems to affect both the 2.0 and current HEAD of the develop branch.

Found as part of work on https://github.com/zephyrproject-rtos/zephyr/pull/77368

kilograham commented 4 hours ago

Good catch; fancy making a PR?

ajf58 commented 3 hours ago

Good catch; fancy making a PR?

Sure! See #1949 .