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

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

Open
Belphemur opened this issue Jan 25, 2022 · 7 comments
Open

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

Belphemur opened this issue Jan 25, 2022 · 7 comments

Comments

@Belphemur
Copy link

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
Copy link

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
Copy link
Owner

contrem commented Jan 25, 2022

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
Copy link
Author

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
Copy link

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
Copy link
Contributor

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.

@philj404
Copy link
Contributor

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.

@skrobinson
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants