raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.62k stars 901 forks source link

add_repeating_timer_us only works with some values of delay_us #737

Closed thaumatichthys closed 1 month ago

thaumatichthys commented 2 years ago

add_repeating_timer_us(-50, isr, NULL, &timer); is fine, but add_repeating_timer_us(-51, isr, NULL, &timer); will hang the pico.

A workaround is to call add_repeating_timer with a working delay value, and write the wanted value to the timer struct. timer.delay_us = -51

I'm not sure if this is mentioned in the C++ SDK documentation, but I could not find any mention of this.

kilograham commented 2 years ago

"hang the pico" really? in what way?

a small example showing the problem would help (it probably matters what your callback is doing)

thaumatichthys commented 2 years ago

it might be that my sdk is not entirely up to date, but for me it will stop running the code when it hits add_repeating_timer_us with some delays

main.cpp:

#include "hardware/gpio.h"

#define timer_delay -51

struct repeating_timer timer;

bool timer_isr(struct repeating_timer *t) {
    return true;
}

void setUpTimer() {
    add_repeating_timer_us(timer_delay, timer_isr, NULL, &timer);
}

int main() {

    gpio_init(25);
    gpio_set_dir(25, GPIO_OUT);

    setUpTimer();

    while(1) {
        gpio_put(25, 1);
        sleep_ms(200);
        gpio_put(25, 0);
        sleep_ms(200);
    }
}

CMakeLists.txt:

cmake_minimum_required(VERSION 3.21)

include(pico_sdk_import.cmake)

project(test_project C CXX ASM)

set(CMAKE_C_STANDARD 11)
set(CMAKE_CXX_STANDARD 20)

pico_sdk_init()

add_executable(${PROJECT_NAME}
    main.cpp
)

target_link_libraries(${PROJECT_NAME}
    pico_stdlib
    pico_multicore
    hardware_gpio
)

pico_enable_stdio_usb(${PROJECT_NAME} 1)
pico_enable_stdio_uart(${PROJECT_NAME} 0)

pico_add_extra_outputs(${PROJECT_NAME})

the LED will not blink if timer_delay is -51, but it will if its -50.

jibonaronno commented 2 years ago

Same issue here.

kilograham commented 2 years ago

is the issue only with negative numbers?

negative numbers means that there may be no pause between callbacks (if the number is too close to zero) and if there is no pause then you will appear to hang. it doesn't make a lot of sense that -51 would not work and -50 would, however there are a number of possible factors at play especially if you are running from (cold) flash.

Felix-000 commented 1 year ago

I have a similar issue: for me it seams like it doesn't work for values smaller than X. On one RP2040 it didn't work for times shorter than -107uS. On another I had issues while debugging around 84/85uS. If it did not work it always ended up in a breakpoint, highlighted by the debugger in: pico-sdk/src/rp2_common/pico_runtime/runtime.c line 186

More information in the comments of my code:

#include <stdio.h>
#include "pico/stdlib.h"
#include "hardware/timer.h"
#include "hardware/clocks.h"

#define PIN 1

bool repeating_timer_callback(struct repeating_timer *t) {
    gpio_put(PIN, !gpio_get(PIN));

    return true;
}

int main()
{
    stdio_init_all();

    gpio_init(PIN);
    gpio_set_dir(PIN, GPIO_OUT);

    struct repeating_timer timer;
    add_repeating_timer_us(-100, repeating_timer_callback, NULL, &timer);     // Working and test: -84, -85, -86, -90, -100, -150, -200, -500
                                                                              // Not working and tested: -83, -82, -81, -80, -79, -70, -51, -50, -25, -13, -12, -1, 0
                                                                              // I also test +20, +40, +84 were not working, +85, +86, +100 were working

                                                                              // 15min later -84 stopped working and it only worked with -85 (and assumably above (I only test -85,-86,-90,-100,-200 afterwards))

    // timer.delay_us = -15;            // By using that line I was able to use times shorter than +-84uS (or -85uS 15min later).

                                    // At times lower than -11uS I noticed a substantial variance of about -10% to +5% at -11uS, +-12% at -10uS and about +-15% at -9uS
                                    // I assume that is because the execution time is very close to the interval time
                                    // I tested the execution time of the ISR by measuring the edge to edge time when using "+1". I measdured arround 7.25uS => 6.25uS for the ISR

    while(1) {
        tight_loop_contents();
    }

    return 0;
}
JakubVanek commented 1 year ago

I think I might have found a workaround. I am hitting a similar-looking bug where timers on the default pool stop firing if the main program does a short usleep at an unfortunate time. I have modified the code near here like this:

diff --git a/src/rp2_common/hardware_timer/timer.c b/src/rp2_common/hardware_timer/timer.c
index f13d24996078..ed0f7d0c9d62 100644
--- a/src/rp2_common/hardware_timer/timer.c
+++ b/src/rp2_common/hardware_timer/timer.c
@@ -208,6 +208,9 @@ bool hardware_alarm_set_target(uint alarm_num, absolute_time_t target) {
                     // and clear our flag so that if the IRQ handler is already active (because it is on
                     // the other core) it will also skip doing anything
                     timer_callbacks_pending = old_timer_callbacks_pending;
+                } else {
+                    // HACK: Force the interrupt to happen
+                    irq_set_pending(harware_alarm_irq_number(alarm_num));
                 }
             }
         }

This resolves the problem for me - timers do not mysteriously stop firing anymore. However, I'm not sure that this is a correct solution and I'm not sure that this is the same bug. Does the workaround work for anyone else?

NolanBrad commented 4 months ago

This patch from @JakubVanek worked for my issue where the timer would stop working for 2^32us and then come back on the rollaround. I am assuming that this was also because of other users of the default alarm pool ( but wasnt proven) Thanks @JakubVanek!