nuttxpr commented on PR #14770: URL: https://github.com/apache/nuttx/pull/14770#issuecomment-2475303953
[**\[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. While it provides some information, it lacks crucial details. Here's a breakdown: * **Insufficient Summary:** "Add arm64 support" is too vague. Which specific aspect of arm64 is being supported? Is this for a new board? A new driver? The summary needs to be more descriptive about the *functional change*. Similarly, "Fixed the merging issue that removed 64bit register parser" lacks context. Which merge? What was the original intent of the 64bit register parser? How does parsing from the ELF file improve robustness? * **Missing Impact Details:** While the PR mentions added features and reduced error proneness, it doesn't adhere to the structured format requested. It needs to specifically address each impact point (user, build, hardware, documentation, security, compatibility) with a YES/NO and a description if YES. For example: * **Impact on hardware:** YES. Adds support for arm64 architecture. This will enable NuttX to run on arm64 hardware platforms. * **Impact on documentation:** YES. Documentation needs to be updated to reflect the new arm64 support, including build instructions, supported boards, and any arm64-specific configurations. * etc. * **Incomplete Testing Information:** The provided test command is helpful, but insufficient. * **Build Host:** Missing. What operating system, CPU, and compiler version were used for building NuttX and the tools? * **Target:** While `qemu` and `arm64` are mentioned, the specific QEMU configuration and the NuttX configuration used within QEMU aren't specified. (e.g., `qemu-system-aarch64 -M virt -cpu cortex-a53 -kernel nuttx.bin -nographic`). * **Logs Before/After:** The PR says it was "tested," but doesn't provide the actual *output* of the tests before and after the changes. Showing the output is crucial for reviewers to understand the change's effect. Ideally, it should demonstrate the previous failure (e.g., inability to parse registers correctly) and the successful behavior after the change. To summarize, the PR needs more detail and structure to meet the requirements. It needs to be more specific about the changes, their impact, and provide concrete evidence of successful testing. Following the template more closely will significantly improve the review process. -- 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]
