InfiniTimeOrg / InfiniTime

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

[WIP/RFC] Remove watchface enum #2040

Open JF002 opened 3 months ago

JF002 commented 3 months ago

In #1928 and #1934, we made it easier to select which apps and watch face will be built into the firmware.

However, those apps and watch faces must still be defined in the InfiniTime code base : the source code must be in src/displayapp and the enums WatchFace and Apps must list all the apps known to InfiniTime.

With this PR, I would like to make another steps toward splitting the InfiniTime "core" from the applications and watch faces by removing the WatchFace enum.

The end goal consist in allowing watch faces and apps to be defined in their own CMake project. Those projects would build a static lib (.a) for each watch face and application and the InfiniTime build system would simply link with them at build time. Those project could be added to InfinITime using git submodules for example.

The advantage of this approach is to allow applications and watch faces developer to manage their own project with their apps on their own, without any interventions of InfiniTime developers. Then, InfiniTime Core developers and fork maintainers would simply select the project they want to add in their own project.

For this to work, we need to remove as much dependencies as possible between apps/watch faces and InfiniTime : we need to be able to add them without making any changes in the InfiniTime code base.

The WatchFace and Apps enums do not play well with this goal : new apps and watch faces need to be added to this enum.

This PR removes the need for the WatchFace enum : watch faces are now identified by their type at build time and by their name at runtime.

The advantage of those changes is that they allow to make one step forward towards "external" application.

I'm less satisfied with the added code generated by CMake and with the string comparisons at runtime (previously, we just needed to check the value of the WatchFace enum - 1Byte, now, we need to compare a whole 16 chars string). But that might be the price to pay for more generic code.

This is WIP (work in progress), and the code generated by CMake needs to be improved (especially the generation of the include paths).

Any feedback/opinion about this?

github-actions[bot] commented 3 months ago
Build size and comparison to main: Section Size Difference
text 377736B 224B
data 940B 0B
bss 63556B 16B
mark9064 commented 2 months ago

The idea makes a lot of sense and sounds good to me. I don't see anyway to avoid the name checks - we need each watchface to have a unique identifier. We could have a 32 bit constant integer that each watchface stores (chosen randomly by watchface author/hash of name/whatever) instead, which would be faster to check. But I think that'd be a silly optimisation - doing ~5 string compares every now and then is not going to impact performance. It's only when loading the watchface (ignoring settings page etc as infrequently used), and then only once for each load. If you go from watchface to another screen 5 times per hour, that's only 25 string compares per hour - not a problem IMO

Unfortunately I don't know much about CMake so I can't offer any wisdom there!

liamcharger commented 2 months ago

I think this will be a good enhancement, as far as giving each watchface a way to be identified.

Especially as the main InfiniLink developer, there’s a feature where the app fetches the watchface from the watch’s settings, and shows the currently set watchface to the user. This will fix any issues where if the user installs a non-release version of InfiniTime with the watchface list in a different order, InfiniLink may show the wrong watchface, because of it just being a plain Int.

Regarding CMake, I don’t have much knowledge there, so I can’t provide any feedback either.

JF002 commented 2 months ago

Thanks @mark9064 @liamcharger for your feedback! It's good to know that this change doesn't seem completely over-engineered at first sight! And the point of view of @liamcharger is very interesting, thanks!

I'll then continue this proof of concept when I get the time :-)