Skip to content

Support for network ID above 255 #1627

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

Merged
merged 1 commit into from
Jul 6, 2023
Merged

Support for network ID above 255 #1627

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 24 additions & 31 deletions can/interfaces/ics_neovi/neovi_bus.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import os
import tempfile
from collections import Counter, defaultdict, deque
from functools import partial
from itertools import cycle
from threading import Event
from warnings import warn
Expand Down Expand Up @@ -317,7 +318,8 @@ def _process_msg_queue(self, timeout=0.1):
except ics.RuntimeError:
return
for ics_msg in messages:
if ics_msg.NetworkID not in self.channels:
channel = ics_msg.NetworkID | (ics_msg.NetworkID2 << 8)
if channel not in self.channels:
continue

is_tx = bool(ics_msg.StatusBitField & ics.SPY_STATUS_TX_MSG)
Expand Down Expand Up @@ -364,50 +366,37 @@ def _get_timestamp_for_msg(self, ics_msg):
def _ics_msg_to_message(self, ics_msg):
is_fd = ics_msg.Protocol == ics.SPY_PROTOCOL_CANFD

message_from_ics = partial(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer this without partial. I see you want to avoid repetition, but it would be more readable if we just extract all parameters and use a single return Message(...) statement instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would mean having a dictionary with the arguments, what's the difference with using partial, or what do you dislike about partial?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't mean a dictionary. I meant something simple like

timestamp = self._get_timestamp_for_msg(ics_msg)
arbitration_id = ics_msg.ArbIDOrHeader
 is_extended_id = bool(ics_msg.StatusBitField & ics.SPY_STATUS_XTD_FRAME)
is_remote_frame = bool(ics_msg.StatusBitField & ics.SPY_STATUS_REMOTE_FRAME)
is_error_frame = bool(ics_msg.StatusBitField2 & ics.SPY_STATUS2_ERROR_FRAME)
channel = ics_msg.NetworkID | (ics_msg.NetworkID2 << 8)
dlc = ics_msg.NumberBytesData
is_fd = is_fd
is_rx = not bool(ics_msg.StatusBitField & ics.SPY_STATUS_TX_MSG)

if is_fd:
    if ics_msg.ExtraDataPtrEnabled:
        data = ics_msg.ExtraDataPtr[: ics_msg.NumberBytesData]
    else:
        data = ics_msg.Data[: ics_msg.NumberBytesData]

    error_state_indicator = bool(ics_msg.StatusBitField3 & ics.SPY_STATUS3_CANFD_ESI)
    bitrate_switch = bool(ics_msg.StatusBitField3 & ics.SPY_STATUS3_CANFD_BRS)
else:
    data = ics_msg.Data[: ics_msg.NumberBytesData]   
    error_state_indicator = False
    bitrate_switch = False

return Message(
    timestamp=timestamp,
    arbitration_id=arbitration_id,
...
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This means doing some assumption on some of the default keyword argument values.

In you example, you assume that the default value for error_state_indicator is False. What if the Message class changes and now the default is None.

This was one of the reason to go with partial.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Creating a partial instance in every recv() call might be inefficient and slow. But i didn't measure it of course.
You could argue that it's only too slow once somebody complains 😄

That said, i understand your reasoning. Feel free to merge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looking at the CPython partial implementation, I believe this overhead will be negligible.

Message,
timestamp=self._get_timestamp_for_msg(ics_msg),
arbitration_id=ics_msg.ArbIDOrHeader,
is_extended_id=bool(ics_msg.StatusBitField & ics.SPY_STATUS_XTD_FRAME),
is_remote_frame=bool(ics_msg.StatusBitField & ics.SPY_STATUS_REMOTE_FRAME),
is_error_frame=bool(ics_msg.StatusBitField2 & ics.SPY_STATUS2_ERROR_FRAME),
channel=ics_msg.NetworkID | (ics_msg.NetworkID2 << 8),
dlc=ics_msg.NumberBytesData,
is_fd=is_fd,
is_rx=not bool(ics_msg.StatusBitField & ics.SPY_STATUS_TX_MSG),
)

if is_fd:
if ics_msg.ExtraDataPtrEnabled:
data = ics_msg.ExtraDataPtr[: ics_msg.NumberBytesData]
else:
data = ics_msg.Data[: ics_msg.NumberBytesData]

return Message(
timestamp=self._get_timestamp_for_msg(ics_msg),
arbitration_id=ics_msg.ArbIDOrHeader,
return message_from_ics(
data=data,
dlc=ics_msg.NumberBytesData,
is_extended_id=bool(ics_msg.StatusBitField & ics.SPY_STATUS_XTD_FRAME),
is_fd=is_fd,
is_rx=not bool(ics_msg.StatusBitField & ics.SPY_STATUS_TX_MSG),
is_remote_frame=bool(
ics_msg.StatusBitField & ics.SPY_STATUS_REMOTE_FRAME
),
is_error_frame=bool(
ics_msg.StatusBitField2 & ics.SPY_STATUS2_ERROR_FRAME
),
error_state_indicator=bool(
ics_msg.StatusBitField3 & ics.SPY_STATUS3_CANFD_ESI
),
bitrate_switch=bool(
ics_msg.StatusBitField3 & ics.SPY_STATUS3_CANFD_BRS
),
channel=ics_msg.NetworkID,
)
else:
return Message(
timestamp=self._get_timestamp_for_msg(ics_msg),
arbitration_id=ics_msg.ArbIDOrHeader,
return message_from_ics(
data=ics_msg.Data[: ics_msg.NumberBytesData],
dlc=ics_msg.NumberBytesData,
is_extended_id=bool(ics_msg.StatusBitField & ics.SPY_STATUS_XTD_FRAME),
is_fd=is_fd,
is_rx=not bool(ics_msg.StatusBitField & ics.SPY_STATUS_TX_MSG),
is_remote_frame=bool(
ics_msg.StatusBitField & ics.SPY_STATUS_REMOTE_FRAME
),
is_error_frame=bool(
ics_msg.StatusBitField2 & ics.SPY_STATUS2_ERROR_FRAME
),
channel=ics_msg.NetworkID,
)

def _recv_internal(self, timeout=0.1):
Expand Down Expand Up @@ -479,12 +468,16 @@ def send(self, msg, timeout=0):
message.StatusBitField2 = 0
message.StatusBitField3 = flag3
if msg.channel is not None:
message.NetworkID = msg.channel
network_id = msg.channel
elif len(self.channels) == 1:
message.NetworkID = self.channels[0]
network_id = self.channels[0]
else:
raise ValueError("msg.channel must be set when using multiple channels.")

message.NetworkID, message.NetworkID2 = int(network_id & 0xFF), int(
(network_id >> 8) & 0xFF
)

if timeout != 0:
msg_desc_id = next(description_id)
message.DescriptionID = msg_desc_id
Expand Down