nuttxpr commented on PR #14713: URL: https://github.com/apache/nuttx/pull/14713#issuecomment-2466629273
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements as described. While it provides some information, it lacks crucial details and context. Here's a breakdown of what's missing and how to improve it: * **Summary:** * **Missing:** Why is removing this feature necessary? What specific problems does it cause? Simply stating it "may cause some problems" is insufficient. Provide concrete examples of the problems. Explain what part of the CAN stack code is being modified. * **Improvement:** Clearly articulate the rationale behind the change. For instance: "Removing the same-ID frame rotation feature from the CAN stack because it introduces unnecessary complexity and can lead to unexpected behavior in applications that rely on strict message ordering. This feature should be handled at the lower half or hardware level for proper implementation. This change modifies the `send()` function within the CAN driver and the `can_tx()` function in the CAN network stack." Also, provide links to relevant NuttX issues if applicable. * **Impact:** * **Missing:** This section is essentially empty. You *must* address each point. * **Improvement:** Fill out *every* item. Even if the impact is "NO," explicitly state it. For example: * Impact on user: YES. Users relying on the (now removed) same-ID frame rotation feature will need to implement it in their application or at the lower half/hardware level. * Impact on build: NO. * Impact on hardware: Potentially YES. If hardware provided this feature, it may now be unused. * Impact on documentation: YES. The documentation should be updated to reflect the removal of this feature and guide users on alternative implementations. * Impact on security: Potentially YES. If the removed feature had security implications, explain how the change affects security. * Impact on compatibility: Potentially YES. Explain if this change breaks compatibility with any existing applications or hardware. * **Testing:** * **Insufficient:** The provided testing logs are minimal. They show some evidence of testing but lack detail. What platform was this tested on? What hardware? What is "0x193"? Is it the CAN ID? What were the expected results vs. actual results? * **Improvement:** Provide more comprehensive logs. Include: * **Build Host:** e.g., "Linux Ubuntu 22.04, x86_64, GCC 11.3.0" * **Target:** e.g., "SIM:qemu-arm, STM32F4DISCOVERY:nsh" * **Clearer logs:** Instead of just "frame in tx mailbox is : 0x193," explain what this means. "Sent three CAN frames with ID 0x193. Verified that they were transmitted in the expected order without rotation." Show logs *before* the change demonstrating the previous behavior and logs *after* demonstrating the new behavior. Quantify the results if possible (e.g., "Latency improved by 10%"). In short, the current PR description is incomplete and doesn't provide enough information for a reviewer to assess the change thoroughly. Addressing the points above will significantly improve the quality of the PR and increase its chances of being 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]
