-
Notifications
You must be signed in to change notification settings - Fork 3k
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
BUGFIX: SFDP Sector Map Table Parameter ID LSB is 0x81 #12270
BUGFIX: SFDP Sector Map Table Parameter ID LSB is 0x81 #12270
Conversation
The Sector Map Function Specific Table is assigned the ID LSB of 81 in hexadecimal, not in decimal[1]. [1] JESD216.01 p. 53
@VeijoPesonen, thank you for your changes. |
@VeijoPesonen Please review Travis failures |
SFDP logic is the same between SPIF and QSIP.
@0xc0170 , @SeppoTakalo astyle and doxygen issues has been fixed now. |
Separates SFDP header retrieval and moves it as a part of the earlier introduced SFDP file. Purpose is to abstract away differences between SPIF and QSPIF devices when it comes to fetching the SFDP headers from a device.
@0xc0170 @SeppoTakalo could you please confirm you are happy with the latest changes ? |
CI started |
Test run: FAILEDSummary: 3 of 4 test jobs failed Failed test jobs:
|
One of the issuesThe issues don't seem to be caused by the changes I made.
|
Pull request has been modified.
I'm not sure are these related but I'm seeing following things in the logs: K66F failure
@adbridge would you please restart the tests to see if compilation error with mbed-os-example-blinky-baremetal has some kind of trickle-down effect which causes failures elsewhere. |
Re starting ci to see if there is a real issue |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
Examples test appears to be running twice - first with
then a second time exactly the same command but with Here, and also in my PR #12142 (run 3), the re-run with Reason unknown - the |
@adbridge, @kjbracey-arm, @SeppoTakalo From my point of view this PR is ready and could be merged in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from tiny documentation improvement request I have one larger question. @VeijoPesonen , have you considered adding the header parsing to BlockDevice
class, so that both subclasses can inherit it, instead of adding separate global functions which need to be befriended with the modules using them?
Haven't really thought about it but is SFDP even logically part of BlockDevice class? I would say it isn't because not all BlockDevices do use SFDP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my point of view this is OK. Doxygen changes to be added in another PR and I accept the logical explanation of why we can't embed SPDF parser into BlockDevice
class.
@kjbracey-arm, @SeppoTakalo This PR is ready and should be merged. |
@0xc0170 This one should be ready to go in. |
@VeijoPesonen Thanks for the reminder. I restarted CI to get the latest run, will be merged asap it completes |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
Summary of changes
Fixes #11722. Value of the SFDP Sector Map Table Parameter ID LSB is 0x81, not 81 in decimal.
Starts also the work where SFDP specific functionality is split from the SPIF and QSPIF Blockdevices to a separate file. At the moment it means retrieval and parsing of the SFDP- and SFDP Parameter-headers.
Impact of changes
Migration actions required
Documentation
Pull request type
Test results
SPIF compilation
mbed test --compile -t ARM -m NRF52840_DK -n features-storage-filesystem-littlefs-* --app-config tools/test_configs/SPIFBlockDeviceAndHeapBlockDevice.json
QSPIF compilation
mbed test --compile -t GCC_ARM -m NRF52840_DK -n features-storage-filesystem-littlefs-* --app-config tools/test_configs/QSPIFBlockDeviceAndHeapBlockDevice.json
Reviewers
@SeppoTakalo