fs/mnemofs: Add portable bit primitives and cleanup hash functions#18506
fs/mnemofs: Add portable bit primitives and cleanup hash functions#18506acassis merged 1 commit intoapache:masterfrom
Conversation
@acassis Please look into this PR |
|
Hello!
Can you please include some logs or more information about how you tested this?
Can you give some information about the configuration you used for this?
Could you show some logs from this testing? Which configuration did you use for this? |
|
@Sumit6307 it seems like you are planing to apply to NuttX GSoC right: https://github.com/Sumit6307/gsoc-nuttx ? Suggestion: there are still some projects without candidates: micro-ros, tinygl, nxboot, adc/dac, etc. It is better to apply for some of these projects than competing with other users, but the final decision is up to you. |
Hello @linguini1 , Thank you for the review and the helpful suggestions! Regarding @acassis's feedback: I have added the Doxygen-style documentation block for the mfs_clz function in fs/mnemofs/mnemofs.h as requested, explaining its functionality and return behavior. Regarding @linguini1's feedback: Verification of mfs_clz logic: I verified the portable binary search implementation of mfs_clz using a standalone test script to ensure correctness across various edge cases (0, powers of 2, boundary conditions). Verification Logs: text Build Configuration: The code was compiled targeting the standard sim:nsh (Simulator) configuration to ensure build consistency within the NuttX tree. Configuration used: bash Simulator Testing: I verified that the mnemofs module compiles without errors under the sim:nsh configuration. This PR primarily targets foundational bit-logic (Count Leading Zeros) and removes redundant code as per internal TODO suggestions. I have pushed these updates to the mnemofs-bit-pr branch. Looking forward to your thoughts! |
Hello @acassis , Yes, I am planning to apply for NuttX GSoC 2026. I understand that I should open a Discussion for a specific project like other candidates have done. I will follow this process from next time and create the discussion accordingly. Thanks for the guidance. |
@Sumit6307 thank you, please squash your commits, if you never did it before the steps are listed here: |
Perfect, you can find the available proposals here: https://cwiki.apache.org/confluence/display/COMDEV/GSoC+2026+Ideas+list |
Nice! Could you attach this script in your test section?
Okay! I don't think sim:nsh has mnemofs enabled though, can you let us know what configuration options you selected to get it to build?
Which updates? I don't see any new commits added. |
|
@acassis I'm fine with the changes themselves. The work is based on some TODOs I had, and the changes look fine to me. @Sumit6307 please fix the commit naming, PR naming, squash, rebase, and address the failing CI (it's related to your commit as well). As for GSoC, having contributions are good. But they're good because the community would know the developer is familiar with the project among other things. The developer, not their tools. That confidence in the developer + their proposal which shows a proper understanding with research into the problem and potential solutions is what gets the person selected as a GSoC contributor. I must stress on this, the developer is the asset here, not their tools. Hope that helps! |
|
@Sumit6307 you need to sign off your git commit to fix the CI error |
4ee4c66 to
7d89731
Compare
Please Check |
|
@Sumit6307 please fix the introduced issue: |
Before submitting your PR it is important to run the ./tools/checkpatch.sh locally to detect issues like it, it avoids spending build time in our CI. |
7d89731 to
b3c6df2
Compare
This PR addresses several portability and technical debt issues in the mnemofs filesystem by resolving source-level TODO items. Changes: - Implemented a portable fallback for mfs\_clz (Count Leading Zeros) in fs/mnemofs/mnemofs.h using a binary search approach. This ensures compatibility with non-GCC compilers. - Removed the redundant 8-bit mfs\_arrhash and consolidated hashing with the existing 16-bit mfs\_hash in mnemofs\_util.c. - Removed the related TODO comments in mnemofs.h and mnemofs\_util.c. - Fixed NuttX style (indentation and braces) in the fallback bit primitives. Signed-off-by: Sumit <sumit6307@gmail.com>
b3c6df2 to
ff1fec2
Compare
|
@acassis Sir I fixed all the Issue Please Check |
linguini1
left a comment
There was a problem hiding this comment.
Verification of
mfs_clz
logic: I verified the portable binary search implementation of
mfs_clz
using a standalone test script to ensure correctness across various edge cases (0, powers of 2, boundary conditions).
Verification Logs:
text
Value: 0x00000000 | Leading Zeros: 32
Value: 0x00000001 | Leading Zeros: 31
Value: 0x00000002 | Leading Zeros: 30
Value: 0x00000010 | Leading Zeros: 27
Value: 0x00000100 | Leading Zeros: 23
Value: 0x00010000 | Leading Zeros: 15
Value: 0x10000000 | Leading Zeros: 3
Value: 0x80000000 | Leading Zeros: 0
Value: 0xFFFFFFFF | Leading Zeros: 0
Value: 0x0000FFFF | Leading Zeros: 16Nice! Could you attach this script in your test section?
Build Configuration: The code was compiled targeting the standard sim:nsh (Simulator) configuration to ensure build consistency within the NuttX tree. Configuration used:
bash
./tools/configure.sh sim:nsh
make
Simulator Testing: I verified that the
mnemofs
module compiles without errors under the sim:nsh configuration. This PR primarily targets foundational bit-logic (Count Leading Zeros) and removes redundant code as per internal TODO suggestions.Okay! I don't think sim:nsh has mnemofs enabled though, can you let us know what configuration options you selected to get it to build?
Requesting change for requested information above ^
Verification Logs: 2. Build Configuration To build
Steps to build: I have also fixed the "Long line found" style issue on line 355 by splitting the credit URL. The PR should be ready for review now. |
|
Thanks @Sumit6307, @linguini1 was asking to put test information into the PR description, see https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md :-) @resyfer thank you for verification and confirmation :-) |
@cederom Thanks @linguini1 for pointing it out |
cederom
left a comment
There was a problem hiding this comment.
Thank you @Sumit6307 :-)
@cederom |
Summary
__builtin_clzon non-GCC compilers and resolves technical debt by removing redundant code based on internal TODOs.__builtin_clzmay not be available.mfs_arrhashand consolidated hashing with the existing 16-bit mfs_hash in mnemofs_util.c to improve metadata consistency.Impact
mnemofscomponent (e.g., non-GCC compilers).Testing
I confirm that changes are verified on local setup and works as intended:
sim:mnemofsTesting logs before change:
N/A
Testing logs after change:
Verification of mfs_clz and mfs_ctz logic using a standalone test script:
Build verification:
Successfully compiled the updated fs/mnemofs files using the sim:mnemofs configuration, which explicitly enables mnemofs and its NAND flash dependencies:
Required NAND flash stack and MTD dependencies:
CONFIG_FS_MNEMOFS=yCONFIG_MTD_NAND=yCONFIG_MTD_NAND_RAM=yCONFIG_ALLOW_BSD_COMPONENTS=yStandalone test script:
Here is the standalone C script I used to verify the portable mfs_clz and mfs_ctz implementations across various edge cases (0, powers of 2, boundary conditions):
PR verification Self-Check