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

hw_timer_t can't be retriggered from inside an interrupt in 2.0.2+ #6693

Closed
1 task done
sweetlilmre opened this issue May 5, 2022 · 18 comments
Closed
1 task done
Assignees
Labels
Area: Peripherals API Relates to peripheral's APIs. Type: Question Only question

Comments

@sweetlilmre
Copy link
Contributor

sweetlilmre commented May 5, 2022

Board

ESP32 Dev Module

Device Description

https://www.banggood.com/ESP32-WiFi-+-bluetooth-Development-Board-Ultra-Low-Power-Consumption-Dual-Core-ESP-32-ESP-32S-Similar-ESP8266-Geekcreit-for-Arduino-products-that-work-with-official-Arduino-boards-p-1175488.html

Hardware Configuration

16x2 LCD connected via I2C
SD card connected via SDMMC pins
Rotary encoder connected to general IO

Version

v2.0.2

IDE Name

Platformio 4.2.0 Visual Studio Code

Operating System

Windows 10 x64

Flash frequency

80 MHz

PSRAM enabled

no

Upload speed

921600

Description

The code below works in framework-arduinoespressif32 3.20001.0 (2.0.1) and below, but not 2.0.2 and above, including https://github.com/espressif/arduino-esp32.git#142fceb8563cd1795d619829e0a103770a344e1a (which I believe is 2.0.3 as based on the info from this issue: #6689 )

In 2.0.1 or below the serial output shows an increasing value for 'tick', but in any framework version greater than 2.0.1, 'tick' stays at 1 indicating that the interrupt is not retriggered by the timer code inside of it.

Using the PIO Arduino support 4.1.0 (2.0.1) <- Works, tick increments
Using the PIO Arduino support 4.2.0 (2.0.2) <- Does not work, tick is 1 (interrupt fired once)
Using the PIO Arduino support 4.2.0 (2.0.3 from the git hash mentioned above) <- Does not work, tick is 1 (interrupt fired once)

Sketch

#include <Arduino.h>

hw_timer_t *signalTimer;
#define IDLE_TIMER_EXECUTE 1000
volatile uint32_t tick = 0;

void IRAM_ATTR signalTimerFunc()
{
  tick++;
  timerWrite(signalTimer, 0);
  // IDLE_TIMER_EXECUTE is used here for simplicity. The actual next interval would be calculated in the ISR
  timerAlarmWrite(signalTimer, IDLE_TIMER_EXECUTE, false);
  timerAlarmEnable(signalTimer);
}

void startTimer()
{
  signalTimer = timerBegin(0, 40, true);
  timerAttachInterrupt(signalTimer, &signalTimerFunc, true);
  timerWrite(signalTimer, 0);
  timerAlarmWrite(signalTimer, IDLE_TIMER_EXECUTE, false);
  timerAlarmEnable(signalTimer);
}

void stopTimer()
{
  timerAlarmDisable(signalTimer);
  timerDetachInterrupt(signalTimer);
  timerEnd(signalTimer);
  signalTimer = NULL;
}

void setup()
{
  Serial.begin(115200);
  Serial.println("Starting timer");
  startTimer();
}

void loop()
{
  delay(10);
  Serial.printf("tick: %d\n", tick);
}

Debug Message

No debug, just the serial output as described.

Other Steps to Reproduce

No response

I have checked existing issues, online documentation and the Troubleshooting Guide

  • I confirm I have checked existing issues, online documentation and Troubleshooting guide.
@sweetlilmre sweetlilmre added the Status: Awaiting triage Issue is waiting for triage label May 5, 2022
@VojtechBartoska
Copy link
Contributor

Hello, are you please able to retest this with newly release 2.0.3 stable version please?

@VojtechBartoska VojtechBartoska added Area: Peripherals API Relates to peripheral's APIs. Resolution: Awaiting response Waiting for response of author and removed Status: Awaiting triage Issue is waiting for triage labels May 5, 2022
@sweetlilmre
Copy link
Contributor Author

sweetlilmre commented May 5, 2022

Hi I will retest, just need confirmation on how I would set this up with Platformio. I am assuming that I can do something like this in my platformio.ini file:

platform_packages = framework-arduinoespressif32 @ https://github.com/espressif/arduino-esp32.git#2.0.3

I'll try this and get back to you.

Edit: I am specifying this to be clear:

platform = https://github.com/platformio/platform-espressif32.git#v4.2.0

platform_packages = framework-arduinoespressif32 @ https://github.com/espressif/arduino-esp32.git#2.0.3

@sweetlilmre
Copy link
Contributor Author

sweetlilmre commented May 5, 2022

Just tested with the configuration below and the issue still persists:

Edit: doing further testing from a completely clean platformio installation.

CONFIGURATION: https://docs.platformio.org/page/boards/espressif32/esp32dev.html
PLATFORM: Espressif 32 (4.2.0+sha.6f9e45a) > Espressif ESP32 Dev Module
HARDWARE: ESP32 240MHz, 320KB RAM, 4MB Flash
DEBUG: Current (cmsis-dap) External (cmsis-dap, esp-prog, iot-bus-jtag, jlink, minimodule, olimex-arm-usb-ocd, olimex-arm-usb-ocd-h, olimex-arm-usb-tiny-h, olimex-jtag-tiny, tumpa)
PACKAGES:
- framework-arduinoespressif32 2.0.3+sha.142fceb
- tool-esptoolpy 1.30300.0 (3.3.0)
- tool-mkfatfs 2.0.1
- tool-mklittlefs 1.203.210628 (2.3)
- tool-mkspiffs 2.230.0 (2.30)
- toolchain-xtensa-esp32 8.4.0+2021r2-patch3

@sweetlilmre
Copy link
Contributor Author

Full clean platformio install with 2.0.3 and the issue persists:

Processing esp32dev (platform: platform = espressif32 @ 4.2.0; board: esp32dev; framework: arduino)
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------Tool Manager: Installing platformio/tool-mkspiffs @ ~2.230.0
Downloading  [####################################]  100%
Unpacking  [####################################]  100%
Tool Manager: tool-mkspiffs @ 2.230.0 has been installed!     
Tool Manager: Installing platformio/tool-mklittlefs @ ~1.203.0
Downloading  [####################################]  100%
Unpacking  [####################################]  100%
Tool Manager: tool-mklittlefs @ 1.203.210628 has been installed!
Tool Manager: Installing platformio/tool-mkfatfs @ ~2.0.0       
Downloading  [####################################]  100%
Unpacking  [####################################]  100%
Tool Manager: tool-mkfatfs @ 2.0.1 has been installed!
Verbose mode can be enabled via `-v, --verbose` option
CONFIGURATION: https://docs.platformio.org/page/boards/espressif32/esp32dev.html
PLATFORM: Espressif 32 (4.2.0) > Espressif ESP32 Dev Module
HARDWARE: ESP32 240MHz, 320KB RAM, 4MB Flash
DEBUG: Current (cmsis-dap) External (cmsis-dap, esp-prog, iot-bus-jtag, jlink, minimodule, olimex-arm-usb-ocd, olimex-arm-usb-ocd-h, olimex-arm-usb-tiny-h, olimex-jtag-tiny, tumpa)
PACKAGES:
 - framework-arduinoespressif32 2.0.3+sha.142fceb
 - tool-esptoolpy 1.30300.0 (3.3.0)
 - tool-mkfatfs 2.0.1
 - tool-mklittlefs 1.203.210628 (2.3)
 - tool-mkspiffs 2.230.0 (2.30)
 - toolchain-xtensa-esp32 8.4.0+2021r2-patch3

@P-R-O-C-H-Y P-R-O-C-H-Y self-assigned this May 5, 2022
@P-R-O-C-H-Y
Copy link
Member

P-R-O-C-H-Y commented May 5, 2022

Hi @sweetlilmre,
Your implementation is wrong. The functions timerAlarmWrite(signalTimer, IDLE_TIMER_EXECUTE, false); the last parameter is, if you want to repeat the alarm. Set it to TRUE and comment out timerAlarmWrite(signalTimer, IDLE_TIMER_EXECUTE, false); and timerAlarmEnable(signalTimer); from void IRAM_ATTR signalTimerFunc() :)

@sweetlilmre
Copy link
Contributor Author

sweetlilmre commented May 5, 2022

Hi @P-R-O-C-H-Y 

Thank you for your response. The sample I have provided is just the smallest possible example of the functionality I am using in a bigger project.

I do not want the timer to repeat as per your code above. In the actual project:

  • the next timer interval is calculated in the ISR
  • IO lines are set high / low
  • the timer is set to re-trigger at the next calculated interval.
  • effectively this is a data based signal generator

So my implementation is logically correct and works as stated for anything pre 2.0.2. If there is a change in the API that would require me to implement this functionality differently then that would be helpful to understand.

I have updated the example code to make the use-case explicit. Apologies for any confusion caused.

@VojtechBartoska VojtechBartoska added the Type: Question Only question label May 5, 2022
@atanisoft
Copy link
Collaborator

@sweetlilmre remove the call to timerAlarmEnable from signalTimerFunc, it shouldn't be necessary and may actually be resetting your parameters!

Be warned however, using HW Timers to adjust GPIO pins WILL generate inconsistencies in your signal! I initially was using the timers in this code and moved to the RMT for accuracy/compliance purposes. You can see some externally captured timing here.

@sweetlilmre
Copy link
Contributor Author

Hi @atanisoft I tried your suggestion. Unfortunately this does not solve the 2.0.3 issue and additionally stops the timer from re-firing in 2.0.1 so it seems to be necessary in this "repeating one-shot" context. 

Luckily signal inconsistency is not a huge issue in my application and it definitely works for use-case.

Thank you very much for the links, I've been meaning to look into the RMT stuff for a while but the complexity scared me off.

@sweetlilmre
Copy link
Contributor Author

tl;dr I can achieve the desired behaviour on 2.0.1 and 2.0.2 (I'm assuming that 2.0.3+ will also work) by doing the following:

  1. Set the timer to auto reload.
  2. adjust the timer alarm using timerAlarmWrite() in the ISR.

In other words, not using a one-shot, but adjusting repeat time.

I've looked into this in some more detail and found the following:

The IDF 4.4. timer driver code contains the following logic:

            uint64_t old_alarm_value = 0;
            timer_hal_get_alarm_value(&(timer_obj->hal), &old_alarm_value);
            // call user registered callback
            is_awoken = timer_obj->timer_isr_fun.fn(timer_obj->timer_isr_fun.args);
            // reenable alarm if required
            uint64_t new_alarm_value = 0;
            timer_hal_get_alarm_value(&(timer_obj->hal), &new_alarm_value);
            bool reenable_alarm = (new_alarm_value != old_alarm_value) || timer_hal_get_auto_reload(&timer_obj->hal);
            timer_hal_set_alarm_enable(&(timer_obj->hal), reenable_alarm);

With the specific logic in question being: bool reenable_alarm = (new_alarm_value != old_alarm_value) || timer_hal_get_auto_reload(&timer_obj->hal);

So two conditions will result in the re-arming of the timer alarm:

  • The timer alarm has been set to auto reload.
  • The timer alarm value has been altered.

Digging further, the example supplied in the documentation: https://github.com/espressif/esp-idf/blob/58f378602faa075585e710df3d0803f62db8e331/examples/peripherals/timer_group/main/timer_group_example_main.c

Uses timer_group_get_counter_value_in_isr() and timer_group_set_alarm_value_in_isr() to retrigger the timer if it is not set to auto reload.

timer_group_set_alarm_value_in_isr() is in IRAM so would be preferable to call, but is not exposed.
timerAlarmWrite() calls timer_set_alarm_value() which does a similar set of operations.

At any rate I now have a solution that works pre and post the 2.0.2 move the the IDF 4.4 timer driver.

@VojtechBartoska
Copy link
Contributor

@sweetlilmre Can I close this as solved?

@sweetlilmre
Copy link
Contributor Author

@VojtechBartoska it can as I now have a means to make the timer work in the way I'd like it to. I was thinking of contributing an example sketch to demonstrate this, if that would be of interest?

@sweetlilmre
Copy link
Contributor Author

Closing

@VojtechBartoska
Copy link
Contributor

Hi @sweetlilmre, sorry for replying a bit late but I was an vacation. Example sketch sounds good, what do you think @P-R-O-C-H-Y?

@VojtechBartoska VojtechBartoska removed the Resolution: Awaiting response Waiting for response of author label Jun 27, 2022
@P-R-O-C-H-Y
Copy link
Member

@sweetlilmre Hi, did you have a time for a sketch to demonstrate this :) Thanks

@sweetlilmre
Copy link
Contributor Author

@P-R-O-C-H-Y firstly let me apologize for the long wait, many life things happened that I will not bore you with.

Here is a sample sketch to demonstrate my required functionality which seems to work across framework versions from 2.0.2 up to 2.0.5. Let me know if you would like any additional examples or changes:

#include <Arduino.h>

/*
  This sketch demonstrates a periodic timer that changes the timer period inside of the interrupt function.
  The initial trigger period is defined by trigger_period and incremented on each interrupt by
  trigger_period_increment.
  The timer runs at 1Mhz

  measured_time reports the actual time difference between interrupt calls as measured by the difference in value
  of esp_timer_get_time()
*/

const uint64_t trigger_period_increment = 100000;
volatile uint64_t trigger_period = trigger_period_increment;
volatile uint64_t measured_time = 0;
volatile uint64_t last_measured_time = 0;
volatile uint64_t last_interrupt_time;
hw_timer_t *signalTimer;

void IRAM_ATTR signalTimerFunc()
{
  uint64_t interrupt_time = esp_timer_get_time();
  measured_time = interrupt_time - last_interrupt_time;
  last_interrupt_time = interrupt_time;
  trigger_period += trigger_period_increment;
  timerAlarmWrite(signalTimer, trigger_period, true);
}

void startTimer()
{
  // 1 MHz timer
  signalTimer = timerBegin(0, 80, true);
  timerAttachInterrupt(signalTimer, &signalTimerFunc, true);
  timerWrite(signalTimer, 0);
  timerAlarmWrite(signalTimer, trigger_period, true);
  timerAlarmEnable(signalTimer);
  last_interrupt_time = esp_timer_get_time();
}

void stopTimer()
{
  timerAlarmDisable(signalTimer);
  timerDetachInterrupt(signalTimer);
  timerEnd(signalTimer);
  signalTimer = NULL;
}

void setup()
{
  Serial.begin(115200);
  Serial.println("Starting timer");
  startTimer();
}

void loop()
{
  if (last_measured_time != measured_time)
  {
    Serial.printf("measured_time: %llu\n", measured_time);
    last_measured_time = measured_time;
  }
}

@x29a
Copy link

x29a commented Apr 20, 2023

@VojtechBartoska can you reopen this bug? stumbled across this in 2.0.7 and 2.0.8, having the same use-case as @sweetlilmre (thanks for the investigation!).

Use-Case: Have a single-shot timer which determines the next firing-timestamp in the ISR, so it "hops" from event to event at different time intervals.

For this it should be sufficient to set a new target counter and (re-)start the timer, or?

BTW, used a counting-downwards timer, same faulty behaviour (beginTimer(0, 80, false);), so that does not make a difference.

@VojtechBartoska
Copy link
Contributor

VojtechBartoska commented Apr 21, 2023

@P-R-O-C-H-Y Can you please take a look on this again? Thanks

@P-R-O-C-H-Y
Copy link
Member

Hello @x29a, have you tried the approach from @sweetlilmre? This case is not a bug btw, but it's more about what you can / can't run within ISR context.

On the other hand I have good news. With the new version 3.0.0 which we are currently working on, the timer is already refactored and from what I remember and tested, all function will be allowed to run within ISR context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Peripherals API Relates to peripheral's APIs. Type: Question Only question
Projects
None yet
Development

No branches or pull requests

5 participants