Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[WIP/RFC] Remove watchface enum #2040

wants to merge 4 commits into from

Conversation

JF002
Copy link
Collaborator

@JF002 JF002 commented Mar 19, 2024

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?

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.
Copy link

Build size and comparison to main:

Section Size Difference
text 377736B 224B
data 940B 0B
bss 63556B 16B

@mark9064
Copy link
Contributor

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
Copy link
Contributor

liamcharger commented Mar 28, 2024

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
Copy link
Collaborator Author

JF002 commented Apr 7, 2024

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 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants