nuttxpr commented on PR #16175: URL: https://github.com/apache/nuttx/pull/16175#issuecomment-2793729476
[**\[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 it can be improved. Here's a breakdown and suggestions: **Strengths:** * **Clear Summary:** The summary explains the problem, the affected code, and the solution concisely. * **Impact is addressed:** All impact categories are considered and answered. * **Testing provided:** Testing instructions are clear, and the expected behavior before and after the patch is described. The inclusion of actual command-line examples is very helpful. **Weaknesses:** * **Missing Issue References:** If this fixes a reported bug, include links to the relevant NuttX or NuttX Apps issue. * **Incomplete Build Host Information:** The Testing section mentions target details, but not the build host details. Specify the OS, CPU architecture, and compiler used for testing. * **Testing Logs are Missing the Key Point:** The logs provided show *peripheral* behavior related to the bug, but they don't directly demonstrate the core issue: the inability to boot with `DEBUG_FEATURES` enabled *before* the patch, and successful boot *after* the patch. You need to show the boot output getting stuck/failing before the patch, and completing successfully after the patch. The current logs mainly show the terminal program's reaction to the board halting. * **Unnecessary Detail in Testing Logs:** The full `picocom` configuration output is not necessary. Focus on the relevant boot messages. Trim the logs to the essential parts demonstrating the fix. **Suggested Improvements:** 1. **Add Issue Reference:** If applicable, include "Fixes #[IssueNumber]" in the summary. 2. **Complete Build Host Information:** In the Testing section, add details like: * Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0 3. **Revised Testing Logs:** Capture the boot logs showing the failure before the patch. Ideally, these logs should show the boot process halting or displaying an error specifically related to the incorrect use of `riscv_lowputc` with `showprogress`. After the patch, the logs should show the boot completing successfully, reaching the NSH prompt. Example (hypothetical): **Before (Broken):** ``` Booting NuttX... Initializing... [Some output] showprogress: [output gets stuck here before applying patch] ``` **After (Fixed):** ``` Booting NuttX... Initializing... [Some output] showprogress: [progress messages displayed correctly] NuttShell (NSH) NuttX-10.4.0 nsh> ``` 4. **Concise Log Output:** Remove unnecessary parts from the `picocom` output. Focus only on the lines relevant to the boot process. By addressing these points, the PR will be much stronger and easier for reviewers to evaluate. Clear, concise, and focused information is key to a successful PR. -- 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
