Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ajf58 opened this issue Sep 24, 2024 · 3 comments
Milestone

Comments

@ajf58
Copy link

ajf58 commented Sep 24, 2024

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 zephyrproject-rtos/zephyr#77368

@kilograham
Copy link
Contributor

Good catch; fancy making a PR?

ajf58 added a commit to ajf58/hal_rpi_pico that referenced this issue Sep 24, 2024
Correct the logic for determining `timer_num`. Previously this would
always evaluate to 0 due to using `alarm_num` instead of `irq_num`.

Fixes raspberrypi#1948.
@ajf58
Copy link
Author

ajf58 commented Sep 24, 2024

Good catch; fancy making a PR?

Sure! See #1949 .

@kilograham kilograham added this to the 2.0.1 milestone Sep 28, 2024
kilograham pushed a commit that referenced this issue Sep 28, 2024
Correct the logic for determining `timer_num`. Previously this would
always evaluate to 0 due to using `alarm_num` instead of `irq_num`.

Fixes #1948.
@kilograham
Copy link
Contributor

merged into develop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants