App storage functionality simplification #865
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## API_LEVEL_22 #865 +/- ##
===============================================
Coverage ? 83.02%
===============================================
Files ? 9
Lines ? 483
Branches ? 0
===============================================
Hits ? 401
Misses ? 82
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
iartemov-ledger
force-pushed
the
app_storage_simple
branch
from
0302e1f to
5a0ad66
Compare
include/app_storage.h
Outdated
| /* Setters */ | ||
| int32_t app_storage_pwrite(const void *buf, uint32_t nbyte, uint32_t offset); | ||
| void app_storage_set_data_version(uint32_t data_version); | ||
| void app_storage_increment_data_version(void); |
There was a problem hiding this comment.
@jarevalo-ledger >> Do we need app_storage_erase() to be able to dynamically reduce app storage size ?
@iartemov-ledger >> To note that from SDK PoV we'll be able to play only if the part at the end of storage.
There was a problem hiding this comment.
FYI Neither app-boilerplate nor app-security-key remove some storage data today (in app-security-key there is an erase, but just only to add another data to the very same slot).
| /* NVM data (ex-filesystem) */ | ||
| /* TODO: should we align the start of N_ variables on page size ? */ |
There was a problem hiding this comment.
There was a problem hiding this comment.
Current version supports 2 approaches:
- NVM accessible through
app_storageAPI, backupable and restorable - NVM accessible through
N_variable (legacy approach), NOT backupable and restorable
I do not see anything bad to support the both it gives the possibility to user to backuping/restoring some data. And it will work for apps that have not yet migrated to the new API (we'll need tests for all this).
| @@ -69,7 +69,11 @@ SECTIONS | |||
|
|
|||
| _nvram_data = .; | |||
|
|
|||
| /* App storage */ | |||
| *(.storage_section) | |||
There was a problem hiding this comment.
@jarevalo-ledger >> to add preprocessor checking for HAVE_APP_STORAGE.
There was a problem hiding this comment.
We do not run processor against the .ld files currently.
If no variable is declared in storage_section it will not occupy anything.
iartemov-ledger
force-pushed
the
app_storage_simple
branch
from
4ca4e02 to
90233cd
Compare
iartemov-ledger
force-pushed
the
app_storage_simple
branch
from
90233cd to
6654e6b
Compare
iartemov-ledger
requested review from
abonnaudet-ledger,
lpascal-ledger and
fbeutin-ledger
master.Corresponds to
app-boilerplatechange: LedgerHQ/app-boilerplate#147Corresponds to
app-security-keychange: LedgerHQ/app-security-key#69Description
HAVE_APP_STORAGE,APP_STORAGE_SIZE,HAVE_APP_STORAGE_PROP_SETTINGSandHAVE_APP_STORAGE_PROP_DATAhave to be define in app's Makefilestruct_versionis the header version (the app data versioning has to be done explicitly in the app)sizerepresents the app data size, starts with 0 and grows with writesTODOsChanges include
Breaking changes
See above ⬆️