arduino-timer icon indicating copy to clipboard operation
arduino-timer copied to clipboard

Improvement: Replace func handler by std::function

Open Belphemur opened this issue 2 years ago • 7 comments

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.

Belphemur avatar Jan 25 '22 00:01 Belphemur

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.

kpfleming avatar Jan 25 '22 01:01 kpfleming

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.

contrem avatar Jan 25 '22 01:01 contrem

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.

Belphemur avatar Jan 25 '22 01:01 Belphemur

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.

kpfleming avatar Jan 25 '22 11:01 kpfleming

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:

  • auto and decltype
  • constexpr
  • lambda expressions
  • nullptr
  • range-for
  • much more...

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

skrobinson avatar Jan 31 '22 17:01 skrobinson

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 ...

  • While each build is successful, there are differences in the compilers. Some platforms show more warnings than others. Warnings are ignored.
  • The builds only compile the examples. If no example exercises a feature it won't be checked. (If you added a new macro, will a build verify the macro works on all platforms? You probably want an example anyway.)
  • arduino-timer does work for other platforms; we just don't test them. It's a nice general-purpose library with minimal dependencies.

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.

philj404 avatar Jan 31 '22 23:01 philj404

"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:

  • GCC 4.8.1 May 2013
  • Arduino 1.6.0 (with avr-gcc-4.8.1) February 2015
  • arduino-timer 0.0.1 October 2018.

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.

skrobinson avatar Feb 01 '22 21:02 skrobinson