-
-
Notifications
You must be signed in to change notification settings - Fork 953
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
User Applications selection using CMake #1928
Conversation
Build size and comparison to main:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! It allows a lot more freedom for building with whatever apps you want. I would say we should add this to the Apps.md documentation before merging, as you now need to register new apps in more places.
Thanks! I've just updated the doc to add instructions related to CMake. |
It seems like InfiniSim will also need to include these same option declarations. @NeroBurner do you know if it would be possible to have InfiniSim get the options from InfiniTime? |
CMakeLists.txt
Outdated
@@ -35,6 +34,19 @@ endif() | |||
set(TARGET_DEVICE "PINETIME" CACHE STRING "Target device") | |||
set_property(CACHE TARGET_DEVICE PROPERTY STRINGS PINETIME MOY-TFK5 MOY-TIN5 MOY-TON5 MOY-UNK) | |||
|
|||
option(ENABLE_APP_STOPWATCH "Enable the Stopwatch application" True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move those options and the Apps.h.in file into its own directory, create a CMake header only library target and link that to everywhere we need it. Because it is in its own folder I can easily include it in InfiniSim afterwards. Like we do for the fonts and images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a look at this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in c200816 and integrated in InfiniSim here : InfiniTimeOrg/InfiniSim#134
CMakeLists.txt
Outdated
if(${enabled}) | ||
list(APPEND ${list} ${type}) | ||
endif () | ||
#return(PROPAGATE ${list}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not enabled, but comment in PR sais it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version of CMake in the docker container is too old for this. See the second commit in this PR.
I don't know much about CMake, but could we put all optional apps in a separate directory and then autogenerate the CMake options?
|
I would imagine that that would be possible, and it would improve the dev experience quite a lot, not having to remember every single place to add a new app 😄 |
One thing I've just thought of is that this doesn't allow choosing the order the apps appear in the app list. I think that that is quite an important feature, so we should try to figure out a way to get that to work. One simple way would just be to have a string CMake parameter that gets placed in UserAppTypes. |
It might be possible, yes. Actually, I would like InfiniTime to support User Applications coming from external repositories. This would allow maintainers of those user apps to maintain their apps the way they want, and to reduce the load on InfiniTime maintainers. I think we can do this in another future PR though :)
Good point! I'll try to find a solution for this! EDIT : Applied @FintasticMan 's suggestion in 00bb624 |
CMakeLists.txt
Outdated
@@ -51,6 +45,12 @@ string(STRIP "${PROJECT_GIT_COMMIT_HASH}" PROJECT_GIT_COMMIT_HASH) | |||
|
|||
message("PROJECT_GIT_COMMIT_HASH_SUCCESS? " ${PROJECT_GIT_COMMIT_HASH_SUCCESS}) | |||
|
|||
if(DEFINED ENABLE_USERAPPS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this variable definition into src/displayapp/apps/CMakeLists.txt. then we'll have one place to update the default apps list and won't need to change the default list in InfiniSim as well.
Otherwise I'd need to update InfiniSim everytime the app list is changed to match InfiniTimes default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, done!
@NeroBurner I'll apply the same change to the watchface selection when you confirm this is ok for you :) |
What about apps that introduce settings pages specific to them? We don't actually have any right now (arguably steps), but some are proposed e.g background heart rate settings |
Handling such dependencies would be a next step. Apps also come with custom BLE interfaces (e.g. music). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small typos, afterwards good to go from my side 🎉
doc/code/Apps.md
Outdated
@@ -159,6 +159,12 @@ If your application is a **user** application, you don't need to add anything in | |||
everything will be automatically generated for you. | |||
The user application will also be automatically be added to the app launcher menu. | |||
|
|||
Since the list of **user** application is generated by CMake, you need to add the variable `ENABLE_USERAPPS` to the command line of CMake. This variable must be set with a string composed of an ordered list of the **user** applications that must be built into the firmware. The items of the list are fields from the enumeration `Apps`. Ex : build the firmware with 3 user application : Alarm, Timer and MyApp (the application will be listed in this specific order in the application menu). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use one line per sentence, makes the diffs in future easier, and "user applications"
Since the list of **user** application is generated by CMake, you need to add the variable `ENABLE_USERAPPS` to the command line of CMake. This variable must be set with a string composed of an ordered list of the **user** applications that must be built into the firmware. The items of the list are fields from the enumeration `Apps`. Ex : build the firmware with 3 user application : Alarm, Timer and MyApp (the application will be listed in this specific order in the application menu). | |
Since the list of **user** applications is generated by CMake, you need to add the variable `ENABLE_USERAPPS` to the command line of CMake. | |
This variable must be set with a string composed of an ordered list of the **user** applications that must be built into the firmware. | |
The items of the list are fields from the enumeration `Apps`. | |
Ex : build the firmware with 3 user applications : Alarm, Timer and MyApp (the applications will be listed in this specific order in the application menu). |
src/displayapp/apps/CMakeLists.txt
Outdated
target_include_directories(infinitime_apps INTERFACE "${CMAKE_CURRENT_BINARY_DIR}/") | ||
|
||
# Generate the list of user apps to be compiled into the firmware | ||
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/Apps.h.in ${CMAKE_CURRENT_BINARY_DIR}/Apps.h) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add newline to end of file
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/Apps.h.in ${CMAKE_CURRENT_BINARY_DIR}/Apps.h) | |
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/Apps.h.in ${CMAKE_CURRENT_BINARY_DIR}/Apps.h) | |
Use CMake's configure_file() functionality to generate the list of User Applications. All the apps included in current versions of InfiniTime are enabled by default, but this can now be overridden by setting variables ENABLE_APP_XXX to True or False. CMake CMP0140 is set to NEW to enable the return PROPAGATE functionality.
Revert changes that need "return PROPAGATE" since this is not available in our Docker build (it needs CMake 3.25 and we have 3.22).
Update documentation about building a new application and add instructions to add the app in CMake files.
Replace the options that allowed to select the user apps independently by a single string variable that contains the ordered list of apps to build.
Move displayapp/Apps.h into a header only library (to make the integration easier in InfiniSim.
Move ENABLE_USERAPPS and USERAPP_TYPES from the root CMake file to src/displayapp/apps/CMakeLists.txt so we do not need to repeat it in InfiniSim
Co-authored-by: NeroBurner <[email protected]>
Fix typos in Apps.md and add new line in src/displayapp/apps/CMakeLists.txt
Fix include path since last rebase.
0fe4ed3
to
f69983e
Compare
Restore the default list of apps to compile. The ordering was changed in the changeset to make the app-list configurable through a CMake-variable in #1928 In the process have one app per line to create the default app list in CMake. This makes git diffs easer and more readable.
Restore the default list of apps to compile. The ordering was changed in the changeset to make the app-list configurable through a CMake-variable in #1928 In the process have one app per line to create the default app list in CMake. This makes git diffs easer and more readable.
Restore the default list of apps to compile. The ordering was changed in the changeset to make the app-list configurable through a CMake-variable in InfiniTimeOrg#1928 In the process have one app per line to create the default app list in CMake. This makes git diffs easer and more readable.
Restore the default list of apps to compile. The ordering was changed in the changeset to make the app-list configurable through a CMake-variable in InfiniTimeOrg#1928 In the process have one app per line to create the default app list in CMake. This makes git diffs easer and more readable.
Use CMake's configure_file() functionality to generate the list of User Applications.
All the apps included in current versions of InfiniTime are enabled by default, but this can now be overridden by setting variables ENABLE_APP_XXX to True or False.
CMake CMP0140 is set to NEW to enable the return PROPAGATE functionality.