subsys: fs: fcb: struct flash_sector usually const #80268

JPHutchins · 2024-10-23T01:55:53Z

Generally, the flash_sector array is known at compile time and is immutable, therefore developers may like to store it in ROM. The only function that requires a mutable struct flash_sector seems to be flash_area_get_sectors(), allowing the developer to store a mutable sector definition in persistent RAM rather than ROM. An example of this approach is in setting_fcb.c.

JPHutchins · 2024-11-06T02:20:05Z

I am able to reproduce the CI error on local:

./scripts/coccicheck --mode=report --cocci=scripts/coccinelle/same_identifier.cocci | grep fcb
/home/jp/repos/zephyrproject/zephyr/tests/subsys/fs/fcb/src/fcb_test_last_of_n.c:13:13-16: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/subsys/fs/fcb/fcb_append.c:59:23-26: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/subsys/fs/fcb/fcb_append.c:120:30-33: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/subsys/fs/fcb/fcb_append.c:39:34-37: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/subsys/fs/fcb/fcb_append.c:14:61-64: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/tests/subsys/fs/fcb/src/fcb_test_empty_walk.c:13:13-16: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/tests/subsys/fs/fcb/src/fcb_test_append_too_big.c:12:14-17: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/tests/subsys/fs/fcb/src/fcb_test_rotate.c:12:13-16: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/tests/subsys/fs/fcb/src/fcb_test_multiple_scratch.c:12:13-16: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/tests/subsys/fs/fcb/src/fcb_test_append_fill.c:12:13-16: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/tests/subsys/fs/fcb/src/fcb_test_init.c:13:13-16: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/subsys/fs/fcb/fcb_walk.c:15:25-28: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/tests/subsys/fs/fcb/src/fcb_test_reset.c:12:13-16: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/subsys/fs/fcb/fcb_rotate.c:12:23-26: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/subsys/fs/fcb/fcb_getnext.c:108:24-27: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/subsys/fs/fcb/fcb_getnext.c:14:34-37: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/subsys/fs/fcb/fcb_getnext.c:43:31-34: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
/home/jp/repos/zephyrproject/zephyr/subsys/fs/fcb/fcb_getnext.c:33:58-61: WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb

But not sure how to improve!

github-actions · 2025-01-06T00:35:20Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

JPHutchins · 2025-01-06T00:55:19Z

Needs rebase.

de-nordic

Please keep the const type qualifier between type definition and after storage class (extern).
The change sticks out from all the other code, including modified files:

kartben · 2025-01-31T03:53:48Z

Needs rebase.

Yes but you're the author? 🙂

Generally, the flash_sector array is known at compile time and is immutable, therefore developers may like to store it in ROM. The only function that requires a mutable struct flash_sector seems to be flash_area_get_sectors(), allowing the developer to store a mutable sector definition in persistent RAM rather than ROM. An example of this approach is in setting_fcb.c. Signed-off-by: JP Hutchins <[email protected]>

JPHutchins · 2025-01-31T06:03:32Z

Any insight on these compliance check problems? They are present on lines that are not modified by this PR.

Error: subsys/fs/fcb/fcb_append.c:14:WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
subsys/fs/fcb/fcb_getnext.c:33:WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
subsys/fs/fcb/fcb_walk.c:15:WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
Error: Process completed with exit code 1.

de-nordic · 2025-01-31T09:36:45Z

Any insight on these compliance check problems? They are present on lines that are not modified by this PR.

Error: subsys/fs/fcb/fcb_append.c:14:WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
subsys/fs/fcb/fcb_getnext.c:33:WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
subsys/fs/fcb/fcb_walk.c:15:WARNING: Violation to rule 5.7 (Tag name should be unique) tag: fcb
Error: Process completed with exit code 1.

I guess CI does not like name of variable to be the same as type used to define it.
It was that before, but you have touched the line, so CI can now insist on you to fix it.

