InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.65k stars 912 forks source link

Event messaging is unsafe and rigid #1644

Open Riksu9000 opened 1 year ago

Riksu9000 commented 1 year ago

Verification

Introduce the issue

With the PushMessage functions, there's a lot of opportunities for errors. Anything can push anything through that function. The classes, such as TimerController have a hard dependency on SystemTask and cannot be reused.

Preferred solution

I had the idea that we could use std::function to pass an event callback function to classes. This way classes can be made much more generic without SystemTask dependencies, and safer.

https://github.com/Riksu9000/InfiniTime/commit/e67451d15e10716bf9c0a4e30edd86d755068430 Here's how this could be used to make TimerController much more generic. The issue with this is that it seems to use more memory, and I'm not sure why that is. If you can, please review this idea and let me know what you think. Is this a good approach? Can it be improved?

Version

No response

JF002 commented 1 year ago

I don't know all the internals of std::function, but, from what I know, std::function and lambda can be very optimized (for example, in some conditions like when the capture is empty, they are optimized as a simple function pointer) or very heavy (with dynamic allocation and a lot of processing).

In this case, I guess the increased memory usage comes from the capture in the lambda.

Riksu9000 commented 1 year ago

I was able to reduce the memory usage of my idea a bit. https://github.com/Riksu9000/InfiniTime/commit/469ce4d2e639f9eedc79cca97f3d2cdbaceb7eae