nuttxpr commented on PR #16182: URL: https://github.com/apache/nuttx/pull/16182#issuecomment-2795807217
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although some clarifications and additions would strengthen it. **Strengths:** * The summary clearly explains the change, its purpose, and how it works. * The impact section addresses key areas, highlighting that the change is primarily about adding a build option and shouldn't affect existing users. * Testing information provides the host and target details. **Areas for Improvement:** * **Impact - Build:** While it mentions affecting the build process, be more explicit. Does this change require any new dependencies for CMake users? Does it change the output directory structure? * **Impact - Documentation:** If the CMake build process is different from the standard make build, documentation updates might be needed. Even a brief mention in the `boards/arm/tiva/tm4c123g-launchpad` README would be helpful. State explicitly "NO" or "YES" and explain if "YES". * **Testing - Logs:** The testing section lacks concrete logs. While you describe the *steps*, you don't show any output. Include relevant snippets of the build output (both successful and, if applicable, any error messages encountered during development). Show output from the target device demonstrating that NuttX boots and functions correctly after being built with CMake. Consider a simple "hello world" or the output of `nsh`. * **Testing - CMake Command:** The CMake command provided is vague ("likely similar to"). Provide the *exact* command used. This is crucial for reproducibility. * **Testing - Flashing Process:** You mention flashing, but not *how*. Provide the commands or steps you used to flash the built image to the `tm4c123g-launchpad`. * **Explicitly answer all Impact questions:** Even if the answer is "NO", state it explicitly for each item (Hardware, Security, Compatibility, etc.). This makes the review process easier. **Example additions to the Testing section:** ``` Testing logs before change (using make): [Relevant output from the make build process and output on the target device] Testing logs after change (using CMake): cmake -B build -DBOARD_CONFIG=tm4c123g-launchpad:nsh -GNinja ninja -C build [Relevant output from the CMake/Ninja build process] Flashing command: openocd -f interface/cmsis-dap.cfg -f target/tm4c123g.cfg -c "program build/tm4c123g-launchpad/nuttx.bin verify reset exit" Target output after flashing (CMake build): NuttShell (NSH) nsh> help [Output of the help command demonstrating basic functionality] ``` By addressing these points, the PR will be much stronger and easier for reviewers to assess. -- 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
