-
Notifications
You must be signed in to change notification settings - Fork 100
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
feat(spi_nand_flash): Add linux target support #472
base: master
Are you sure you want to change the base?
feat(spi_nand_flash): Add linux target support #472
Conversation
05e804f
to
4790c94
Compare
4790c94
to
6082e74
Compare
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.
If I understand correctly, NAND flash is emulated by the "storage" partition in the emulated SPI flash.
If yes, this raises a couple of questions.
First, emulated SPI flash size is currently limited to 4 MB (espressif/esp-idf#14894). So it seems we can only emulate very tiny NAND flash, if we rely on esp_partition calls. This looks like a limitation.
Second, hard-coded usage of the "storage" partition doesn't seem to be documented. How should the user of this library find out that the spi_nand_flash component is using the "storage" partition, so that they avoid using "storage" themselves?
I also wonder if you have considered any alternative designs while working on this feature. For example, storing NAND flash contents in a separate file — which wouldn't be impacted by the above limitations. What are the reasons why you have chosen this particular design over the alternatives?
By the way, I would also suggest including such information in the PR description. A few sentences with an overview of the design or implementation, and some reasons why it was chosen, can help reviewers a lot.
17ef5d3
to
e7d6c04
Compare
Thank you for your comments @igrr. Regarding PR documentation, Agreed! Adding a design rationale in the PR description will help reviewers better understand the approach. I’ll make sure to include it moving forward. Additionally, I have updated the README.md file to include documentation for these changes. |
d8c8cfe
to
a7b4b6d
Compare
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.
First round of the final review done. Thanks for revamping the previous design, now all generally LGTM, except for the dependencies: please, try to resolve the FatFS separation, as there should be no relation of any file system to this component. Consider placing all the FatFS specific code (diskio, examples) into IDF, and add the dependency on spi_nand_flash there (similar to esp_littlfs model). Thanks
spi_nand_flash/CMakeLists.txt
Outdated
@@ -1,28 +1,44 @@ | |||
idf_build_get_property(target IDF_TARGET) | |||
|
|||
set(reqs fatfs) |
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.
Why do we need dependency on FatFS?
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.
This dependency is primarily required for the diskio layer implementation. However, I have now made it private to this component, preventing other components from inheriting it directly.
|
||
```mermaid | ||
graph TD | ||
A[Application] --> B[FATFS] |
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.
FatFS is unrelated to this component, imo. See previous comment
@@ -133,11 +133,12 @@ DRESULT ff_nand_ioctl(BYTE pdrv, BYTE cmd, void *buff) | |||
break; | |||
} | |||
#if FF_USE_TRIM | |||
case CTRL_TRIM: | |||
case CTRL_TRIM: { |
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.
Dtto. This update should be internal to the FatFS code (please, check CMake for possible forking)
"src/nand_impl_wrap.c" | ||
"src/nand_diag_api.c" | ||
"src/spi_nand_oper.c" | ||
"vfs/vfs_fat_spinandflash.c") |
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.
FatFS deps, see previous comments
231ffc1
to
cbc7032
Compare
cbc7032
to
40e002c
Compare
40e002c
to
555fe21
Compare
Change description
Below is the implementation architecture of the spi_nand_flash component.
This PR adds support for the Linux target, following the Linux path shown in the diagram above. The files nand_impl_linux.c and nand_linux_mmap_emul.c have been added to implement the NAND emulation layer and memory-mapped file handling which is used to emulate NAND Flash.
Additionally, host_tests have been included to verify that all tests function correctly on the Linux platform.