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

fix: checking can id #40

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Conversation

ismetatabay
Copy link
Member

Description

At the Extended Can ID, the most significant bit determines the EFF flag (CAN_EFF_FLAG). For example, if our Extended CAN ID will be:
0x01F0A020 (00000001111100001010000000100000)
and if it's EFF Flag is 1, the value will be
0x81F0A020 (10000001111100001010000000100000).

Therefore, the CAN ID value is exceeds the 29 bit max value. (~536million - 0x1FBF'FFFFU). If we want to check can id value, we need to unmask it first. This PR fixes this problem.

@ismetatabay ismetatabay force-pushed the fix/can-id-check branch 2 times, most recently from 57d77b4 to 44c0676 Compare January 25, 2024 13:21
@ismetatabay
Copy link
Member Author

@JWhitleyWork can you check this fix?

@JWhitleyWork
Copy link
Collaborator

There are two ways to test for standard vs extended frames in SocketCAN: First mask the ID with the largest extended CAN ID and then check the value of the CAN ID and see if it is larger than the maximum standard frame ID (how it is done in the code now) or check the EFF bit returned in the unmasked ID. Both are valid so I don't think this change is necessary. Let me know if you feel differently.

@ismetatabay
Copy link
Member Author

@JWhitleyWork Thanks for the fast response, but I couldn't handle the following situation:

In case, my extended ID will be: 0x01F0A020, and if I give this ID to my can_msgs Frame message,

is_extended() function returns False:

CanId & CanId::extended() noexcept
{
m_id = m_id | EXTENDED_MASK;
return *this;
}

If I set the my can id:0x81F0A020 at can_msgs::Frame msg, is_extended() function returns true but I get this error since it exceeds the maximum limit:

const auto max_id = is_extended() ? MAX_EXTENDED : MAX_STANDARD;
if (max_id < id) {
throw std::domain_error{"CanId would be truncated!"};
}

At this point, what do you recommend me to do in this situation?

Thanks in advance @JWhitleyWork

@ismetatabay
Copy link
Member Author

@JWhitleyWork friendly reminder 😃

Signed-off-by: ismetatabay <[email protected]>
Copy link
Contributor

@xmfcx xmfcx left a comment

Choose a reason for hiding this comment

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

@JWhitleyWork you've mentioned that there are two valid ways to check for standard vs. extended frames:

  1. Masking the ID with the largest extended CAN ID and comparing the value: This is what's being done currently, but it doesn't account for the flag bits included in id.
  2. Checking the EFF bit in the unmasked ID: This is essentially what the proposed change is doing.

While both methods are valid, the current implementation doesn't handle cases where id includes flag bits, leading to potential errors.

Current proposal addresses the issue where including the EFF flag in the id causes incorrect behavior.
By masking out the flag bits before the comparison, we align the function's behavior with the expected CAN ID specifications.

Therefore, I'll approve the change to ensure correct handling of extended CAN IDs.

@xmfcx xmfcx merged commit d0a4422 into autowarefoundation:main Sep 23, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants