contrem / arduino-timer

Non-blocking library for delaying function calls
BSD 3-Clause "New" or "Revised" License
331 stars 50 forks source link

Improvement: Replace func handler by std::function #59

Open Belphemur opened 2 years ago

Belphemur commented 2 years ago

Hello,

With the current definition of the func handler:

typedef bool (*handler_t)(T opaque); /* task handler func signature */

It's impossible to use std::bind when you want the timer to run a callback that is a member of the class.

by changing the definition to use std::function like

typedef std::function<bool(T opaque)> handler_t;

It should let the user use any type of supported callback and shouldn't break the current working of the library.

kpfleming commented 2 years ago

This is certainly true, and it would also allow the use of lambdas for callbacks, which can have benefits.

One the downside it will increase the size of the handler_t from 4 bytes to (probably) 32 bytes.

contrem commented 2 years ago

The other downside is that these are c++11 features, and we lose c++98 compatibility. It's worth discussing as the next planned release will likely be a major version release.

Belphemur commented 2 years ago

I admit I'm quite new into Arduino development, but I would argue that https://github.com/esp8266/Arduino seems to have made the move to C++11 sometimes ago.

I don't know if keeping compatibility with c++98 is still something that would make sense in 2022.

Technically, telling people that need c++98 to use a previous major version shouldn't be much of an issue, I imagine.

kpfleming commented 2 years ago

There are dozens of Arduino platforms that do not support C++11 and likely won't ever support it. That makes such a change for a library owner complicated, because they need to decide which platforms to support (or support both).

If you are really interested in using a 'modern C++' timer library, feel free to check out my library which was inspired by this one, but leverages C++11/14/17 features.

skrobinson commented 2 years ago

I'd like to see C++11 (or later) language use, but not an STL requirement.

Things that could be done with just a language update:

I have a fork branch that uses some of the above list.

philj404 commented 2 years ago

FYI The Github Action at https://github.com/contrem/arduino-timer/blob/master/.github/workflows/githubci.yml runs a build check for examples on some (not all) platforms.

We inherit much of this list from the scripts at adafruit/ci-arduino -- and so we don't have to maintain the script. As of January 2022 we get automated builds on:

SWITCHING TO arduino:avr:uno ... SWITCHING TO arduino:avr:leonardo ... SWITCHING TO arduino:samd:arduino_zero_native ... SWITCHING TO adafruit:samd:adafruit_qtpy_m0 ... SWITCHING TO esp8266:esp8266:huzzah:eesz=4M3M,xtal=80 ... SWITCHING TO esp32:esp32:featheresp32:FlashFreq=80 ... SWITCHING TO adafruit:samd:adafruit_metro_m4:speed=120 ... SWITCHING TO adafruit:samd:adafruit_trinket_m0 ... SWITCHING TO esp32:esp32:featheresp32:FlashFreq=80 ... SWITCHING TO esp8266:esp8266:huzzah:eesz=4M3M,xtal=80 ...

So really my point is just that we have quite a few variants and that we want it ALL to continue to work.

If we really need a new C++XX feature which really improves readability (or efficiency?), and is incompatible with some legacy platform, we should make a formal break. Let's make it clear which platforms need to use an earlier release of this library.

skrobinson commented 2 years ago

"GCC 4.8.1 was the first feature-complete implementation of the 2011 C++ standard..." -- https://gcc.gnu.org/projects/cxx-status.html#cxx11

Release dates:

How many arduino-timer-using projects have a C++98- or C++03-only language requirement? Does arduino-timer target other, older compilers? Answering these questions will help decide if a language feature is reasonable for this project.

If we really need a new C++XX feature which really improves readability

As a concrete example of a change that could be made:

- timer_foreach_const_task(task) {
+ for (const auto &task : tasks) {

I would say the readability is roughly the same, except that the first line is not what the compiler sees. One must interpret the timer_foreach_const_task macro to determine the actual code to be compiled.