-
Notifications
You must be signed in to change notification settings - Fork 2.1k
pkg/flashdb: bump version and improve test #21900
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
base: master
Are you sure you want to change the base?
pkg/flashdb: bump version and improve test #21900
Conversation
0973db1 to
095bfc3
Compare
|
squashed fixups |
leandrolanzieri
left a comment
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.
Nice! Thanks for this.
Do you mind applying the static check suggestions now that we're touching the file anyway? I'll run this on hardware tomorrow.
tests/pkg/flashdb_vfs/main.c
Outdated
| /* create database directory */ | ||
| vfs_mkdir(FDB_DIR "/fdb_tsdb1", 0777); | ||
| int res = vfs_mkdir(TSDB_DIR, 0777); | ||
| printf("mkdir '%s' %s", TSDB_DIR, resstr(res == 0 || res == -EEXIST)); |
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.
Any particular reason to return if there's an error while creating a directory for the KVDB and not here?
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 catch!, I just forgot to also add it here. Fixed.
The stacksize was not large enough to run this on an embedded target with flash based mtd and littlefs2. The test now checks return values for expected values and reports issues to the user.
Without this change, some changes to the persistent storage (e.g., with littlefs2 on mtd_spi_nor) are not actually written back to the 'disk'. The boot_count in the example was therefore not persistent across reboots. Deinitializing solves this by properly closing all open files.
This bumps the package version to the current master as the most recent release has a bug in the LIBC-flavour of the file-based storage backend. Without this fix the file-based sotrage is therefore not usable on any target other than native.
095bfc3 to
f85002b
Compare
| { /* GET the KV value */ | ||
| /* get the "boot_count" KV value */ | ||
| fdb_kv_get_blob(kvdb, "boot_count", fdb_blob_make(&blob, &boot_count, sizeof(boot_count))); | ||
| /* the blob.saved.len is more than 0 when get the value successful */ |
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 was under the impression most (all?) the tests were taken from their docs: https://armink.github.io/FlashDB/#/sample-kvdb-basic
I haven't done so yet, but maybe you could double-check if they have changed in the meantime. Nevertheless, it definitely makes sense to catch return values.
Will try to have a closer look at it today or beginning of next week.
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.
Right, makes sense to check that. And yes, the tests look like a straight copy of the package examples. Maybe it makes sense to also upstream that later. The FlashDB repo has no changes compared to the version currently used in RIOT (git diff --name-only b1043ed10defdeec6f97c4099cb212a777a727fe 2.1.1 tests samples in the FlashDB repo lists nothing). As I pointed out in the PR description in the long run it would probably be better to replace our tests with the the package tests instead of using the examples for testing. But the changes of this PR already grew bigger than I anticipated for a quick version bump ;)
mguetschow
left a comment
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.
Nice! Smaller nit's below. Now that we have the [OK] values, it wouldn't be to hard to add a Python test for it (maybe in a follow-up PR, though) :)
| /* converts a test condition to a human readable string */ | ||
| const char *resstr(bool condition) | ||
| { | ||
| return condition ? "[OK]" : "[FAILED]"; | ||
| } |
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.
why no static inline function in a header file that can be included in the c files instead of explicitly declaring extern resstr everywhere?
| /* change the "boot_count" KV's value */ | ||
| fdb_kv_set_blob(kvdb, "boot_count", fdb_blob_make(&blob, &boot_count, sizeof(boot_count))); | ||
| printf("set the 'boot_count' value to %d\n", boot_count); | ||
| int res = fdb_kv_set_blob(kvdb, "boot_count", |
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.
| int res = fdb_kv_set_blob(kvdb, "boot_count", | |
| fdb_err_t res = fdb_kv_set_blob(kvdb, "boot_count", |
is the correct return type
| { /* GET the KV value */ | ||
| int expected = boot_count; | ||
| /* get the "boot_count" KV value */ | ||
| int res = fdb_kv_get_blob(kvdb, "boot_count", |
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.
| int res = fdb_kv_get_blob(kvdb, "boot_count", | |
| size_t res = fdb_kv_get_blob(kvdb, "boot_count", |
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.
(could also be its own function as this is basically duplicated from above)
| USEMODULE += vfs_auto_format | ||
|
|
||
| CFLAGS += -DTHREAD_STACKSIZE_MAIN=THREAD_STACKSIZE_LARGE | ||
| CFLAGS += -DTHREAD_STACKSIZE_MAIN=THREAD_STACKSIZE_LARGE*2 |
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.
why is this a new requirement?
Contribution description
Bumps the FlashDB package to current master and improves the test a bit.
The current version in RIOT has a bug in the file-based storage implementation. The bug breaks the libc based file access and therefore breaks the file-based mode on all boards except native, which uses the posix mode file access.
During testing I noticed the test apps are not easy to interpret on the first look. They also don't actually work on real hardware. The test in master swallows most errors silently and in most cases doesn't check return codes. Some values were not persisted across reboots (e.g., the boot_counter) as they got lost somewhere in the cache.
I added some output to the test that makes it easier to see if something went wrong. Explicitly calling the de-init functions of FlashDB in the test application fixes the issue with the broken persistence.
Testing procedure
To test on a board you should make sure the persistent storage is clean, for example:
make BOARD=adafruit-feather-nrf52840-sense -C tests/sys/vfs_default all flash termThen format through the RIOT shell:
Run the flashdb_vfs test:
USEMODULE=test_utils_interactive_sync make BOARD=adafruit-feather-nrf52840-sense -C tests/pkg/flashdb_vfs all flash termsmash that reset button
Same for
tests/pkg/flashdb_mtd. Both applications should also work on native (for me they did).Note that the failure with the time series append operations happens due to the dummy timer implementation of the test. That also happened before but wasn't visible.
Issues/PRs references
The package repo actually some more tests. We should probably integrate those in the future instead of the simplified examples we currently have as our tests.