SFDP: consolidation of SFDP parsing [5/5] #12528
Conversation
ciarmcom
requested review from
a team,
SeppoTakalo,
jeromecoutant and
kyle-cypress
|
@VeijoPesonen, thank you for your changes. |
VeijoPesonen
changed the title
|
@SeppoTakalo @kyle-cypress @jeromecoutant Could we have a review please ? |
kyle-cypress
left a comment
kyle-cypress
left a comment
There was a problem hiding this comment.
Overall looks good. Flagged a couple of minor items.
Rebased on latest master locally and successfully ran storage tests on CY8CKIT_062_WIFI_BT.
Tried to run on CY8CKIT_064_SB as well but encountered an assertion (on both this branch and on master - so not caused by these changes):
[1583886448.20][CONN][RXD] Error Status: 0x80FF0144 Code: 324 Module: 255
[1583886448.21][CONN][RXD] Error Message: Assertion failed: _area_params[0].size == _area_params[1].size
[1583886448.21][CONN][RXD] Location: 0x1000ED11
[1583886448.21][CONN][RXD] File: ./features/storage/kvstore/tdbstore/TDBStore.cpp+209
[1583886448.22][CONN][RXD] Error Value: 0x0
[1583886448.22][CONN][RXD] Current Thread: main Id: 0x8001BE8 Entry: 0x1000FFE5 StackSize: 0x1000 StackMem: 0x8002A48 SP: 0x80037CC
[1583886448.24][CONN][RXD] For more info, visit: https://mbed.com/s/error?error=0x80FF0144&osver=999999&core=0x410FC241&comp=2&ver=60300&tgt=CY8CPROTO_064_SB
[1583886448.24][CONN][RXD] -- MbedOS Error Info --
I'll investigate more tomorrow and file a separate issue if necessary.
| goto exit_point; | ||
| } | ||
| /**************************** Parse SFDP data ***********************************/ | ||
| { |
There was a problem hiding this comment.
This set of curly braces doesn't seem to serve a purpose (no flow control, and contains no declarations so it is not being used to introduce a child scope)
There was a problem hiding this comment.
True, only reason I added those was to show clearly which part of the code has something to do with SFDP parsing. And to remove comments which were saying the same thing as functions being called.
|
@VeijoPesonen could you please take a look at @kyle-cypress 's comments ? |
| } | ||
|
|
||
| /**************************** Parse SFDP headers and tables ***********************************/ | ||
| { |
There was a problem hiding this comment.
Why is this set of scope brackets here ?
There was a problem hiding this comment.
Only reason I added those was to show clearly which part of the code has something to do with SFDP parsing. And to remove comments which were saying the same thing as functions being called.
| return status; | ||
| } | ||
|
|
||
| int SPIFBlockDevice::_handle_vendor_quirks() |
There was a problem hiding this comment.
Is this a new externally accessible interface function ?
There was a problem hiding this comment.
No, it's actually private even when it might look like it's public. private-label has been used twice in this file.
drivers/source/SFDP.cpp
Outdated
| hdr_info.bptbl.addr = sfdp_get_param_tbl_ptr(phdr_ptr->DWORD2); | ||
| hdr_info.bptbl.size = std::min((phdr_ptr->P_LEN * 4), SFDP_BASIC_PARAMS_TBL_SIZE); | ||
| break; | ||
| /* TODO: Following headers should be parsed */ |
There was a problem hiding this comment.
We shouldn't have TODO's in released code....
There was a problem hiding this comment.
This doesn't look ready for coming into master? Should this really be on a feature branch ?
There was a problem hiding this comment.
This is a good question @VeijoPesonen @SeppoTakalo why are we not parsing these headers? Do we need to mention somewhere not supported cases?
There was a problem hiding this comment.
If these TODo's are due to currently unsupported devices or functionality, then we should say something along those lines ie 'This is currently unsupported functionality'. This should also be in our docs somewhere ? 'TODO's ' look unprofessional in released code....
There was a problem hiding this comment.
We shouldn't have TODO's in released code....
Personally I would prefer to see if something is not implemented. I wouldn't want to give a wrong impression that SFDP database parsing takes into consideration all possible combinations as long as it's not true.
There was a problem hiding this comment.
If these TODo's are due to currently unsupported devices or functionality, then we should say something along those lines ie 'This is currently unsupported functionality'. This should also be in our docs somewhere ? 'TODO's ' look unprofessional in released code....
TODO -> UNSUPPORTED, agreed.
There was a problem hiding this comment.
This doesn't look ready for coming into master? Should this really be on a feature branch ?
The plan is not to implement parsing of all possible headers so I don't see a reason to introduce a feature branch.
| return largest_erase_type; | ||
| } | ||
|
|
||
| int sfdp_detect_device_density(uint8_t *bptbl_ptr, sfdp_bptbl_info &bptbl_info) |
There was a problem hiding this comment.
Is this also another publicly available interface function ?
There was a problem hiding this comment.
Yes, this time that is the case.
Apologies for the delayed response, I took a vacation last week. I'll come back to this as soon as possible. |
mergify
bot
dismissed stale reviews from SeppoTakalo and adbridge
Pull request has been modified.
Earlier it was assumed that the table has a certain max length but that isn't true.
Making search for common erase type between regions more clear
C++14-ify memory allocation.
Indexing runs from highest to lowest, not other way round.
|
CI started |
Test run: FAILEDSummary: 1 of 7 test jobs failed Failed test jobs:
|
|
Test results updated. |
|
Dynamic memory usage reported to @ARMmbed/mbed-os-test team, will be reviewed |
|
Should be fixed now so I will restart CI |
|
CI started |
Test run: SUCCESSSummary: 7 of 7 test jobs passed |
adbridge
added
ready for merge
release-version: 6.0.0-beta-1
and removed
needs: CI
labels
8 participants
Summary of changes
Depends on PR #12524[merged].
Purpose of this PR is to consolidate SFDP header and table parsing and make the QSPIF- and SPIFBlockDevices to use the same shared implementation. Before this PR almost the same code was found from both of the components.
Impact of changes
Migration actions required
Documentation
None
Pull request type
Test results
Failures with Cypress targets are also visible with master branch.
Reviewers
@SeppoTakalo
@kyle-cypress
@jeromecoutant