timerAttachInterrupt() and timerDetachInterrupt() broken in 2.0.2+

[sweetlilmre]

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

latest master (checkout manually)

IDE Name

PlatformIO

Operating System

Windows 10 x64

Flash frequency

80 MHz

PSRAM enabled

no

Upload speed

921600

Description

Preamble:

After digging through a lot of code in my mission to figure out a resolution to issue #6693 I traced a calls from 2.0.1 through to master, to see what was happening in the timer code.
While I am no closer to a resolution for that issue, it seems that in the move to IDF, esp32-hal-timer.c has not been completely refactored.

Firstly:

void timerAttachInterrupt(hw_timer_t *timer, void (*fn)(void), bool edge){
    if(edge){
        log_w("EDGE timer interrupt is not supported! Setting to LEVEL...");
        edge = false;
    }
    timer_enable_intr(timer->group, timer->num);

    timer_info_t *timer_info = calloc(1, sizeof(timer_info_t));
    timer_info->timer_group = timer->group;
    timer_info->timer_idx = timer->num;
    timer_info->auto_reload = timerGetAutoReload(timer);
    timer_info->alarm_interval = timerAlarmRead(timer);

    timer_isr_callback_add(timer->group, timer->num, (timer_isr_t)fn, timer_info, 0);
}

1. timer_enable_intr(timer->group, timer->num); is called. This is handled in driver/timer.c in IDF-4.4, so I'm not sure why it's handled here.
2. the void (*fn)(void) call back is cast to timer_isr_t as typedef bool (*timer_isr_t)(void *); so there is no way to allow a yield as specified in the IDF docs (if needed), also the cast seems dodgy.
3. a 'timer_info' struct is allocated, never used and never freed (this would eventually be passed to the callback function, but as per (2) above this is not possible. See static void IRAM_ATTR timer_isr_default(void *arg) in driver/timer.c

Secondly:

void timerDetachInterrupt(hw_timer_t *timer){
    timerAttachInterrupt(timer, NULL, false);
}

This is the previous implementation from pre 2.0.2 code, where passing NULL to timerAttachInterrupt would effectively undo the attach. See 2.0.1 and previous code in esp32-hal-timer.c.

Suggestions:

For timerAttachInterrupt:

1. Remove all code except for the edge check and call to timer_isr_callback_add()
2. Use an intermediate function of the proper type to wrap void (*fn)(void) in a typedef bool (*timer_isr_t)(void *);
3. More work may be required here to determine if a yield is necessary

For timerDetachInterrupt:

1. Call timer_isr_callback_remove()

Sketch

Not applicable

Debug Message

N/A

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.

[P-R-O-C-H-Y]

@sweetlilmre Thanks for the report. I am adding that to roadmap.

[mrengineer7777]

Possible workaround: try Ticker class. It works great in our projects.

[sweetlilmre]

@mrengineer7777 Thanks for the suggestion. The Ticker class does not have a fine enough resolution (ms, I require us). I performed some tests using the esp_timer_* underlying functions which have the required resolution, unfortunately they are not quite as accurate as the timerBegin based API in pre 2.0.2. It may not matter in the actual implementation and the required  changes are not too intense, so I will see if this is an alternative.

[sweetlilmre]

@mrengineer7777 circling back here. I tried this out and there doesn't seem to be sufficient resolution / accuracy for my use-case.

[mrengineer7777]

@mrengineer7777 circling back here. I tried this out and there doesn't seem to be sufficient resolution / accuracy for my use-case.

Sorry to hear that. I'm just a fellow engineer, so you'll need to wait for one of the maintainers to fix the code. Try this: look at the history of changes on that library and see if you can spot the change that broke it. Any of us can suggest a fix via a PR.

[P-R-O-C-H-Y]

I think it was in 2.0.2 where timer was refactored to use ESP-IDF API as many other peripherals in order to work on all chips and be ready for future chip support. The timer itself isn't broken, it works as expected. But as you reported there are few thing that are unnecessary or forgotten to change when code was refactored.

[sweetlilmre]

I have submitted a PR.