nuttxpr commented on PR #15799: URL: https://github.com/apache/nuttx/pull/15799#issuecomment-2646964772
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR does **not** fully meet the NuttX requirements, although it provides a good starting point. Here's why: * **Summary:** While the summaries for each commit are present, they lack crucial context for a holistic understanding of the PR. The overall purpose of adding ivshmem support is mentioned, but the *why* is missing. Is this for a specific use case? Does it address a performance bottleneck? The summaries also don't mention which functional part of the code is changed beyond `arm64/qemu-armv8a`. More detail is needed. * **Impact:** Marking this as N/A is incorrect. This PR adds a new feature (ivshmem support) and makes changes to the device tree and FDT handling. Therefore, there *is* an impact. At a minimum, the following need to be addressed: * **Impact on user:** YES. Users now have the option to use ivshmem. How should they configure it? Are there any limitations? * **Impact on hardware:** YES. This is specific to QEMU's arm64 architecture and requires ivshmem device emulation. This needs to be explicitly stated. * **Impact on build:** Potentially YES. The changes to FDT handling and the build fix suggest possible build process changes, at least for the `qemu-armv8a` configuration. This needs investigation and documentation. * **Impact on documentation:** YES. New features require documentation. Where is this documentation? Is it part of this PR or a separate one? * **Testing:** The provided testing information is insufficient. * **Build Host(s):** Missing. Specify the OS, CPU architecture, and compiler version used for building. * **Target(s):** While `qemu-armv8a` is mentioned, the specific QEMU version and configuration details (e.g., machine type, memory size) are absent. * **Logs:** The "before" and "after" logs are empty. Provide actual log output demonstrating the functionality before and after the changes. Ideally, the logs should showcase the ivshmem example working correctly. Simply showing build commands is not testing. Show the output of running the `rpserver_ivshmem` and `rpproxy_ivshmem` examples and how they interact. **To improve this PR:** 1. **Expand the overall summary:** Explain the motivation for adding ivshmem support. What problem does it solve? What are the benefits? 2. **Detail the impact:** Address all impact categories honestly. Don't dismiss them as N/A when changes are clearly present. Provide specific details. 3. **Provide comprehensive testing information:** Specify the build host environment, the target QEMU configuration, and include relevant log output demonstrating the functionality before and after the changes. Include performance metrics if relevant. 4. **Consider code style and comments:** Ensure the code adheres to NuttX coding style guidelines and is well-commented. 5. **Document the new feature:** Include documentation explaining how to use the new ivshmem feature, its configuration options, and any limitations. By addressing these points, the PR will be much stronger 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
