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.

Sign up for GitHub

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

Jump to bottom

periph/flashpage: remove deprecated flashpage_*_free functions #18093

Merged
merged 6 commits into from
Apr 17, 2024
Merged

periph/flashpage: remove deprecated flashpage_*_free functions #18093

merged 6 commits into from
Apr 17, 2024

Conversation

Ollrogge
Copy link
Member

Contribution description

With the 2022.04 release being out this PR removes the flashpage_*_free functions deprecated with #17860.

I reverted all the changes I introduced with #16972 except for be254d4 since I this was a kind of fix that is still valid in my opinion.

Testing procedure

  • tests/periph_flashpage

Issues/PRs references

#17860
#16972

@github-actions github-actions bot added Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: MSP Platform: This PR/issue effects MSP-based platforms Platform: native Platform: This PR/issue effects the native platform Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels May 12, 2022
@benpicco
Copy link
Contributor

Those functions came in handy to get the current image with riotboot_vfs_dump_rom() in #17379 though

@Ollrogge
Copy link
Member Author

Those functions came in handy to get the current image with riotboot_vfs_dump_rom() in #17379 though

Well in this case the functions can stay since they now have a use case I guess. @chrysn do you have any thoughts on this ?

@chrysn
Copy link
Member

chrysn commented May 16, 2022

If we need a way to tell which flash pages correspond to the text section, let's use that (introducing first_used / last_used, or just feed the relevant linker symbols through the mapped address to flash page conversion -- a bootloader needs to understand some section stuff anyway). These functions invite usage patterns that conflict between parts of an application, and should go.

@Ollrogge
Copy link
Member Author

If we need a way to tell which flash pages correspond to the text section, let's use that (introducing first_used / last_used, or just feed the relevant linker symbols through the mapped address to flash page conversion -- a bootloader needs to understand some section stuff anyway). These functions invite usage patterns that conflict between parts of an application, and should go.

I can change the functions to reflect this but it should be first_used / last_used based on the whole firmware not just the .text section right ?

@chrysn
Copy link
Member

chrysn commented May 19, 2022

I can change the functions to reflect this but it should be first_used / last_used based on the whole firmware not just the .text section right ?

That's really what whoever does the firmware dump needs to concern itself with (is it dumping firmware? or everything allocated by the linker? also pages used through the deprecated functions? or the full ROM?) -- another reason why I'd rather not have these functions used for that purpose.

@Ollrogge
Copy link
Member Author

I can change the functions to reflect this but it should be first_used / last_used based on the whole firmware not just the .text section right ?

That's really what whoever does the firmware dump needs to concern itself with (is it dumping firmware? or everything allocated by the linker? also pages used through the deprecated functions? or the full ROM?) -- another reason why I'd rather not have these functions used for that purpose.

@benpicco what is your opinion?

@Ollrogge
Copy link
Member Author

@benpicco @chrysn I don't care if the functions stay or get removed. You guys have to discuss & decide please.

@Teufelchen1
Copy link
Contributor

@chrysn @benpicco ping! 😠 ⏳
The functions have been marked deprecated for years! Either, we move this forward or we de-deprecate them. The current state is not okay for me.

chrysn
chrysn approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

Looks good to me (didn't test, all relevant tests should be doable by CI), please rebase and let's get this merged.

tests/periph_flashpage/main.c Outdated
@@ -31,6 +31,14 @@

#define LINE_LEN (16)

/* For MSP430 cpu's the last page holds the interrupt vector, although the api
Copy link
Member

Choose a reason for hiding this comment

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

I think we should fix our tests to not depend on this, but while they are as they are, this looks like a viable workaround.

@chrysn chrysn added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Apr 2, 2024
@Ollrogge
Copy link
Member Author

Ollrogge commented Apr 4, 2024

@benpicco what is your call ? If you are fine with the functions being removed I will rebase.

@benpicco
Copy link
Contributor

benpicco commented Apr 4, 2024 •
edited
Loading

flashpage_first_free() was useful to dump the current firmware (to enable roll-back) in a single-slot design where the new firmware is stored on e.g. external flash. (#17379)

But since that has not been merged and there is currently no use for this, I'm fine with dropping those functions.

I can re-introduce the function under a different name if the need for single-slot updates with external storage arises again.

@Ollrogge Ollrogge force-pushed the remove_flashpage_free_funcs branch from 9b63368 to 10c987e Compare April 4, 2024 16:02
@github-actions github-actions bot removed the Area: build system Area: Build system label Apr 4, 2024
@riot-ci
Copy link

riot-ci commented Apr 4, 2024 •
edited
Loading

Murdock results

✔️ PASSED

10c987e Revert "periph/flashpage: extend API"

Success Failures Total Runtime
10048 0 10048 13m:22s

Artifacts

@Ollrogge
Copy link
Member Author

Ollrogge commented Apr 4, 2024

Murdock results

❌ FAILED

10c987e Revert "periph/flashpage: extend API"
Success Failures Total Runtime
3710 0 9428 04m:07s

Artifacts

Idk why this failed ?

@Teufelchen1
Copy link
Contributor

I think that's unrelated. We might have bug in the samr21-xpro timer implementation, maybe related to #10019

@chrysn chrysn added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 4, 2024
@Teufelchen1
Copy link
Contributor

The problem was probably fixed in #20548.

@Teufelchen1 Teufelchen1 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 9, 2024
@Ollrogge
Copy link
Member Author

All green now :)

@Teufelchen1 Teufelchen1 added this pull request to the merge queue Apr 16, 2024
Merged via the queue into RIOT-OS:master with commit 5bc8ca5 Apr 17, 2024
29 checks passed
@fabian18 fabian18 mentioned this pull request May 8, 2024
Merged
@mguetschow mguetschow added this to the Release 2024.07 milestone Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrysn chrysn chrysn approved these changes

@smlng smlng Awaiting requested review from smlng

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@gschorcht gschorcht Awaiting requested review from gschorcht gschorcht is a code owner

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers 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 CI: run tests If set, CI server will run tests on hardware for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: MSP Platform: This PR/issue effects MSP-based platforms Platform: native Platform: This PR/issue effects the native platform Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms
Projects
None yet
Milestone
Release 2024.07
Development

Successfully merging this pull request may close these issues.
6 participants
@Ollrogge @benpicco @chrysn @Teufelchen1 @riot-ci @mguetschow