Skip to content

STM32F4 FLASH API update #13802

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
Merged
merged 1 commit into from
Nov 11, 2020
Merged

STM32F4 FLASH API update #13802

merged 1 commit into from
Nov 11, 2020

Conversation

jeromecoutant
Copy link
Collaborator

Summary of changes

Fixes #13792

We need to add critical section in flash_erase_sector function

@ARMmbed/team-st-mcd
@moshe-shahar

Impact of changes

Migration actions required

Documentation

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[x] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Oct 22, 2020
@ciarmcom ciarmcom requested a review from a team October 22, 2020 15:30
@ciarmcom
Copy link
Member

@jeromecoutant, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

0xc0170
0xc0170 previously requested changes Oct 22, 2020
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Can you add to the commit msg why is this change needed (what is it fixing) ? why we need critical section whyle we erase (I assume flash is directly used and it's not interrupt safe) ?

@mergify mergify bot dismissed 0xc0170’s stale review October 23, 2020 11:52

Pull request has been modified.

@jeromecoutant
Copy link
Collaborator Author

Can you add to the commit msg why is this change needed

Done

0xc0170
0xc0170 previously approved these changes Oct 23, 2020
View reviewed changes
@moshe-shahar
Copy link
Contributor

@jeromecoutant , I would expect to see the same change in flash_program_page.
Is there any difference between the two APIs?

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 26, 2020

@jeromecoutant , I would expect to see the same change in flash_program_page.
Is there any difference between the two APIs?

As @jeromecoutant is out this week, @moshe-shahar shall we wait to review also flashing part or make this 2 step fix (this one progresses alone and the issue won't be closed until programming is also reviewed).

@teetak01
Copy link
Contributor

Do we need similar fixes to the other flash_api.c files? There seems to be similar implementations also for other targets.

@moshe-shahar
Copy link
Contributor

@jeromecoutant , for my understanding, it is only a partial fix.
Because it's not hanging consistently, as @teetak01 mentioned before, it could pop in other targets too.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 2, 2020

CI started

The other targets will need own fixes (one possible way could be to note in the documentation what implication this has when using flash functionality - someone should review if protecting this via critical section is correct no matter what target). We should keep #13792 opened until this is answered.

@mbed-ci
Copy link

mbed-ci commented Nov 2, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@moshe-shahar
Copy link
Contributor

moshe-shahar commented Nov 2, 2020 •
edited
Loading

@jeromecoutant , I would expect to see the same change in flash_program_page.
Is there any difference between the two APIs?

This question was not answered.
Does the program process also need to be protected?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 2, 2020

Set back to review stage

@jeromecoutant
Copy link
Collaborator Author

I would expect to see the same change in flash_program_page.
Is there any difference between the two APIs?

I agree, patch update is on going

@jeromecoutant
Copy link
Collaborator Author

Do we need similar fixes to the other flash_api.c files? There seems to be similar implementations also for other targets.

I agree, only STM32WB is currently well protected.
I propose to go on with only STM32F4 in this PR, I will update other families soon after.

Thx

@jeromecoutant
STM32F4 FLASH update
5cd3533 
Add critical section in
- flash_erase_sector and
- flash_program_page
to make FLASH erase procedure interrupt safe
(can occur with Ethernet)
@jeromecoutant
Copy link
Collaborator Author

I agree, patch update is on going

Done

@mergify mergify bot dismissed 0xc0170’s stale review November 5, 2020 16:47

Pull request has been modified.

teetak01
teetak01 approved these changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@teetak01 teetak01 left a comment

Choose a reason for hiding this comment

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

Verified to fix the client issue.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 10, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Nov 10, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_cmake-example-test ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@0xc0170 0xc0170 merged commit f550ed3 into ARMmbed:master Nov 11, 2020
@mergify mergify bot removed the ready for merge label Nov 11, 2020
@jeromecoutant jeromecoutant deleted the PR_FLASH_F4 branch November 12, 2020 07:52
JeanMarcR added a commit to JeanMarcR/mbed-os that referenced this pull request Nov 13, 2020
@JeanMarcR
FLASH API UPDATE
c6911e5 
See PR ARMmbed#13802 (for F4 board)

Concerned boards are

STM32F0
STM32F1
STM32F2
STM32F3
STM32F7
STM32G0
STM32G4
STM32H7
STM32L0
STM32L1
STM32L4
STM32L5
@JeanMarcR JeanMarcR mentioned this pull request Nov 17, 2020
Merged
JeanMarcR added a commit to JeanMarcR/mbed-os that referenced this pull request Nov 17, 2020
@JeanMarcR
STM32 FLASH API : add critical sections
6d671e8 
See PR ARMmbed#13802 (for F4 board)

Concerned boards are

STM32F0
STM32F1
STM32F2
STM32F3
STM32F7
STM32G0
STM32G4
STM32H7
STM32L0
STM32L1
STM32L4
STM32L5

Adding test of return code of HAL_FLASH_Lock() function.
@jeromecoutant
Copy link
Collaborator Author

Do we need similar fixes to the other flash_api.c files? There seems to be similar implementations also for other targets.

@teetak01 Please review #13914

JeanMarcR added a commit to JeanMarcR/mbed-os that referenced this pull request Nov 18, 2020
@JeanMarcR
STM32 FLASH API : add critical sections
7266ead 
See PR ARMmbed#13802 (for F4 board)

Concerned boards are

STM32F0
STM32F1
STM32F2
STM32F3
STM32F4
STM32F7
STM32G0
STM32G4
STM32H7
STM32L0
STM32L1
STM32L4
STM32L5

Adding test of return code of HAL_FLASH_Lock() function
Adding board STM32F4
Running AStyle
JeanMarcR added a commit to JeanMarcR/mbed-os that referenced this pull request Nov 18, 2020
@JeanMarcR
STM32 FLASH API : add critical sections
ba37eb8 
See PR ARMmbed#13802 (for F4 board)

Concerned boards are

STM32F0
STM32F1
STM32F2
STM32F3
STM32F4
STM32F7
STM32G0
STM32G4
STM32H7
STM32L0
STM32L1
STM32L4
STM32L5

Adding test of return code of HAL_FLASH_Lock() function
Adding board STM32F4
Running AStyle
JeanMarcR added a commit to JeanMarcR/mbed-os that referenced this pull request Nov 20, 2020
@JeanMarcR
STM32 FLASH API : add critical sections
16e63dc 
See PR ARMmbed#13802 (for F4 board)

Concerned boards are

STM32F0
STM32F1
STM32F2
STM32F3
STM32F4
STM32F7
STM32G0
STM32G4
STM32H7
STM32L0
STM32L1
STM32L4
STM32L5

Adding test of return code of HAL_FLASH_Lock() function
Adding board STM32F4
Running AStyle
@mbedmain mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@teetak01 teetak01 teetak01 approved these changes

Assignees
No one assigned
Labels
release-version: 6.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NUCLEO_F429ZI hangs during flash erase
7 participants
@jeromecoutant @ciarmcom @moshe-shahar @0xc0170 @teetak01 @mbed-ci @mbedmain