nuttxpr commented on PR #15825: URL: https://github.com/apache/nuttx/pull/15825#issuecomment-2654769480
[**\[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 ways: **Strengths:** * **Clear Summary:** The summary explains the change, its purpose, and how it works. * **Specific Impact Descriptions:** The impact section clearly identifies the areas affected and provides concise explanations. The "YES" answers are well-supported. * **Detailed Testing Information:** The testing section provides build instructions, expected output, and how to interpret the results. The reference to the ESP32-S3 TRM and the included diagram are excellent. **Areas for Improvement:** * **Summary - References:** While it mentions .bss, it might be helpful to link to relevant documentation about this memory section within NuttX or a general explanation for those unfamiliar. * **Testing - Before Change Logs:** The template requests "Testing logs before change." While the behavior before the change is implied (`.bss` would be in internal memory), explicitly showing the output of the `hello` example *before* the patch would make the comparison more compelling. This would demonstrate the difference in memory addresses and confirm the change is working as intended. * **Testing - Host Information:** The "Build Host(s)" section is missing. Include details about the development environment used for testing (OS, CPU architecture, compiler version). This helps with reproducibility and understanding the context of the test. * **Testing - Target Details:** While "esp32s3-devkit" is mentioned, specifying the exact configuration (e.g., the specific `defconfig` used) would be beneficial. Consider adding the output of `toplevel_config.h` or relevant sections to ensure the test setup is fully documented. * **Consider a Kconfig Option:** For a change like this, which affects memory allocation, a Kconfig option might be desirable. This would allow users to enable or disable placing .bss data in external PSRAM. If a Kconfig option is not included, explain the rationale for not providing one. **Revised Testing Section Example (Illustrative):** ``` ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Ubuntu 22.04, x86_64, GCC 11.3.0 * Target(s): esp32s3-devkit:nsh (using `esp32s3_defconfig`) Testing logs before change: ``` nsh> hello Hello, World!! data_bss: 0x3fc8a758 <-- Note: Address in internal RAM data_bss_ext: 0x3fc8a960 <-- Note: Also in internal RAM nsh> ``` Testing logs after change: ``` nsh> hello Hello, World!! data_bss: 0x3fc8a758 data_bss_ext: 0x3c050000 <-- Now in external PSRAM nsh> ``` (Include output of `cat build/esp32s3-devkit/nsh/toplevel_config.h | grep CONFIG_ESP32S3_SPIRAM` or similar to document the SPIRAM configuration) ``` By addressing these points, the PR will be even stronger and better aligned with NuttX's requirements. This will make it easier for reviewers to understand, verify, and ultimately merge the changes. -- 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
