nuttxpr commented on PR #15411: URL: https://github.com/apache/nuttx/pull/15411#issuecomment-2568262894
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR generally meets the NuttX requirements, but could be improved with some clarifications and additions. **Strengths:** * **Clear Summary:** The summary explains the "what" and "why" of the change, referencing the similar STM32G4 implementation. The listed differences are helpful. * **Specific Testing:** The testing description is relatively detailed, covering various modes and hardware configurations. The inclusion of a screenshot and log file is also good. **Areas for Improvement:** * **Impact - More Detail:** While the impact section mentions STM32H5, it needs to be more comprehensive. Address all the points explicitly: * **New Feature:** YES (FDCAN driver) * **User Impact:** Possibly YES. If users were using a different CAN driver or method, they might need to adapt. Specify any configuration changes needed. * **Build Impact:** Likely YES. Describe any Kconfig options added or modified. Mention the dependency on the STM32H5 architecture. * **Hardware Impact:** YES. Specifically state that this affects STM32H5 boards with FDCAN peripherals. Mentioning the Adafruit transceiver suggests a hardware dependency, but clarify if this is required or just an example. * **Documentation Impact:** YES. State that documentation updates are needed (and ideally included in the PR or a follow-up PR). This should include driver usage and configuration instructions. * **Security Impact:** Even if NO, explicitly state it and briefly justify why (e.g., "No security impact as this is a standard peripheral driver"). * **Compatibility Impact:** Likely NO if this is a new feature. If there are any backward compatibility concerns, address them. * **Testing - Before/After Logs:** The provided log file seems to be only *after* the change. Include logs from *before* the change to demonstrate the issue or lack of functionality that this PR addresses. Ideally, the logs should show specific test cases and their expected outcomes. Consider showing snippets of the logs within the PR description for key tests, rather than just linking a large file. * **Testing - Build Host:** Specify the build host details as requested (OS, CPU, compiler). * **Testing - Target:** Be more precise about the target. Instead of "arch(sim,RISC-V,ARM..), board:config", provide the specific board used (e.g., "STM32H5 Nucleo-H563ZI"). Since a follow-up PR is mentioned for board changes, link to that PR here. * **Queue Mode Changes:** The summary mentions changes for Queue Mode, but doesn't detail them. Briefly explain the nature of the changes and why they were necessary. By addressing these points, the PR will be more complete and easier for reviewers to assess. -- 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]
