Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

com/can: Remove unnecessary judgment logic #14713

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HedongGao
Copy link
Contributor

@HedongGao HedongGao commented Nov 10, 2024

Summary

The feature of preventing the same ID frame rotation should be done in the lowerhalf or hardware, otherwise it may cause some problems. The CAN protocol stack only ensures that message is written into the hardware in the order of application layer packets, and does not care hardware sending order. This patch just remove this feature from CAN stack.

Impact

When the application layer sends frames faster than Hardware and the can_id of frames is the same, the CAN protocol stack will directly hand over frames to Hardware and no longer prevent it from being sent.

Testing

The application layer sends CAN frames with same can_id, constructs a scenario where the sending queue is full, and manually adds log information to record the frame in sending queue. Print the frame.can_id information for each frame as follows:
pending_list lentgth is :3 , and tx mailbox count is : 3
frame in tx mailbox is : 0x193, 0x193, 0x193

@github-actions github-actions bot added Area: Drivers Drivers issues Size: S The size of the change in this PR is small labels Nov 10, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 10, 2024

[Experimental Bot, please feedback here]

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.

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include in the commit log the remaining explanation you put in the Summary

@HedongGao
Copy link
Contributor Author

Please include in the commit log the remaining explanation you put in the Summary

DOne

@acassis
Copy link
Contributor

acassis commented Nov 11, 2024

@HedongGao I mean, add:

The CAN protocol stack only ensures that message is written into the hardware in the order of application layer packets, and does not care hardware sending order. This patch just remove this feature from CAN stack.

To the commit log message

@HedongGao
Copy link
Contributor Author

HedongGao commented Nov 12, 2024

Please include in the commit log the remaining explanation you put in the Summary

I am sorry!
Is it OK to make this modification?

The feature of preventing the same ID frame rotation should be done in the lowerhalf, not in the upperhalf.

Signed-off-by: gaohedong <gaohedong@xiaomi.com>
@acassis
Copy link
Contributor

acassis commented Nov 22, 2024

Please include in the commit log the remaining explanation you put in the Summary

I am sorry! Is it OK to make this modification?

Yes, you can just git commit --amend and update the commit message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants