nuttxpr commented on PR #15858: URL: https://github.com/apache/nuttx/pull/15858#issuecomment-2665301955
[**\[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 more explicitly filled out. Here's a breakdown of why and suggestions for improvement: **Strengths:** * **Clear Summary of the "Why":** The summary explains the issue and its root cause (incorrect interrupt completion handling due to PLIC behavior). * **Specific Functional Change:** It identifies the affected code (MPFS interrupt handling). * **How it Works:** Describes the change in behavior (acknowledging completion before disable). * **Impact Description:** Explains the user impact (fixes broken interrupt handling for MPFS I2C) and clearly states no other impact is expected. * **Testing Provided:** Includes test results demonstrating the fix. **Areas for Improvement (to make the PR even stronger):** * **Missing Issue References:** If a related NuttX or NuttX Apps issue exists, link it. Even if there isn't one, stating "No related issue" is helpful. * **More Complete Impact Assessment:** While the impact section is started well, explicitly state "NO" for all other impact categories (build, hardware, documentation, security, compatibility) to avoid ambiguity. Even if there's no impact, stating it explicitly removes any doubt for the reviewer. * **Detailed Testing Information:** The testing section should be more comprehensive. * **Build Host Details:** Specify OS, CPU architecture, compiler version. Example: "Linux, x86_64, GCC 12.1.0" * **Target Details:** Be more specific. Example: "sim:qemu-rv32" or "polarfire_soc:i2c_test_config". * **More Descriptive Logs:** Instead of just saying "everything works," show relevant log snippets that demonstrate the interrupt behavior before and after the fix. For example, logs showing I2C transactions completing successfully after the patch, where they previously failed. **Example of Improved Testing Section:** ``` Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux, x86_64, GCC 12.1.0 * Target(s): polarfire_soc:i2c_test_config Testing logs before change: ``` [I2C] Transfer started [I2C] Interrupt received [I2C] Transfer started [I2C] Interrupt received [I2C] Transfer started [I2C] Transfer timed out <-- Interrupts stopped working ``` Testing logs after change: ``` [I2C] Transfer started [I2C] Interrupt received [I2C] Transfer started [I2C] Interrupt received [I2C] Transfer started [I2C] Interrupt received [I2C] Transfer complete <-- Interrupts continue to function ``` By adding these small improvements, the PR will be even clearer and easier for maintainers to review and merge. -- 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
