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

Introducing compile-time apps modularity #73

Closed
wants to merge 9 commits into from

Conversation

boricj
Copy link
Contributor

@boricj boricj commented Sep 2, 2017

The title says it all. The trick lies in a linked list of App::Snapshot that is built at run-time through constructors, each app containing a statically-declared node along with its Snapshot instance (instead of hard-coding everything inside AppsContainer). With some further trickery and code moving it could lay the foundations of a third-party apps ecosystem for NumWorks.

As a proof-of-concept, this branch enables Computation, Python and Settings apps, disabling all other apps just by commenting them out inside apps/Makefile. Apps to include are specified with the APPS_LIST make variable.

Known issues:

  • Apps order on the home menu is pseudo-random ;
  • The list of apps to use should be stored in an environment variable, not directly inside a Makefile ;
  • Python can't be left out without causing a linkage error ;
  • Will have to move per-app translation strings inside apps ;
  • Will have to move per-app translation strings inside apps once merged ;
  • The technical solution is somewhat clever but dirty, but I can't think of a smarter way to do that (I'm no C++ expert) ;
  • Home app menu list is broken when 3 apps are present ;
  • Tested only under the simulator.

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2017

CLA assistant check
All committers have signed the CLA.

@adriweb
Copy link
Contributor

adriweb commented Sep 4, 2017

Great job, it makes the whole system more "dynamically manageable" - I hope this gets merged eventually :)

@boricj boricj force-pushed the compile_time_apps_modularity branch 3 times, most recently from 8fe8227 to 6d5f77f Compare September 5, 2017 11:18
@boricj
Copy link
Contributor Author

boricj commented Sep 5, 2017

OK, so this should be in good enough shape now for a proper review. I should probably code a third-party app to illustrate how all of this works in practice, but that can wait later.

We could go as far as modularizing the support libraries too (python, poincare) so that we could for example replace Python with Lua without wasting Flash space in the process, but that's probably way overkill for now.

Copy link
Contributor

@Ecco Ecco left a comment

Choose a reason for hiding this comment

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

White I really like the idea of that pull request, I'm not a big fan of the "com.numworks" part. I think this is a bit over-engineered at the moment.

@boricj
Copy link
Contributor Author

boricj commented Sep 5, 2017

That part is to both enforce a globally unique name for all apps (like Android) and have a directory structure that naturally prevents conflicts between apps.

For example, if I create a third-party app for a periodic table hosted at https://github.com/boricj/periodic-table, I would give it the URI com.github.boricj.periodic-table, write in the README to git clone https://github.com/boricj/periodic-table.git apps/com.github.boricj.periodic-table and add com.github.boricj.periodic-table to the list of apps to build.

If adriweb then creates another completely different periodic table app hosted on GitHub too, there's no way either of them will be in conflict at this layer (obviously, they could still butthead on symbol redefinition or something else).

Without that URI scheme, if two people come up with the periodic-table app name then we can't select only one in APPS_LIST without modifying the other's source code.

I agree this may be a bit of over-engineering on my part, but it makes things as plug'n play as it gets.

@adriweb
Copy link
Contributor

adriweb commented Sep 5, 2017

obviously, they could still butthead on symbol redefinition or something else

I'd argue that apps will have to have their own namespace as to avoid this - and I don't think there's a need for any extern "C" functions?

@boricj
Copy link
Contributor Author

boricj commented Sep 5, 2017

Yes, C++ namespaces will be needed to prevent apps from stepping on each other's toes, but it remains to be seen how this will unfold. After all, homebrew apps integrated to the firmware at compile-time is not the usual way things are done in the calculator world. But even if the firmware architecture changes towards run-time apps down the road where such issues won't happen, it'll be some time before that happens.

The last technical barrier is translation. Apps can't just pretend that they don't exist since the app Descriptor class returns identifiers to be translated and those are currently centralized. Obviously we can't embed translations for every third-party app ever in-tree, so I need to come up with a cunning plan that won't end up in rebasing disaster territory.

@boricj boricj force-pushed the compile_time_apps_modularity branch 2 times, most recently from 2b87b55 to 45bbd88 Compare September 7, 2017 10:21
@boricj
Copy link
Contributor Author

boricj commented Sep 7, 2017

Alright, so translation reworking is done and it should be able to handle out-of-tree apps now. After this is merged, app-specific translations should be moved to their respective app directories in order not to bundle them if the relevant app is not selected, but I'd rather keep rebasing of this branch as painless as possible.

@adriweb
Copy link
Contributor

adriweb commented Sep 7, 2017

Nice!
Regarding the app GUID, I suppose getting rid of the "TLD" dev prefix (com. for instance) would be feasible. I'll assume that 'numworks', 'boricj' and 'adriweb' for example, will be unique enough. After all, as an idea, it could just be tied to the github username, and it'd just work fine. This also may address @Ecco's point that it looked a bit too heavy/over-engineered - just having the dev name before the app name is okay IMHO.
It wouldn't look like iOS/Android names anymore, but do we really care?

@boricj boricj force-pushed the compile_time_apps_modularity branch from 45bbd88 to 43236b0 Compare September 7, 2017 12:06
@boricj
Copy link
Contributor Author

boricj commented Sep 7, 2017

I dropped the com. prefix from the app GUID.

