nuttxpr commented on PR #16147: URL: https://github.com/apache/nuttx/pull/16147#issuecomment-2782008840
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR mostly meets the NuttX requirements, but could be improved. **Strengths:** * **Clear Summary:** The summary explains the *why*, *what*, and *how* of the change, referencing relevant external resources (Linux kernel implementation and CRC catalogues). The reasoning for changing the default is well-explained. Renaming the old functions to `crc16xmodem` is also clearly stated. * **Impact: ** Acknowledges dependency on a `nuttx-apps` PR. This is important for reviewers. * **Testing:** Indicates *how* testing was performed ("crc16 test via rpmsg uart"). **Weaknesses and Suggestions for Improvement:** * **Impact Section Incomplete:** The impact section needs to be fully filled out. While referencing the `nuttx-apps` PR is good, the impact on the core NuttX OS itself needs to be assessed directly in this PR. Consider the following: * **Is new feature added?**: Arguably, CRC-16/IBM is a new feature, even though another CRC existed before. * **Is existing feature changed?**: Yes, the default CRC is changed, and the old CRC functions are renamed. This is a significant change. * **Impact on user:** YES. Users relying on the default `crc16` behavior *will* need to adapt. They'll need to switch to `crc16xmodem` or explicitly configure CRC-16/IBM if that was their intended behavior. This needs to be clearly explained. * **Impact on build:** Probably NO. Clarify this. * **Impact on hardware:** Likely NO. Clarify this. * **Impact on documentation:** YES. Documentation needs to be updated to reflect the new default and the renamed functions. This PR should ideally include those documentation changes. * **Impact on security:** Potentially YES. If CRC16 was used for any security-sensitive operations (though unlikely), changing the default algorithm could have security implications. Consider this carefully and document the findings. * **Impact on compatibility:** YES. Backward compatibility is broken for code that implicitly relied on the previous default CRC. Clearly document the migration path. * **Missing Test Logs:** The "Testing logs before change" and "Testing logs after change" sections are empty. Include actual logs demonstrating the change in behavior. Show the output of the CRC calculations before and after, confirming the expected values. * **Consider a Kconfig Option:** Since this is a breaking change, consider adding a Kconfig option to allow users to select the default CRC algorithm at compile time. This would preserve backward compatibility for existing users while providing the new default. By addressing these weaknesses, the PR will be much stronger 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: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
