Allow setting skip_unhandled_events on esp timer

[julienvincent]

Currently the esp timer service hardcodes the skip_unhandled_events flag to false which means there is no way to construct a timer that doesn't wake the device from light sleep.

This change introduces a skip_unhandled_events fn on the EspTimerService which allows changing the value of this timer flag.

---

I added this as a mutating fn on the timer service - but I wonder if it's a better design to make the new constructor accept this flag. I didn't want to change it's signature though to avoid introducing a breaking change.

[ivmarkov]

It is desirable to keep backwards compatibility in EspTimerService indeed. While we broke backwards compatibility just a couple of days ago in EspNow, EspNow is a bit of a niche, leaf-only code, while EspTimerService is used everywhere.

With that said, I don't think it is ideal to have the newly introduced flag on the global EspTimerService level. Isn't it better to introduce it (in the form of new functions) when you are creating a timer instead? I.e. with extra, new timer_* methods?

[ivmarkov]

As per my other comment.

[julienvincent]

With that said, I don't think it is ideal to have the newly introduced flag on the global EspTimerService level. Isn't it better to introduce it (in the form of new functions) when you are creating a timer instead? I.e. with extra, new timer_* methods?

@ivmarkov Ok I tried this out but it just seems a bit messy to me. There then needs to be multiple new methods all doing the same thing sans flag.

See 8af264d for WIP implementation of the above.

Is this what you had in mind?

[ivmarkov]

With that said, I don't think it is ideal to have the newly introduced flag on the global EspTimerService level. Isn't it better to introduce it (in the form of new functions) when you are creating a timer instead? I.e. with extra, new timer_* methods?

@ivmarkov Ok I tried this out but it just seems a bit messy to me. There then needs to be multiple new methods all doing the same thing sans flag.

See 8af264d for WIP implementation of the above.

Is this what you had in mind?

Yes, that's what I had in mind.
It might be a bigger change, but it is still might be a bit less risky than your original suggestion, in that while the EspTimerService is not technically a singleton, it is used as such, and it might become a singleton in future.

If that happens, then you have to decide - for your whole app - if you want ALL timers to skip events or not skip events. Hence why I think it is better to have it on a per-timer level, which mostly follows the ESP IDF API/approach anyway.

If you could lower a bit the copy-pasted methods in your 8af264d (perhaps, by introducing an extra internal_* method or two), then I think that would be the better suggestion, yes.

[julienvincent]

Thanks for the explanation that all makes perfect sense.

I've made the requested changes, including deduplicating the timer_async implementation.

[ivmarkov]

Thanks!