kartben · 2025-01-31T09:40:13Z

ya most of these files hadn't been touched in many years, and the coding guidelines check has been introduced since then. Luckily the fix should be pretty straightforward

fabiobaltieri · 2025-03-06T10:47:11Z

tests/subsys/fs/fcb/src/main.c

-struct flash_sector test_fcb_sector[] = {
-	[0] = {
-		.fs_off = 0,
-		.fs_size = SECTOR_SIZE
-	},
-	[1] = {
-		.fs_off = SECTOR_SIZE,
-		.fs_size = SECTOR_SIZE
-	},
-	[2] = {
-		.fs_off = 2 * SECTOR_SIZE,
-		.fs_size = SECTOR_SIZE
-	},
-	[3] = {
-		.fs_off = 3 * SECTOR_SIZE,
-		.fs_size = SECTOR_SIZE
-	}
-};
-
+const struct flash_sector test_fcb_sector[] = {
+	[0] = {.fs_off = 0, .fs_size = SECTOR_SIZE},
+	[1] = {.fs_off = SECTOR_SIZE, .fs_size = SECTOR_SIZE},
+	[2] = {.fs_off = 2 * SECTOR_SIZE, .fs_size = SECTOR_SIZE},
+	[3] = {.fs_off = 3 * SECTOR_SIZE, .fs_size = SECTOR_SIZE}};


unrelated change? same in another couple of files, PRs should nto change unrelated code

@JPHutchins If you did run clang format on the code, please do not do that. At least not with other changes.

github-actions · 2025-05-06T00:38:24Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

de-nordic · 2025-05-06T15:47:20Z

Any movement here?

github-actions · 2025-07-06T00:44:18Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

zephyrbot added area: Settings Settings subsystem area: File System labels Oct 23, 2024

zephyrbot requested review from Laczen, de-nordic and nashif October 23, 2024 01:56

zephyrbot assigned de-nordic Oct 23, 2024

de-nordic added the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label Oct 23, 2024

de-nordic added this to the 4.1 milestone Oct 23, 2024

Laczen removed their request for review October 24, 2024 03:38

erwango modified the milestones: 4.1, v4.1.0 Oct 24, 2024

JPHutchins force-pushed the refactor/fcb-flash-sectors-const branch from 8d068c2 to 82f8700 Compare November 6, 2024 01:54

github-actions bot added the Stale label Jan 6, 2025

github-actions bot removed the Stale label Jan 7, 2025

de-nordic requested changes Jan 14, 2025

View reviewed changes

JPHutchins force-pushed the refactor/fcb-flash-sectors-const branch from 82f8700 to 5ea63d0 Compare January 31, 2025 05:32

JPHutchins requested a review from de-nordic January 31, 2025 05:33

fabiobaltieri modified the milestones: v4.1.0, v4.2.0 Mar 6, 2025

fabiobaltieri reviewed Mar 6, 2025

View reviewed changes

github-actions bot added the Stale label May 6, 2025

de-nordic removed the Stale label May 6, 2025

github-actions bot added the Stale label Jul 6, 2025

github-actions bot closed this Jul 20, 2025

subsys: fs: fcb: struct flash_sector usually const #80268

subsys: fs: fcb: struct flash_sector usually const #80268

Uh oh!

Conversation

JPHutchins commented Oct 23, 2024

Uh oh!

JPHutchins commented Nov 6, 2024

Uh oh!

github-actions bot commented Jan 6, 2025

Uh oh!

JPHutchins commented Jan 6, 2025

Uh oh!

de-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

kartben commented Jan 31, 2025

Uh oh!

JPHutchins commented Jan 31, 2025

Uh oh!

de-nordic commented Jan 31, 2025

Uh oh!

kartben commented Jan 31, 2025

Uh oh!

fabiobaltieri Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

de-nordic Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 6, 2025

Uh oh!

de-nordic commented May 6, 2025

Uh oh!

github-actions bot commented Jul 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants