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

pkg/flashdb: enhance FAL config #20221

Merged
merged 6 commits into from
Feb 11, 2024

Conversation

fabian18
Copy link
Contributor

@fabian18 fabian18 commented Jan 2, 2024

Contribution description

I wanted to split out the FlashDB fal_cfg partition offset fix from #19557.
This also adds a short test to see if all partitions initialize fine and it adds a configuration option for the sector size.
The test very much reminds of an auto_init module for FLashDB.

Testing procedure

Test output of BOARD=same54-xpro make -C tests/pkg/flashdb_fal_cfg flash term

2024-01-03 00:09:14,858 # main(): This is RIOT! (Version: 2024.01-devel-475-g2de30-pr/flashdb_cfg_enhancement)
2024-01-03 00:09:14,862 # Initializing FlashDB KVDB partition part0
2024-01-03 00:09:14,869 # [I/FAL] Flash Abstraction Layer (V0.5.99) initialize success.
2024-01-03 00:09:14,879 # Initializing FlashDB KVDB partition part1
2024-01-03 00:09:14,883 # Initializing FlashDB TSDB partition part2
2024-01-03 00:09:14,887 # Initializing FlashDB TSDB partition part3
2024-01-03 00:09:14,887 # SUCCESS

Issues/PRs references

@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: pkg Area: External package ports Area: Kconfig Area: Kconfig integration labels Jan 2, 2024
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 3, 2024
Comment on lines 10 to 16
ifeq (,$(filter periph_rtc,$(FEATURES_PROVIDED)))
FEATURES_REQUIRED += periph_rtt
USEMODULE += rtt_rtc
USEMODULE += ztimer_no_periph_rtt
else
FEATURES_REQUIRED += periph_rtc
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work unfortunately.
You don't have the FEATURES_PROVIDED information at this point, so you will always select periph_rtt

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the known issue with the make dependency resolution... Maybe DEFAULT_MODULE can somehow be used but that only gets resolved on the last iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not figure out how to resolve this with DEFAULT_MODULE but FEATURES_REQUIRED_ANY seems to do a good job here.

When I compile with BOARD=same54-xpro make it compiles fine.
And when I remove FEATURES_PROVIDED += periph_rtc from boards/same54-xpro/Makefile.featuresit also compiles fine with "Using real-time-timer RTC".

@riot-ci
Copy link

riot-ci commented Jan 3, 2024

Murdock results

✔️ PASSED

bf4bfdf tests/pkg/flashdb_fal_cfg: add Makefile.ci

Success Failures Total Runtime
10011 0 10011 10m:33s

Artifacts

@fabian18 fabian18 changed the title Pr/flashdb cfg enhancement pkg/flashdb: enhance FAL config Jan 5, 2024
$(info Using real-time-timer RTC)
USEMODULE += rtt_rtc
USEMODULE += ztimer_no_periph_rtt
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work? I would expect that doing this after the dependency resolution won't have any effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don´t know the magic behind it but I assume it works. When I remove FEATURES_PROVIDED += periph_rtc from boards/same54-xpro/Makefile.features and do an info-modules I see that rtt_rtc and ztimer_no_periph_rtt are in. When I try to build after I remove USEMODULE += rtt_rtc linking fails, as expected.

Copy link
Contributor Author

@fabian18 fabian18 Jan 14, 2024

Choose a reason for hiding this comment

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

tests/pkg/flashdb_fal_cfg/main.c:99: undefined reference to 'rtc_get_time'

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Please squash

@fabian18 fabian18 force-pushed the pr/flashdb_cfg_enhancement branch from 6a8af08 to 94f074e Compare January 18, 2024 20:47

# handle RTC backend after inclusion of $(RIOTBASE)/Makefile.include
ifeq (,$(filter periph_rtc,$(FEATURES_USED)))
$(info Using real-time-timer RTC)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this output messes up CI

The specified board Using does not exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not able to figure this out by the CI message.

* a KV with a length of 10K, you can use the control function to set the sector size to 12K or larger.
*
*/
#define CONFIG_FLASHDB_MIN_SECTOR_SIZE_EXP 12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should say that this is just the default but it can be different for 2 databases.
For that I would rename this to CONFIG_FLASHDB_DEFAULT_MIN_SECTOR_SIZE_EXP.

A module M that initializes a database should have something like:

#ifndef M_FLASHDB_MIN_SECTOR_SIZE_EXP
#define M_FLASHDB_MIN_SECTOR_SIZE_EXP CONFIG_FLASHDB_MIN_SECTOR_SIZE_EXP
#endif

Do you agree?

Hmm and the fact that it can be 10K or 12 actually means that it is not necessarily a power of 2.
I would change it to a factor x KiB Default would be 4 for 4K

Copy link
Contributor

Choose a reason for hiding this comment

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

the fact that it can be 10K or 12

Ah ok so there are no assumptions about this being a power of two in the code then - where did you find the 10k sector?

Copy link
Contributor Author

@fabian18 fabian18 Feb 7, 2024

Choose a reason for hiding this comment

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

In the documentation of now CONFIG_FLASHDB_MIN_SECTOR_SIZE_DEFAULT_FACTOR I copied something from the documentation of flashDB Sector size and block size.


#ifdef __cplusplus
extern "C" {
#endif

#if !defined(CONFIG_FLASHDB_MIN_SECTOR_SIZE_DEFAULT_FACTOR) || defined(DOXYGEN)
/**
* @brief Virtual sector size factor in multiples of 1KiB for FlashDB
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @brief Virtual sector size factor in multiples of 1KiB for FlashDB
* @brief Minimal virtual sector size in KiB for FlashDB

* a KV with a length of 10K, you can use the control function to set the sector size to 12K or larger.
*
*/
#define CONFIG_FLASHDB_MIN_SECTOR_SIZE_DEFAULT_FACTOR 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#define CONFIG_FLASHDB_MIN_SECTOR_SIZE_DEFAULT_FACTOR 4
#define CONFIG_FLASHDB_MIN_SECTOR_SIZE_DEFAULT_KiB 4

Instead of FACTOR you can just use the unit.

The MIN DEFAULT is a bit confusion, maybe add a note that this is only for the automatic partition configuration.

@fabian18 fabian18 force-pushed the pr/flashdb_cfg_enhancement branch from e6b67ee to 057695b Compare February 8, 2024 19:48
@benpicco benpicco added this pull request to the merge queue Feb 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 9, 2024
@benpicco
Copy link
Contributor

benpicco commented Feb 9, 2024

You might have to run make generate-Makefile.ci

@benpicco benpicco added this pull request to the merge queue Feb 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 9, 2024
@benpicco benpicco added this pull request to the merge queue Feb 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 10, 2024
@fabian18 fabian18 force-pushed the pr/flashdb_cfg_enhancement branch from f702848 to bf4bfdf Compare February 10, 2024 22:22
@benpicco benpicco enabled auto-merge February 10, 2024 22:27
@benpicco benpicco added this pull request to the merge queue Feb 10, 2024
Merged via the queue into RIOT-OS:master with commit 270aa70 Feb 11, 2024
25 checks passed
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants