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

User Applications selection using CMake #1928

Merged
merged 9 commits into from
Dec 23, 2023
Merged

Conversation

JF002
Copy link
Collaborator

@JF002 JF002 commented Dec 17, 2023

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.

Copy link

github-actions bot commented Dec 17, 2023

Build size and comparison to main:

Section Size Difference
text 368984B -16B
data 940B 0B
bss 63516B 0B

Copy link
Member

@FintasticMan FintasticMan left a 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.

@JF002
Copy link
Collaborator Author

JF002 commented Dec 17, 2023

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.

@FintasticMan
Copy link
Member

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

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

Copy link
Collaborator Author

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!

Copy link
Collaborator Author

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

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

Copy link
Member

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.

@minacode
Copy link
Contributor

minacode commented Dec 18, 2023

I don't know much about CMake, but could we put all optional apps in a separate directory and then autogenerate the CMake options?

.../displayapp/
    apps/
        music/
            music.cpp
            music.h
        twos/
            twos.cpp
            twos.h
        ...

@FintasticMan
Copy link
Member

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 😄

@FintasticMan
Copy link
Member

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.

@JF002
Copy link
Collaborator Author

JF002 commented Dec 19, 2023

@minacode

I don't know much about CMake, but could we put all optional apps in a separate directory and then autogenerate the CMake options?

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

@FintasticMan

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.

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done!

@JF002
Copy link
Collaborator Author

JF002 commented Dec 21, 2023

@NeroBurner I'll apply the same change to the watchface selection when you confirm this is ok for you :)

@mark9064
Copy link
Member

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

@minacode
Copy link
Contributor

Handling such dependencies would be a next step. Apps also come with custom BLE interfaces (e.g. music).

@JF002
Copy link
Collaborator Author

JF002 commented Dec 23, 2023

@mark9064 That's a good question! And I agree with @minacode, we can figure that out in a next step. In this current state, the worse that can happen is that settings will show a setting that no userapp will actually use, but the whole firmware will still be functional.

Copy link
Contributor

@NeroBurner NeroBurner left a 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 Show resolved Hide resolved
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).
Copy link
Contributor

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"

Suggested change
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).

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

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

Suggested change
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)

JF002 and others added 9 commits December 23, 2023 21:15
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
Fix typos in Apps.md and add new line in src/displayapp/apps/CMakeLists.txt
Fix include path since last rebase.
@JF002 JF002 force-pushed the cmake-userapps-selection branch from 0fe4ed3 to f69983e Compare December 23, 2023 20:24
@JF002 JF002 added this to the 1.14.0 milestone Dec 23, 2023
@JF002 JF002 merged commit fc5424c into main Dec 23, 2023
6 of 7 checks passed
@JF002 JF002 deleted the cmake-userapps-selection branch December 23, 2023 20:29
NeroBurner added a commit that referenced this pull request Jan 6, 2024
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.
NeroBurner added a commit that referenced this pull request Jan 6, 2024
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.
mark9064 pushed a commit to mark9064/InfiniTime that referenced this pull request Jan 25, 2024
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.
lasnikr pushed a commit to lasnikr/LasnikrInfiniTime that referenced this pull request Mar 5, 2024
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.
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.

5 participants