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

App controller #1840

Closed
wants to merge 5 commits into from
Closed

Conversation

minacode
Copy link
Contributor

@minacode minacode commented Aug 22, 2023

This PR stems from #1571 and is related to #1262.
It is a draft, because there is work left to do (see the open problems below).

This PR implements an AppController class as a single place in the code where all non-core apps are registered.
Short-term this allows us to implement optional apps (see #1408).
Long-term this allows us to implement dynamically loadable apps.

Currently the app Twos is handled by the AppController as an example, mostly because it has no dependencies and is therefore simple to handle.

The implementation uses the Apps enum (see also #1760), but removes all entries of apps that will be handled by the AppController. Instead, the entry Dynamic is added to the enum.
The ids of dynamic apps are then calculated by Dynamic + <offset> where <offset> is the id of an app inside the AppController (e.g. an array index). This leads to a lot of static_cast everywhere.

All places, where the current apps where hardcoded before have to use the AppController now.

Apps that can be used with the AppController must implement a Get method and its symbol like this:

...
public:
  static std::unique_ptr<Twos> Get() {
    return std::make_unique<Twos>();
  }

  static constexpr const char* Symbol {"2"};

The AppController saves those contructors internally and can reference them via the Dynamic + <offset> ids described above.

There are also already fragments of an AppInterface in the code that every Get method must receive. The AppInterface bundles all the controllers that apps can require.

The current version runs with InfiniSim if you use my branch.

Feedback and ideas would be very apprechiated 😊

Open Problems:
  • The AppController should be a component and live in the respective directory.
  • How are we handling the core apps? Are we registering them in the AppController as well or are we hardcoding them (for now)?
  • Implement the AppInterface.
  • Move all/more apps to the AppController.
  • Currently, there are a lot of std::vector in the code. Is it better to swap them for std::array? This is a tradeoff between dynamically allocating a collection when we know how many apps we have vs. hardcoding an array with reasonable bounds. Using only vectors seems to overflow the stack.

@github-actions
Copy link

Build checks have not completed. Possible reasons for this are:

  1. The checks need to be approved by a maintainer
  2. The branch has conflicts
  3. The firmware build has failed

@FintasticMan FintasticMan added the enhancement Enhancement to an existing app/feature label Aug 27, 2023
@JF002
Copy link
Collaborator

JF002 commented Dec 10, 2023

@minacode Thanks for your work on this topic! Since you created this PR, we merged #1894 which, I think, does mostly the same functionality at build time. Is this OK for you if we close this PR in favor of #1894 ?

@minacode
Copy link
Contributor Author

Yes, absolutely :)

@minacode minacode closed this Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing app/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants