GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — User Applications selection using CMake by JF002 · Pull Request #1928 · InfiniTimeOrg/InfiniTime

Conversation

@JF002

@JF002 JF002 commented Dec 17, 2023

Copy link
Copy Markdown
Collaborator

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@github-actions

github-actions Bot commented Dec 17, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Build size and comparison to main:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FintasticMan
FintasticMan requested changes Dec 17, 2023
View reviewed changes

@FintasticMan FintasticMan left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@JF002

JF002 commented Dec 17, 2023

Copy link
Copy Markdown
Collaborator Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FintasticMan
FintasticMan approved these changes Dec 17, 2023
View reviewed changes
@FintasticMan

FintasticMan commented Dec 17, 2023

Copy link
Copy Markdown
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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

NeroBurner
NeroBurner reviewed Dec 17, 2023
View reviewed changes
Comment thread CMakeLists.txt Outdated
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)

@NeroBurner NeroBurner Dec 17, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FintasticMan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@JF002 JF002 Dec 19, 2023

Copy link
Copy Markdown
Collaborator Author

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@JF002 JF002 Dec 19, 2023

Copy link
Copy Markdown
Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
NeroBurner
NeroBurner reviewed Dec 17, 2023
View reviewed changes
Comment thread CMakeLists.txt Outdated
if(${enabled})
list(APPEND ${list} ${type})
endif ()
#return(PROPAGATE ${list})

@NeroBurner NeroBurner Dec 17, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@FintasticMan FintasticMan Dec 17, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@minacode

minacode commented Dec 18, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan

FintasticMan commented Dec 18, 2023

Copy link
Copy Markdown
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 😄

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan

FintasticMan commented Dec 19, 2023

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JF002

JF002 commented Dec 19, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

@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

minacode and FintasticMan reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FintasticMan
FintasticMan approved these changes Dec 19, 2023
View reviewed changes
NeroBurner
NeroBurner reviewed Dec 19, 2023
View reviewed changes
Comment thread CMakeLists.txt Outdated

message("PROJECT_GIT_COMMIT_HASH_SUCCESS? " ${PROJECT_GIT_COMMIT_HASH_SUCCESS})

if(DEFINED ENABLE_USERAPPS)

@NeroBurner NeroBurner Dec 19, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FintasticMan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@JF002 JF002 Dec 21, 2023

Copy link
Copy Markdown
Collaborator Author

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@JF002

JF002 commented Dec 21, 2023

Copy link
Copy Markdown
Collaborator Author

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

NeroBurner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Dec 22, 2023

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@minacode

minacode commented Dec 22, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JF002

JF002 commented Dec 23, 2023

Copy link
Copy Markdown
Collaborator Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

NeroBurner
NeroBurner approved these changes Dec 23, 2023
View reviewed changes

@NeroBurner NeroBurner left a comment

Copy link
Copy Markdown
Contributor

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 🎉

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread doc/code/Apps.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread doc/code/Apps.md Outdated
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).

@NeroBurner NeroBurner Dec 23, 2023

Copy link
Copy Markdown
Contributor

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"

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread 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) No newline at end of file

@NeroBurner NeroBurner Dec 23, 2023

Copy link
Copy Markdown
Contributor

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
JF002 and others added 9 commits December 23, 2023 21:15
@JF002
CMake User Applications selection
f5736b6
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.
@JF002
CMake User Applications selection
96727c5
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).
@JF002
CMake User Applications selection
20c9aad
Update documentation about building a new application and add instructions to add the app in CMake files.
@JF002
Watch face selection at build time
7ed65d3
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.
@JF002
Watch face selection with CMake
a4577f3
Move displayapp/Apps.h into a header only library (to make the integration easier in InfiniSim.
@JF002
CMake user application selection
009c1d2
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
@JF002 @NeroBurner
Fix trailing space in doc/code/Apps.md
996c045
Co-authored-by: NeroBurner <[email protected]>
@JF002
User applications selection using CMake
5aae9fe
Fix typos in Apps.md and add new line in src/displayapp/apps/CMakeLists.txt
@JF002
User applications selection using CMake
f69983e
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
@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
@NeroBurner
apps: restore default apps ordering in CMake
eaca73c
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 NeroBurner mentioned this pull request Jan 6, 2024
apps: restore default apps ordering in CMake #1952
Merged
NeroBurner added a commit that referenced this pull request Jan 6, 2024
@NeroBurner
apps: restore default apps ordering in CMake
12acef6
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
@NeroBurner @mark9064
apps: restore default apps ordering in CMake
3f58eb8
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.
user84918342 pushed a commit to user84918342/LasnikrInfiniTime that referenced this pull request Mar 5, 2024
@NeroBurner @user84918342
apps: restore default apps ordering in CMake
80be235
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.
@JF002 JF002 mentioned this pull request Mar 19, 2024
[WIP/RFC] Remove watchface enum #2040
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NeroBurner NeroBurner NeroBurner approved these changes
@FintasticMan FintasticMan FintasticMan approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.14.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@JF002 @FintasticMan @minacode @mark9064 @NeroBurner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.