nuttxpr commented on PR #16143: URL: https://github.com/apache/nuttx/pull/16143#issuecomment-2781384628
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR does *not* fully meet the NuttX requirements. Here's why: * **Incomplete Summary:** While it states the *what* (adding CRC-16/ANSI), it lacks the *why*. Why is this CRC algorithm needed? What use case does it enable? How does it differ from existing CRC16 implementations in NuttX (if any)? The provided links offer some context, but a brief explanation directly in the summary is crucial. * **Missing Impact Assessment:** "N/A" is insufficient. Even seemingly small changes can have unintended consequences. Consider at least: * **Impact on user:** Will users need new configuration options or APIs? * **Impact on build:** Are there any new dependencies or build flags required? * **Impact on size:** Does this increase the code size? (Especially relevant for embedded systems) * **Insufficient Testing:** While it mentions testing with the Linux kernel, it doesn't provide any details about the *NuttX* testing environment. Which architecture, board, and configuration was used? The provided code snippets appear to be definitions, not logs. Actual *before* and *after* logs demonstrating the functionality and expected output are required. Crucially, how was it verified that the *new* CRC16 works correctly within the NuttX environment? **To improve this PR:** 1. **Expand the Summary:** Explain the motivation for adding CRC-16/ANSI. What specific need does it address within NuttX? 2. **Complete the Impact Assessment:** Address all the points, even if the answer is "NO" with a brief justification. For example: "Impact on build: NO, no new dependencies or build flags are introduced." Quantify the code size increase, if any. 3. **Provide detailed Testing information:** Specify the NuttX build host and target details (architecture, board, configuration). Include actual *before* and *after* logs showing the CRC calculations. Demonstrate how the expected results were verified. Ideally, include a simple test case within NuttX itself. By addressing these points, the PR will be significantly improved and 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
