nuttxpr commented on PR #15800: URL: https://github.com/apache/nuttx/pull/15800#issuecomment-2647395852
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although some sections could be slightly improved. Here's a breakdown: **Strengths:** * **Clear Summary:** The summary provides a good overview of the changes and the motivation behind them. The step-by-step explanation of the implementation is helpful. The links to the custom QEMU build are essential. * **Impact Section:** While concise, it addresses the key impact areas. The statement about not affecting other targets is important. * **Testing Section:** Specifies the host (implicitly through the prebuilt binary reference) and the target configurations tested. Mentioning CI checks is good. **Areas for Improvement:** * **Summary:** While the related issue/PR references aren't explicitly stated, they might not be applicable if this is a new feature. However, it's good practice to confirm this explicitly (e.g., "No related issues/PRs as this is a new feature"). * **Impact Section:** While the impact is minimized, specifying "NO" for each category explicitly would enhance clarity. For example: * Impact on user: YES (Users can now run NuttX in protected mode on QEMU Cortex-R5) * Impact on build: YES (New configuration `qemu-armv7r:pnsh` added) * Impact on hardware: NO (Changes are specific to the QEMU emulated environment) * Impact on documentation: YES (Documentation should be updated to describe the new configuration and how to use it) - This is particularly important. * Impact on security: Potentially YES (Running in protected mode has security implications that should be documented). Even if the implications are inherent to protected mode itself and not specific to this implementation, it's worth mentioning. * Impact on compatibility: NO (Should not affect existing configurations) * **Testing Section:** The testing logs are missing. While they might be extensive, including snippets of relevant output demonstrating the key changes (e.g., boot log showing protected mode, output from a simple test application) would significantly strengthen the PR. Be sure to include the host details (OS, Compiler version) even if it seems obvious from the context. By adding these minor improvements, the PR would be even stronger and demonstrate thoroughness in addressing the NuttX requirements. -- 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
