Ticker updated to match extensions in ESP8266 API

[dok-net]

This PR updates the Ticker class to the ESP8266 in corresponding PR there.

Additional info:
This also prepares for an enhancement in another PR that would provide a co-op single-thread, IRQ safe Scheduler.
Even on FreeRTOS a single-thread scheduler in the loop-context is useful: for most sketches OS-level multitasking is not necessary, but synchronization is a difficult effort to get right. Also, the single-thread performance of the ESP32 in measured tests is lower than that of current non-OS SDK ESP8266, so an efficient user-context scheduler for timed and IRQ-based tasks is useful - besides, the async programming pattern this affords is quite popular.

[dok-net]

@full-stack-ex You've recently made a change to Ticker, could you please review this PR? I've adopted the ULL integer literal fix from your patch.

[full-stack-ex]

Well, it looks like the function called from Timer accepts a 64 bit us time (period):

esp_err_t esp_timer_start_periodic(esp_timer_handle_t timer, uint64_t period);

But this one, which calls it, accepts 32 bits only (micros, passed to the above as period):

void Ticker::_attach_us(uint32_t micros, bool repeat, callback_with_arg_t callback, void* arg)

Should it also accept 64 bits, like this, to pass any valid value without truncation?

void Ticker::_attach_us(uint64_t micros, bool repeat, callback_with_arg_t callback, void* arg)

[dok-net]

@full-stack-ex Converted API to 64bit time values. Thanks!
@me-no-dev Code is again dissimilar enough to ESP8266, so it doesn't matter, moved most from header into compilation unit as you requested in review comments.

Good to merge now?

[full-stack-ex]

As far as 64 bit conversion is concerned:

Converted API to 64bit time values.

Good to go.

By the way, there is a side effect, though positive. Just in case, it might affect some documentation, tests, etc. I don't know of any. The old 32 bit millisecond range used to limit the period to UINT32_MAX ms, which is 4294967295/1000/60/60/24 = 49.7 days. The 64 bit millisecond range eliminates that constraint, and so the period may be any practical value. Say, a year, should anybody need that.

[dok-net]

@hreintke I don't understand - microseconds attach is there on ESP32. And, is the comment on "alternative timer usage" a suggestion to any action/change?

[hreintke]

@dok-net Sorry for the confusion. Did not check the PR properly.
You are right, it is already included.
Will delete my original comment.

[dok-net]

@me-no-dev Just a heads up :-) All reviewers gave their OK, I've recently followed your request to move as much from the header into the compilation unit as possible.
EDIT: I was asked by the ESP8266 devs to check the ROM size of the ticker patch there, and armed with those results, I've checked this PR on the ESP32. It turns out that the header inlining saves significant 204 and more bytes of flash memory. I'm moving the functions back into the header for that reason.

[Domochip]

Thanks a lot Dok-Net for this work. It think this PR is very usefull. I'm just trying to upgrade my first ESP8266 project to ESP32 and encounter this missing feature...

[fabianoriccardi]

Hi all,
is there any possibility to get this PR on master branch anytime soon?
Anyway, thanks for your work!

[mrengineer7777]

@dok-net Congrats on updating your PR. Unfortunately it deletes one of the examples and the third parameter is wrong on one of the calls.

I'm a fellow developer (not a maintainer), so I can't tell you what to do. That said, your PR introduces a lot of changes. It is more likely to get merged with just a few clear changes. For instance adding new example files.

I like the update on keywords.txt. I don't know if it is necessary (I don't use Arduino IDE), but seems reasonable.

I don't agree with changing _attach_ms() to _attach_us().

I'm curious about introducing Ticker::_static_callback(void* arg). How is that used?

Honestly at this point I would likely start with a fresh PR and just introduce minimal changes. I have successfully written and merged several PRs for this project. Happy to give feedback.

See example below of how I'm bringing over some changes from ESP8266. Note I reference prior PRs and code lines.
#7766
#7774

[dok-net]

"Congrats"? You're welcome.
You missed my last comment then? I only forgot to set the PR to draft. I am not expecting this to be of any more interest to the maintainers now than during the past 2.7 years. If you are interested in picking it up, please make a clone soon.

