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

Reply via email to