Note that this will not compile. The rest of the source code is adapted
to the new translation scheme in another commit to ease rebasing.
To regenerate this commit, use the following Shell snippet:

for i in $(seq 1 20); do
  for i in '*.cpp' '*.h'; do
    find . -name $i | xargs sed -i 's|I18n::Message::|\&I18n::Common::|'
    find . -name $i | xargs sed -i 's|(I18n::Message)0|\&I18n::NullMessage|'
    find . -name $i | xargs sed -i 's|I18n::Message |const I18n::Message*|'
  done
done

for i in $(seq 1 20); do
  for i in '*.cpp' '*.h'; do
    find . -name $i | xargs sed -i 's|const I18n::Message\*|const I18n::Message \*|'
  done
done
@boricj boricj force-pushed the compile_time_apps_modularity branch from 43236b0 to ae1a4dd Compare September 7, 2017 13:25
@boricj
Copy link
Contributor Author

boricj commented Sep 7, 2017

All right, here's the first ever third-party app for NumWorks: https://github.com/boricj/numworks-hello-world. Not much to look at, but it proves this pull request actually works as intended.

One issue to note is that the home view goes crazy if all nine apps are built. It's probably a good idea to modify it so that it scrolls up/down instead of left/right.

@boricj boricj changed the title RFC: introducing compile-time apps modularity Introducing compile-time apps modularity Sep 7, 2017
@boricj boricj force-pushed the compile_time_apps_modularity branch 2 times, most recently from 551f3a6 to f700a6b Compare September 9, 2017 10:31
@boricj boricj force-pushed the compile_time_apps_modularity branch from f700a6b to 0a9a787 Compare September 9, 2017 10:36
@boricj
Copy link
Contributor Author

boricj commented Sep 9, 2017

The Python app can now be left out as well. MicroPython is still built and linked, but we have some time ahead of us before introducing library compile-time modularity.

With all outstanding issues out of the way, this pull request is now ready to be a candidate for merging. There are a couple of glitches with the home screen when there is a certain number of apps, but that's a separate problem.

@boricj boricj mentioned this pull request Sep 11, 2017
@boricj
Copy link
Contributor Author

boricj commented Sep 11, 2017

After chatting a bit on IRC, it became apparent that this pull request is a bit on the heavy side. @Ecco would you prefer for me to break it down into smaller pull requests, one at a time?

Also, if somebody could confirm this works on the hardware that would be great.

@Ecco
Copy link
Contributor

Ecco commented Sep 11, 2017

Hi @boricj ! First of all, a big thank you, both for your work and your positive mindset. The current PR is indeed a bit big, but in my opinion that's not the biggest issue. Having smaller PRs would indeed make the review/merge cycle faster though.

If you don't mind, here are the issues I have with this PR:

  • I'm not a big fan of moving Epsilon-provided apps in a subfolder. Those are first-party apps, and I think they deserve to live directly in the "apps" directory. It makes the whole tree smaller, and I find it more welcoming for newcomers. I understand the need to prevent collisions though, but I think this is something we should deal with later on.
  • This PR deals with i18n. It is a good idea, but I think this is also something we should deal with later on.
  • Last but not least, this adds a runtime behavior that was previously done at compile time. This is something we try to avoid as much as possible (and here it's clearly avoidable).

I think the first thing we should focus on is being able to select which Epsilon apps are included in a build. We'll then add on this to make i18n more flexible and allow for third-party apps. To achieve this, I think the best approach would be as follow:

  1. Use a compile-time, Makefile-based app selection and ordering. For example, we could have an apps/apps.mak file that would, in order, define all apps that we want on the homescreen. This file would contains lines such as "include apps/calculation/Makefile".
  2. Based off of that, each app's Makefile would add new objects to be built (like they currently do), but also append the app's App subclass (e.g. Calculation::App) to a given Make variable.
  3. That variable would be forwarded to the compiler when building apps_container.cpp, which would allow for a compile-time generation of the list of Snapshot instance variables.

@boricj
Copy link
Contributor Author

boricj commented Sep 12, 2017

OK then. Considering that:

  • this pull request has to be split into at least two (compile-time modularity, translation decentralization),
  • the changes would break my third-party app example,
  • the changes are not trivial,

it's probably best to close this pull request, keep the branch around as a proof-of-concept, and open new, clean pull requests one at a time for each topic.

I'm mostly worried about translation at this point, because reworking it is a quite involved task and I don't really have a good technical solution at hand (my hack is really more of a superficial workaround than a real solution at this point). #71 is probably a good place to discuss that topic further.

@NetMage
Copy link

NetMage commented Feb 2, 2019

Having to compile a new firmware to add apps makes this an approach for hobbyists and not a serious calculator for end users.

@boricj
Copy link
Contributor Author

boricj commented Feb 3, 2019

Well then, what do you suggest?

Keep in mind that there's not a lot of Flash to spare, epsilon is both fairly tightly integrated and a quickly moving target and the NumWorks team doesn't want to get burdened maintaining extra fluff without a good reason, even more so if it means providing backwards compatibility.

I do believe apps written in (Micro)Python loaded at runtime without compilation/linking to be an achievable goal, unlike the same for native third-party apps, but I have a finite amount of free time to spend.

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

5 participants