Whenever you spend your time and find issues in code, be as precise as you can when you mention them to the devs, just letting me know that I've got a (third) parameter wrong isn't quite so helpful.

I did just fix a warning that occurs in the TickerParameter.ino sample. I believe my original thinking was that the three examples in this PR are more comprehensible than the previous one, the one I have deleted. Nevermind.

Take care!

[mrengineer7777]

"Congrats"? You're welcome.

Thank you for your efforts.

Whenever you spend your time and find issues in code, be as precise as you can when you mention them to the devs, just letting me know that I've got a (third) parameter wrong isn't quite so helpful.

Correct. I didn't go into details because it sounds like you've given up on the PR. I don't blame you. It's hard to get a PR merged. I hear your frustration. Let's take a step back. What functionality would you like to see added?

[anabolyc]

Really sad to see that such useful change cannot be merged in 4 years. I'm working on few esp32 projects and constantly having issues with inability to use Ticker in classes, due to (honestly speaking) ridiculous type incompatibility. This would nail it, I assume.

[mrengineer7777]

@anabolyc I suspect the Ticker class is about to be refactored due to changes in IDF 5.1. What functionality are you looking for?

[anabolyc]

@mrengineer7777 I'm compiling this simple class

#pragma once

#include <Ticker.h>

class SampleClass
{
private:
    uint32_t lastUpdate = 0;
    Ticker tick;
public:
    SampleClass(/* args */) {};
    void init(void);
};

void SampleClass::init(void) {
    tick.attach(5, [&](){
        lastUpdate = millis();
    });
}

No issues compiling against esp8266, but on the esp32 getting

src/SampleClass.h:18:6: error: no matching function for call to 'Ticker::attach(int, SampleClass::init()::<lambda()>)'

[mrengineer7777]

@anabolyc
Try using attach_ms() instead of attach(). Check your timing!
Not sure if an inline function (lambda) is supported. Here's a working example:
https://github.com/espressif/arduino-esp32/blob/master/libraries/Ticker/examples/Blinker/Blinker.ino

[anabolyc]

@anabolyc Try using attach_ms() instead of attach(). Check your timing! Not sure if an inline function (lambda) is supported. Here's a working example: https://github.com/espressif/arduino-esp32/blob/master/libraries/Ticker/examples/Blinker/Blinker.ino

Point is, in the current shape it would only accept free function, then you need workarounds and compromises to access class members. While it just builds against esp8266 version.

[VojtechBartoska]

Status update:

• adding PR for Review and validation

[github-actions]

	Warnings
⚠️	Some issues found for the commit messages in this PR: the commit message "64bit integers instead of 32bits, timer functions on ESP32 accept 64bit integers.": summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message "Astyle from ESP8266": summary looks empty type/action looks empty the commit message "Changes after review": summary looks empty type/action looks empty the commit message "Default for LED_BUILTIN": summary looks empty type/action looks empty the commit message "Examples": summary looks empty type/action looks empty the commit message "Expose µs resolution of OS API in Ticker class": summary looks empty type/action looks empty the commit message "Fix a compiler warning due to c-style casting.": summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message "Fix omitted casts in template member function": summary looks empty type/action looks empty the commit message "Fixed Arduino keywords.txt": summary looks empty type/action looks empty the commit message "Fixing Build server complaints": summary looks empty type/action looks empty the commit message "Implementing inline in header saves 204+ bytes program size.": summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message "In Ticker, the *scheduled functions become available in another development branch.": summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message "More format reversions": summary looks empty type/action looks empty the commit message "Move code from header into compiliation unit.": summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message "Return Ticker to libraries only for modularity.": summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message "Revert": summary looks empty type/action looks empty the commit message "Revert": summary looks empty type/action looks empty the commit message "Revert": summary looks empty type/action looks empty the commit message "Test case same as ESP8266": summary looks empty type/action looks empty the commit message "Unify Ticker examples.": summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message "Update Ticker API to compatibility with ESP8266, prepares for co-op loop Scheduler": summary looks empty type/action looks empty Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).
⚠️	Please consider squashing your 23 commits (simplifying branch history).

👋 Hello dok-net, we appreciate your contribution to this project!

---

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 9a1c4e5

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

LGTM.