FlashIAP driver: Add retries to erase and program operations. #9548
Conversation
|
@davidsaada, thank you for your changes. |
drivers/FlashIAP.cpp
Outdated
| if (ret != 0) { | ||
| ret = -1; | ||
| break; | ||
| // Few boards may fail the write actions due to HW limitations (like critical drivers that |
There was a problem hiding this comment.
Please fix the comment to "erase" instead of "write"
drivers/FlashIAP.cpp
Outdated
| // Few boards may fail the write actions due to HW limitations (like critical drivers that | ||
| // disable flash operations). Just retry a few times until success. | ||
| ret = -1; | ||
| for (unsigned int retry = 0; retry < num_write_retries && ret; retry++) { |
There was a problem hiding this comment.
should be an exit trigger of retries loop in case of success (maybe if total_size == 0)?
There was a problem hiding this comment.
ret is the exit trigger (ret == 0 will exit on the first iteration).
There was a problem hiding this comment.
sorry, I missed ret at the condition.
Could flash_program_page() be called several times for one "program" ?
If so, what happens if not the first one fails , will it retry from the beginning?
There was a problem hiding this comment.
Very good point. It will retry from the beginning. This will likely succeed in most of the flash components (programming the same value twice), if not all, but there's always a chance that this logic won't always work. Will try to modify the code accordingly.
There was a problem hiding this comment.
Modified the code to retry the physical operation (and not on the entire process). Please re-review.
davidsaada
force-pushed
the
david_flashiap_retries
branch
from
f551df4
to
00ee88c
Compare
Few boards may fail the write actions due to HW limitations (like critical drivers that disable flash operations). Just retry a few times until success. In addition, remove the redundant retries in NVStore (not needed now).
davidsaada
force-pushed
the
david_flashiap_retries
branch
from
00ee88c
to
48a3ae9
Compare
There was a problem hiding this comment.
Changes look fine to me.
For my understanding:
Issue was noticed earlier with NVStore and fix is now made generic for FlashIAP as I understand. But any reason for magic number 16 (I am happy that we are limiting to some number and not in while(1)) just curious.
Also description states failure is due to HW limitation like critical drivers that disable flash operations, do we have more information on this? Which driver disables flash operation in Mbed OS.
|
CI started |
@deepikabhavnani We started noticing this issue in NVStore, but were afraid to make the change in the driver back then, which was a mistake apparently. We saw it happening with WiFi (which is also what the client test application operated in the ODIN board), but then learned that this problem could also happen in other protocols such as BLE. |
I would like to also understand this. So some stacks just disable these operations and led to failures in kvstore. |
Was KVStore in this case (discovered by client tests), but the failures were lower - in the flash driver, hence the change. |
Test run: SUCCESSSummary: 12 of 12 test jobs passed |
FlashIAP driver: Add retries to erase and program operations.
Description
Few boards may fail the write actions due to HW limitations (like critical drivers that
disable flash operations). Just retry a few times until success.
In addition, remove the redundant retries in NVStore (not needed now).
This fixes failures we saw in client tests, mainly with the UBLOX_EVK_ODIN_W2 board.
Tested with all flash related tests (FlashIAP, NVStore, KVStore) on K64F and UBLOX_EVK_ODIN_W2.
Pull request type