nuttxpr commented on PR #16086: URL: https://github.com/apache/nuttx/pull/16086#issuecomment-2758247703
[**\[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 sections could be more detailed. Here's a breakdown: **Strengths:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the change. * **Specific Impact:** The impact section identifies the affected area (serial read) and describes the change's logic. * **Testing Provided:** Testing on both a simulator and physical hardware is good practice. The code example demonstrating the issue and fix is helpful. **Weaknesses & Suggestions for Improvement:** * **Missing Issue References:** If this PR addresses a specific NuttX issue, link it. If not, consider creating one to track the bug. * **Impact Details Lacking:** While the impact section identifies areas, it lacks detail. Specifically: * **Impact on User:** Even if the answer is "NO," explain briefly *why* there's no user impact. Example: "NO. This change is internal to the serial driver and doesn't affect user API or behavior." * **Other Impact Sections:** All "NO" answers should have brief justifications. For example, "Impact on Build: NO. No build system changes are required." * **Testing Logs Missing:** The template requests actual log output before and after the change. This provides concrete evidence of the fix. Include relevant snippets, even if short. * **Target Details:** Be more specific about your target. "raspberrypi-pico-2 board" is good, but add the configuration (e.g., `raspberrypi-pico-2:nsh`). For `sim`, specify which simulator was used. * **Clarity of Test Case:** The code example helps, but adding a step-by-step description of how to reproduce the issue (and verify the fix) would be even better. For example, "Connect to the serial console, send the string 'AB', and observe..." **Example of improved Testing Section:** ``` ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): macOS Ventura, Intel x86_64, GCC 12.2 * Target(s): sim:nsh, raspberrypi-pico-2:nsh To reproduce the issue before the fix: 1. Connect to the serial console of the target. 2. Send the string "AB". 3. Observe that the `read()` call in the provided code example only returns 1 byte, even though 2 are available. Testing logs before change (sim:nsh): ``` got 1 bytes: A ``` Testing logs after change (sim:nsh): ``` got 2 bytes: AB ``` Testing logs before change (raspberrypi-pico-2:nsh): ``` got 1 bytes: A ``` Testing logs after change (raspberrypi-pico-2:nsh): ``` got 2 bytes: AB ``` ``` By addressing these weaknesses, the PR will be clearer, easier to review, and more likely to be accepted. -- 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
