-
-
Notifications
You must be signed in to change notification settings - Fork 954
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
[WIP/RFC] Remove watchface enum #2040
base: main
Are you sure you want to change the base?
Conversation
… and WatchFaces.h
Watch faces are not identified by their type at build type and their name at runtime
Generate the includes necessary for the watch face by CMake.
Build checks have not completed. Possible reasons for this are:
|
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! |
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. |
Thanks @mark9064 @liamcharger for your feedback! It's good to know that this change doesn't seem completely over-engineered at first sight! I'll then continue this proof of concept when I get the time :-) |
Move all watch faces in their own folder, along with a CMake file that specify the name of the watch face, its namespace, the include file and sources files.
I've just pushed new changes : all watch faces are now in their own folder along with a Cmake file that specify the name, namespace, include and sources files. Since the separation between watch faces code from the rest of the code is now clearer, those change also allow to more easily host the source code of the watch face in external repo and include them in the project (or a fork) using git submodules, for example. Feel free to provide feedback about those changes :) |
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 enumsWatchFace
andApps
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 gitsubmodule
s 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
andApps
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?