Add Functional and Scheduled callback option to Ticker #4062
Conversation
Add Scheduled callback to Ticker
libraries/Ticker/Ticker.h
Outdated
|
|
||
| void attach(float seconds, callback_t callback) | ||
| void attach(float seconds, TickerFunction tf, bool reqSchedule = false) |
There was a problem hiding this comment.
I suggest making the case for the scheduled function more explicit.
E.g. attach_scheduled(10, myfunc) reads much better than attach(10, myfunc, true).
There was a problem hiding this comment.
Also, why did void attach(float seconds, void (*callback)(TArg), TArg arg) overload not get the version with "scheduled" option?
I would expect to see the product of {attach, once} x {sec, msec} x {no arg, arg} x {normal, scheduled}, i.e. 16 functions.
There was a problem hiding this comment.
There's no point in repeatedly attach the same callback as the callback will run after the loop and there no way (yet?) to stop that. Or do I miss something?
Just asking because I'm using the esp8266's interface in my esp32's version of the Ticker lib.
I like the more explicit version!
libraries/Ticker/Ticker.h
Outdated
| @@ -34,17 +36,22 @@ class Ticker | |||
| public: | |||
| Ticker(); | |||
| ~Ticker(); | |||
| typedef void (*callback_t)(void); | |||
There was a problem hiding this comment.
It might be better to keep this definition for compatibility. I ran grep on my sketchbook and found a few uses of Ticker::callback_t. Perhaps others could be using it as well.
| @@ -24,6 +24,8 @@ | |||
|
|
|||
| #include <stdint.h> | |||
| #include <stddef.h> | |||
| #include <functional> | |||
| #include <Schedule.h> | |||
There was a problem hiding this comment.
This header file is probably only needed in Ticker.cpp
libraries/Ticker/Ticker.h
Outdated
|
|
||
|
|
||
| protected: | ||
| ETSTimer* _timer; | ||
| TickerFunction internalTicker = nullptr; | ||
| bool scheduleTicker = false; |
There was a problem hiding this comment.
Instead of introducing a flag, can we do something like this for the "scheduled" case?
internalTicker = std::bind(schedule_function, tf);and
internalTicker = tf;for the normal (not scheduled)?
libraries/Ticker/Ticker.h
Outdated
|
|
||
|
|
||
| protected: | ||
| ETSTimer* _timer; | ||
| TickerFunction internalTicker = nullptr; |
There was a problem hiding this comment.
For consistency with the rest of this file, please use snake_case for member and function names, and prefix the member names with an underscore. Also, perhaps _callback would be a better name for this member? internalTicker makes it look like there is an instance of Ticker here.
|
This is the update for making it conistent with your remarks except
I did not overload these because they are now just a specific case of the nomal functions for The first is kept in to have backward compatibility. If you want the overload still included just let me know |
|
@hreintke Yes, please rebase on the current master. Thanks. |
|
Have some trouble getting it done but will solve it. |
|
Could only rebase by creating a new PR : #4209 |
|
Closing in favor of #4209 . |
Add Functional and Scheduled callback option to Ticker