nuttxpr commented on PR #15827: URL: https://github.com/apache/nuttx/pull/15827#issuecomment-2655252149
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR mostly meets the NuttX requirements, but is lacking in key areas, making it difficult to review properly. Here's a breakdown: **Strengths:** * **Clear Summary of Functionality:** The summary explains the core changes (adding ivshmem support, including client and server components) and provides a helpful link to QEMU documentation. * **Detailed Testing Steps:** The testing section provides clear, step-by-step instructions for reproducing the functionality. This is a significant plus. * **Example Logs:** Including example logs, though limited, gives reviewers a better idea of expected output and helps with verification. **Weaknesses:** * **Impact Section is Inadequate:** "N/A" is not sufficient. Even if the *user* impact is minimal, there are clear impacts on the codebase (new features, hardware dependencies). This section needs to be filled out thoroughly. Consider: * **Is new feature added?** YES - ivshmem support * **Impact on hardware?** YES - Requires QEMU with ivshmem support, specific configuration. Specify the architectures affected (armv8a). * **Impact on documentation?** Potentially YES - If no documentation exists for this feature, it should be added. If this PR *doesn't* include documentation, explain why (e.g., planned for a separate PR). * **Impact on compatibility:** Possibly YES - Does this affect existing OpenAMP implementations? Is it backwards compatible? * **Missing "Before Change" Logs:** The testing section requests "Testing logs before change," but none are provided. This makes it impossible to assess the impact of the changes. * **Limited Scope of Testing Logs:** The logs primarily focus on successful communication. More testing is needed, including: * **Error Handling:** What happens if the shared memory setup fails? If the connection drops? * **Edge Cases:** Test with different message sizes, large data transfers, stress testing. * **Different Configurations:** Test with variations in QEMU parameters. **Recommendations for Improvement:** 1. **Expand the Impact Section:** Address *all* the points in the template, even if the answer is "NO." Justify "NO" answers briefly. Provide detail for "YES" answers. 2. **Add "Before Change" Logs:** Capture logs from the system *without* the ivshmem changes to demonstrate the difference in behavior. 3. **Broaden Testing and Include More Logs:** Test error conditions, edge cases, and different configurations. Include logs for these tests. 4. **Consider Documentation:** If this feature warrants documentation (and it likely does), either include it in this PR or explain why it will be handled separately. By addressing these weaknesses, the PR will be much stronger and easier to review, increasing its chances of being 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
