nuttxpr commented on PR #3033: URL: https://github.com/apache/nuttx-apps/pull/3033#issuecomment-2744005039
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description mostly meets the NuttX requirements, but could be improved in a few key areas: **Summary:** * **Needs more detail on *why* this change is necessary.** What problem does this solve for users? Is it for benchmarking, debugging, or something else? Simply stating it "allows measuring" isn't enough justification. Explain the use case. * **More explicit about the functional change.** Instead of "Add MTD test...", be more specific, e.g., "Add a new application to the `testing/fs` directory that performs sequential read/write tests on MTD devices to measure transfer rates." **Impact:** * **User impact is understated.** While it provides a testing application, the patch to `esp32s3_board_spiflash.c` suggests more than just adding an application. Explain why that patch is necessary for this test to work. Does it enable some functionality needed by the test? If so, that's a larger impact than simply adding a test application. If the patch is unrelated, it shouldn't be included in this PR. * **Build impact is likely YES.** Adding a new application and potentially modifying board configuration files usually impacts the build system, at least by adding a new target. Clarify this. Even if the user has to explicitly enable the test application, it's still a build impact. * **Documentation impact is likely YES.** A new test application should have at least some minimal documentation (e.g., in the `README.txt` or a help message within the application itself). **Testing:** * **Build Host(s) is missing.** Specify the OS, CPU architecture, and compiler version used for testing. * **Patch application should be avoided.** Submitting a patch that needs another patch is confusing. Either integrate the necessary changes into the main PR or submit the board modification as a separate, prerequisite PR. * **Testing logs before change are irrelevant.** Since this is a new feature, there shouldn't be any prior logs. Remove that section. * **"Data verification successful" is crucial, but needs more detail.** How is data verification performed? Byte-by-byte comparison? Checksum? Be explicit. * **More test cases are needed.** Test with different MTD configurations (different block sizes, different drivers, etc.) to ensure robustness. Testing on only one configuration is insufficient. Ideally, test on both a real target and a simulator if possible. **Example of Improved Description:** ## Summary This PR adds a new test application to `testing/fs` to benchmark the read/write performance of MTD devices. This is useful for characterizing flash performance, identifying bottlenecks, and verifying driver correctness. This allows developers to easily measure transfer rates on raw MTD devices without the overhead of a filesystem. This PR also modifies the `esp32s3_board_spiflash.c` file to [explain why this change is necessary, e.g., to expose the MTD device in a way accessible by the test application]. ## Impact * **User impact: YES.** Users can now benchmark MTD performance using the provided test application. The modification to `esp32s3_board_spiflash.c` [explain user impact, e.g., now exposes a new block device]. Users may need to enable the `TESTING_MTD_TRANSFER_RATE` Kconfig option to build the test. * **Build impact: YES.** The build system now includes the new `mtd_test` application. * **Hardware impact: NO.** * **Documentation impact: YES.** This PR adds documentation for the `mtd_test` application within the application's help output (accessible via `mtd_test -h`). * **Security impact: NO.** * **Compatibility impact: NO.** ## Testing * **Build Host:** Linux (Ubuntu 22.04), x86_64, GCC 11.2.0 * **Target:** esp32s3-devkit:spiflash, sim:qemu-esp32s3 **Testing on esp32s3-devkit:spiflash:** ``` nsh> mtd_test /dev/mtdblock0 ... (output as before, but include the -h output as well) ``` **Testing on sim:qemu-esp32s3:** ``` nsh> mtd_test /dev/mtd0 (or appropriate device) ... ``` Data verification is performed via a byte-by-byte comparison of the written and read data. Additional tests were performed with block sizes of [list sizes] and on [list other configurations]. By addressing these points, the PR description will be clearer, more complete, and easier for reviewers to assess. Remember to clearly justify each change and provide comprehensive testing results. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
