nuttxpr commented on PR #15786: URL: https://github.com/apache/nuttx/pull/15786#issuecomment-2643457512
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary. Yes, this PR appears to meet the NuttX requirements, although the provided information could be more thorough. Here's a breakdown of why and where it could be improved: **Strengths:** * **Clear Summary:** The summary explains the "why" (missing features/fixes), "what" (pin maps, buffers, IRQ definitions, baud calculation fix), and "how" (adding definitions, correcting names/functions). * **Impact Description:** It identifies the affected component and the overall improvement (support for all serial devices). * **Testing Description:** Provides basic testing steps and a confirmation of successful build. **Weaknesses and Suggestions for Improvement:** * **Missing Issue References:** Are there any related NuttX issues this PR addresses? If so, they should be linked. * **Impact Detail:** While the impact is mentioned, it lacks specifics. * **User Impact:** Will users need to change configurations or code to use the new UARTs/USARTs? Be explicit. Even if the answer is "NO", state it clearly. * **Build Impact:** While a successful build is mentioned, are there any new dependencies or configuration options introduced? * **Hardware Impact:** Specifies STM32H5, but which specific boards were tested? Listing them explicitly is helpful. Also, were *only* those boards tested? * **Documentation Impact:** Does this change require documentation updates? If so, were they included in the PR? * **Security Impact:** While likely "NO", explicitly stating this is good practice. * **Compatibility Impact:** Is this backwards compatible? Explicitly stating "NO" or "YES" is important. * **Testing Detail:** The testing is too high-level. * **Build Host(s):** Provide details about the build host environment (OS, CPU architecture, compiler version). * **Target(s):** Be specific about the target hardware (e.g., "STM32H5 Nucleo-H563ZI"). "sim" is insufficient; if a simulator was used, specify which one. * **Logs:** "Check for errors" is vague. Provide actual build logs (or snippets of relevant sections) *before* and *after* the change. Showing that the build failed before and succeeded after is crucial. Ideally, also include logs demonstrating the functionality of the newly added UARTs/USARTs (e.g., loopback tests). **Example Improvements for Testing Section:** ``` ## 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): STM32H5 Nucleo-H563ZI (nsh config) Testing logs before change: ``` ... (snippet showing build failure related to missing definitions/incorrect IRQs) ... ``` Testing logs after change: ``` ... (snippet showing successful build) ... ``` Functional Test (Loopback on USART6): ``` nsh> uart6_loopback_test // (Hypothetical test command) ... (output showing successful loopback communication) ... ``` ``` By providing more specific information, especially regarding testing and impact, you'll make it much easier for reviewers to assess and approve your 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
