Socketcand ext ID bug fixes, and implement channel{_info} and Message attributes #1508
Conversation
| @@ -40,11 +45,15 @@ def convert_can_message_to_ascii_message(can_message: can.Message) -> str: | |||
| # Note: socketcan bus adds extended flag, remote_frame_flag & error_flag to id | |||
| # not sure if that is necessary here | |||
| can_id = can_message.arbitration_id | |||
| if can_message.is_extended_id or (can_id > 0x7FF): | |||
There was a problem hiding this comment.
I'm sorry but this check for (can_id > 0x7FF) is still wrong.
If you want to create robust code you might mask the can_id after the check for is_extended_id.
(masking ext ID with & 1FFFFFFF and a 11 bit ID with & 0x7FF)
There was a problem hiding this comment.
Absolutely correct .. fixed and force pushed.
There was a problem hiding this comment.
I added Message.is_rx attribute in the convert_ascii_message_to_can_message function, instead of after it is called.
socketcand uses the length of the arbitration id field to indicate whether a frame is using extended id or not. 3 characters for standard, 8 for extended. Prior to this fix, the arbitration id would be truncated to the lower 11 bits, or at times even garbage.
faisal-shah
force-pushed
the
socketcand-extid-channel-info
branch
from
c2553d5
to
7822232
Compare
faisal-shah
force-pushed
the
socketcand-extid-channel-info
branch
from
7822232
to
f43ac66
Compare
|
@zariiii9003 I'm definitely fine with 6ddbfa9 now. Can you please check (and commit?) the two more Python specific patches? Thx! |
| if can_message.is_extended_id: | ||
| can_id_string = f"{(can_id&0x1FFFFFFF):08X}" | ||
| else: | ||
| can_id_string = f"{(can_id&0x7FF):03X}" | ||
| # Note: seems like we cannot add CANFD_BRS (bitrate_switch) and CANFD_ESI (error_state_indicator) flags | ||
| data = can_message.data | ||
| length = can_message.dlc | ||
| bytes_string = " ".join(f"{x:x}" for x in data[0:length]) |
There was a problem hiding this comment.
@hartkopp Shouldn't the hex bytes here be zero-padded, e.g. 0c instead of c?
If so, the format string should be f"{x:02x}". (I know nothing about socketcand...)
There was a problem hiding this comment.
Doesn't f"{(can_id&0x1FFFFFFF):08X}" mean it is zero padded with a total length of 8 -> 00000123 ??
Because this is what would be needed here.
f"{x:02x}" would be the right approach for data bytes (not CAN identifier), right?
There was a problem hiding this comment.
I'm talking about the data bytes here:
bytes_string = " ".join(f"{x:x}" for x in data[0:length])For example, should it be 2 4 6 8 a or 02 04 06 08 0a? Does it make a difference for socketcand? Or is it okay as long as there is spacing between the bytes?
There was a problem hiding this comment.
For < send >, it's supposed to be lowercase unpadded (%hhx) with spaces in-between data bytes:
https://github.com/linux-can/socketcand/blob/master/state_raw.c#L158-L171
There was a problem hiding this comment.
And on the receive side (< frame > message), the data is padded uppercase (%02X) without spaces in between:
https://github.com/linux-can/socketcand/blob/master/state_raw.c#L125-L135
There was a problem hiding this comment.
Ouch! I wasn't aware of this inconsistency.
But I assume changing this in socketcand is not the way to go as it would potentially break other applications.
There was a problem hiding this comment.
Alright, then i will merge this.
There was a problem hiding this comment.
Ouch! I wasn't aware of this inconsistency.
But I assume changing this in socketcand is not the way to go as it would potentially break other applications.
Sent PR to correct the docs in socketcand:
See #1504 for bug description
This pull request combines:
#1505
#1503