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.

Sign up for GitHub

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

Jump to bottom

Socketcand ext ID bug fixes, and implement channel{_info} and Message attributes #1508

Merged
merged 3 commits into from
Jan 28, 2023
Merged

Socketcand ext ID bug fixes, and implement channel{_info} and Message attributes #1508

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ddbfa9
Handle extended arbitration id properly
faisal-shah Jan 26, 2023
93beec2
Add is_rx attribute
faisal-shah Jan 27, 2023
f43ac66
Populate channel{_info} and Message attributes
faisal-shah Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 15 additions & 2 deletions can/interfaces/socketcand/socketcand.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,17 @@ def convert_ascii_message_to_can_message(ascii_msg: str) -> can.Message:
frame_string = ascii_msg[8:-2]
parts = frame_string.split(" ", 3)
can_id, timestamp = int(parts[0], 16), float(parts[1])
is_ext = len(parts[0]) != 3

data = bytearray.fromhex(parts[2])
can_dlc = len(data)
can_message = can.Message(
timestamp=timestamp, arbitration_id=can_id, data=data, dlc=can_dlc
timestamp=timestamp,
arbitration_id=can_id,
data=data,
dlc=can_dlc,
is_extended_id=is_ext,
is_rx=True,
)
return can_message

Expand All @@ -40,11 +46,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:
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])
Copy link
Collaborator

Choose a reason for hiding this comment

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

@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...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, then i will merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

linux-can/socketcand#27

return f"< send {can_id:X} {length:X} {bytes_string} >"
return f"< send {can_id_string} {length:X} {bytes_string} >"


def connect_to_server(s, host, port):
Expand All @@ -70,6 +80,8 @@ def __init__(self, channel, host, port, can_filters=None, **kwargs):
self.__socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
self.__message_buffer = deque()
self.__receive_buffer = "" # i know string is not the most efficient here
self.channel = channel
self.channel_info = f"socketcand on {channel}@{host}:{port}"
connect_to_server(self.__socket, self.__host, self.__port)
self._expect_msg("< hi >")

Expand Down Expand Up @@ -139,6 +151,7 @@ def _recv_internal(self, timeout):
if parsed_can_message is None:
log.warning(f"Invalid Frame: {single_message}")
else:
parsed_can_message.channel = self.channel
self.__message_buffer.append(parsed_can_message)
buffer_view = buffer_view[end + 1 :]

Expand Down