Compile-time APP_DATA_PATH() and APP_ASSETS_PATH()

[WillyJL]

What's new

• APP_DATA_PATH() and APP_ASSETS_PATH() use appid from compile time instead of runtime thread_id
• Fixes incorrect paths when these app storage macros are used in timer / service context (eg. timer/draw callback)
• Less runtime overhead due to less string replacing to generate correct paths
• Code not compiled as FAP will fail since FAP_APPID is not defined
• No user intervention required, this is drop in fix, only recompile is needed
• To function, adds C define FAP_APPID, could be useful to app makers aswell

Verification

• Open app that uses APP_DATA_PATH() or APP_ASSETS_PATH() in draw/timer callback
• Path is still correct /ext/apps_.../appid, instead of ending as /ext/apps_.../system or /ext/apps_.../gui

Checklist (For Reviewer)

• [ ] PR has description of feature/bug or link to Confluence/Jira task
• [ ] Description contains actions to verify feature/bugfix
• [ ] I've built this code, uploaded it to the device and verified feature/bugfix

[WillyJL]

Just occurred to me that this would need an equivalent in ufbt, I don't use that myself so I forgot... I can make a PR there as well if we go through with this PR

[hedger]

Thanks for the PR. Overall, it's well-implemented. However, it has a certain side-effect.

Currently, if a .fap file is renamed or copied on the device, it gets a separate app data folder, according to its new file name. That way, you can create multiple isolated environments for a single app. However, there are no apps that would make use of that at the moment - at least to my knowledge. With current implementation from this PR, app is confined to its appid. Surely, that approach has its own benefits and enables a predictable file location.

This PR also introduces a system-wide define with "system" name at a fairly low level - maybe its scope could also be reduced to app loader level. I'll look into that and see whether that can be adjusted.

[WillyJL]

Agreed on the define in furi.h, the naming was rushed just to get something working lol. As you say yeah maybe Loader or Storage services could handle this. Before making this PR I had originally fully removed furi_thread_get_appid() since it wasn't really used elsewhere, so that's part of why I put it in furi.h, but then chose against removing it so here we are.

As for renaming .fap files, you make a very good point... at the very least then the behavior should be consistent for APP_ASSETS_PATH(). Currently, it extracts to folder based on fap filename, and then the app looks based on compiled appid... easiest solution I see would be making the assets extract to folder based on fap header appid, so after rename it is same folder path. The other option would be finding a way for loader to tell the app what it's appid is based on fap filename, but then we are back to the issue of threads, and it becomes overcomplicated i think.

[WillyJL]

easiest solution I see would be making the assets extract to folder based on fap header appid, so after rename it is same folder path

i was looking around to implement this but realized that fap header has app name but not appid... not sure what the best idea would be then...

[skotopes]

@Willy-JL Some proxy object that is initialized at the app start and got it name and then all your internal references goes to it. That is not something that should be on firmware side, but something that lives in app bss

[WillyJL]

so then APP_ASSETS_PATH() and APP_DATA_PATH() will remain half broken? and apps will need to make their own system?

or do you mean an alternative way of doing it that is not drop-in, but replaces the existing macros?

[WillyJL]

ah i think i get what you mean, i was unfamiliar with the concept of .bss... so then, at app launch, loader would initialize this global static variable that is located in bss with the app's id, and the app code references to it?

sounds like then we could provide helper macros that construct a path using this variable, but still not handled by firmware. like some sort of printf in app code that uses this appid variable, but inside a macro so app developers dontn eed to worry about the variable itself?

would mean this is not strictly drop-in, but close enough, same idea but using a printf or similar under the hood...

[skotopes]

yep

[WillyJL]

im new to those concepts but ill try to figure something out, if you have some examples i could look at that would be greatly appreciated!

also i drafted for now, but ill perhaps close this and open a new pr when i have something working

[WillyJL]

@skotopes i was looking into how to implement this with bss, and had a counter argument.

i feel like the behavior of renaming a fap file changing storage location as @hedger described is quite weird. yes it has a very niche use case, but i think it instead leads to a lot more confusion for people messing around with faps installed manually, than the small advantages it provides.

instead, i was considering that maybe a better solution would be sticking with what i have in this PR thus far (save for moving where the define is made), so paths are defined at compile time (and backwards compatible with existing code), and instead add a new elf section (or attribute in fap header) for the appid, so that assets extractor doesnt need to rely on fap name and can instead use the same appid that was used at compile time to hardcode the paths.

this would fix the disjunction between runtime and compile time paths, as well as as avoid unnecessarily complex logic to store the appid in bss and construct paths on app side at runtime with printf. as well as be backwards compatible, requiring one simple recompile.

[CookiePLMonster]

FWIW from the user's point of view this PR is worth it even for this

Fixes incorrect paths when these app storage macros are used in timer / service context (eg. timer/draw callback)

point alone.

[skotopes]

Put implementation aside for a moment.

Do you guys feel like you want to have ability to have multiple app versions with isolated spaces or not?

[WillyJL]

While I see the value in that option in some niche use cases, to the average user it can be more of a pain when the app doesn't work as intended when they renamed a file. Say the download a fap, and they download it again on a newer version, but they didn't clean the downloads folder. Now it's appid (1).fap. Suddenly, the app lost all its configuration. The user doesn't even have the slightest indication why, since the app itself works, just the configuration is lost.

also, there's the point that compile time macros are easier to use. No need for furi strings or printf to insert appid at runtime, easier to use, and not a breaking change compared to current api.

Personally, I'm 100% for the option of compile time, appid is hardcoded in fap, not chosen by filename.

[WillyJL]

Also, I recently discovered there is compile time macro "FAP_VERSION". It only makes logical sense to offer "FAP_APPID". I wasn't aware at the time of making this PR, hence the "system" and "FURI_APPID". "FAP_APPID" would work great and be super intuitive, and "APP_DATA_PATH()" can rely on FAP_APPID then?

[skotopes]

Ok, we'll discuss it with @hedger and @DrZlo13 today and then decide

[WillyJL]

i feel like this is satisfactory and achieves what we discussed. however, there are 2 parts i am not sure what to do about:

• unit tests test_storage_data_path_apps and test_storage_data_path are not relevant anymore, since its not based on thread appid anymore. rather, only thing i can think to test is that accessing /ext/apps_data/any_appid will create this folder correctly.
• since apps use hardcoded assets paths now, they need to be extracted to hardcoded path too, not based on filename. fap header contains app name, not appid. first solution i can think of is adding appid to fap header, and bumping api version (but also check header version in case of same api version).

i will implement those solutions in the meantime waiting for comments

[WillyJL]

tested with ufbt and apps that use both assets and data paths, seems to be working well to me :D

[WillyJL]

Yep, as I mentioned above those tests aren't really relevant anymore. Should I just remove them?

[hedger]

Yes - if they are obsolete.

[WillyJL]

@hedger done )

[WillyJL]

@hedger unit tests are failing in CI setup step, not in unit test itself it seems. when uploading files for apps_data/nfc, complains that nfc folder already exists, which makes sense because now all literal accesses to /ext/apps_data/anything will create such folder. i guess, update CI to accept "already exist" as success, instead of fail?

[skotopes]

@hedger @Willy-JL @DrZlo13 PR with int storage drop was merged, so we ready to merge this one. Please finalize and let me know when we can merge it