nuttxpr commented on PR #15564: URL: https://github.com/apache/nuttx/pull/15564#issuecomment-2592260110
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR, while functional, doesn't fully meet the NuttX requirements in terms of clarity and completeness. Here's a breakdown: **Strengths:** * **Clear problem statement:** The summary clearly explains the issue being fixed. * **Testing provided:** Logs are absent, but the testing environment is described. * **Impact description:** The impact on users and the system is adequately explained. * **Forward-thinking discussion:** The author raises valid points about code improvements, bitrate calculation, and driver structure. **Weaknesses:** * **Missing logs:** The "Testing" section lacks actual logs, making verification impossible for reviewers. * **Incomplete summary details:** The summary mentions corrected defines, but doesn't specify which ones. More detail on *how* the CAN FD format flag assignment works would be beneficial. * **Impact - Compatibility:** The impact on compatibility isn't addressed. Does this fix break anything for existing users? * **Impact - Documentation:** Is documentation impacted? If not explicitly stated, it should be stated as "NO". * **Impact - Security:** While unlikely, the security impact should be addressed, even if it's "NO". * **Discussion oversteps PR scope:** While valuable, the extensive discussion about future improvements and RTEMS belongs in a separate issue, not the PR. The PR should focus solely on the current changes. **Recommendations for Improvement:** 1. **Provide testing logs:** Include actual logs demonstrating the issue before the change and the corrected behavior after. 2. **Elaborate on the summary:** Specify the corrected defines and provide more detail on the CAN FD flag assignment implementation. 3. **Complete the Impact section:** Explicitly address compatibility, documentation, and security, even if the answer is "NO". 4. **Move future discussions to an issue:** Create a separate issue to discuss bitrate calculation, driver structure, and cross-platform compatibility. Link the PR to the issue. 5. **Clarify `nx_` prefix:** While mentioning the prefix is good, explicitly state whether this PR introduces it or if it's a question for future work. 6. **Focus on the immediate changes:** The PR description should primarily focus on the specific changes introduced by this contribution. Keep it concise and directly related to the code being modified. By addressing these points, the PR will better adhere to NuttX requirements and make the review process smoother. The discussion about broader improvements is valuable, but it should be separated to maintain focus and clarity within the PR itself. -- 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
