nuttxpr commented on PR #15570: URL: https://github.com/apache/nuttx/pull/15570#issuecomment-2593778904
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary. Yes, this PR appears to mostly meet the NuttX requirements, but is lacking some crucial information. Here's a breakdown of what's missing and how to improve it: * **Summary:** While the summary explains the *why*, it lacks sufficient detail on the *what* and *how*. Specifically: * **What functional part is changed?** Clearly state that the change modifies the CRC calculation within the MS5611 driver. * **How exactly does the change work?** Don't just say it's different. Show the code difference. Explain *why* the manufacturer's code is different and why it's believed to be correct. A link to the manufacturer's documentation or code would be very helpful. * **Impact:** The provided impact information is too brief. While it mentions the positive impact, it needs more detail and to address the other points: * **Is new feature added? Is existing feature changed?** This is a bug fix to an existing feature. * **Impact on user:** Yes, users with MS5611 sensors will now have them work correctly. Be more specific if possible (e.g., "Users who previously experienced failure to register MS5611 sensors will now be able to use them"). * **Impact on build, hardware, documentation, security, compatibility:** Explicitly state "NO" for each of these if there's truly no impact. Justify any "YES" answers. For example, if the CRC algorithm changes, there might be a backward compatibility impact if existing applications rely on the old, incorrect CRC values. * **Testing:** The testing information is insufficient. * **Build Host(s):** Provide details about the development environment used (OS, CPU architecture, compiler version). * **Target(s):** Be specific about the target architecture and board configuration used for testing. "sim,RISC-V,ARM" is too vague. * **Testing logs:** The PR *must* include actual log output before and after the change. Simply saying "failing CRC in the debug logs" isn't enough. Show the logs! This is crucial for reviewers to understand the problem and verify the fix. **In short, to make this PR acceptable, the author needs to:** 1. **Elaborate on the technical details of the change** in the Summary section, including the code difference and rationale. 2. **Provide a complete impact assessment,** explicitly addressing all points, even if the answer is "NO." 3. **Include detailed testing information,** including build host details, target details, and *actual* log output before and after the change. This is the most critical missing piece. By addressing these points, the PR will be much more likely to be accepted. -- 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