nuttxpr commented on PR #15645: URL: https://github.com/apache/nuttx/pull/15645#issuecomment-2606465377
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements. Here's a breakdown: * **Summary:** While it explains the "what," it lacks a clear "why." Why is sim rpmsg uart support needed? What problem does it solve or feature does it enable? It mentions verification, but what exactly is being verified? What's the bigger picture use case? * **Impact:** "N/A" is insufficient. Even if there's *minimal* impact, it needs to be explicitly stated. For example: * **Impact on user:** NO (No user-facing changes) * **Impact on build:** YES (New files added to the build system, specifically `sim_rpmsg_port_uart.c`. Build instructions for the sim targets need to be updated.) * **Impact on hardware:** NO (Sim only) * **Impact on documentation:** YES (Documentation needs to be added explaining how to use the new sim rpmsg uart support) * **Impact on security:** NO (Assuming no security implications) * **Impact on compatibility:** NO (Assuming no compatibility issues) * **Testing:** The testing section is inadequate. * **Build Host(s):** Missing. Specify the OS, CPU architecture, and compiler used for building. * **Target(s):** While the `sim` target is mentioned, it's not clear which specific simulated architectures are being tested. Be explicit. For example: `qemu-x86_64`, `qemu-arm`. * **Logs:** The provided logs only show the processes running. They don't demonstrate that the rpmsg uart functionality is working. Show actual communication over the rpmsg uart. What data was sent? What data was received? Did it match expectations? Include error cases or boundary conditions tested. The logs should be *before* and *after* the change, demonstrating a clear difference in behavior. Currently, both logs are identical, seemingly from the *after* state. **How to improve this PR:** 1. **Clarify the "why":** Explain the purpose and benefit of this change. What use cases does it enable? 2. **Detail the impact:** Explicitly address each impact category, even if the answer is NO. Justify the answer. For YES responses, provide specifics. 3. **Provide comprehensive testing information:** * Specify the build host environment completely. * Specify the exact simulated targets tested. * Show logs that demonstrate the functionality working as expected, both before and after the change, highlighting the difference in behavior. Include tests that cover various scenarios and potential edge cases. 4. **Consider code comments and documentation:** Ensure the code is well-commented, explaining the logic and purpose of different sections. Update the relevant documentation to include instructions on how to use the new sim rpmsg uart support. By addressing these points, the PR will be much 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
