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

Support for network ID above 255 #1627

Merged
merged 1 commit into from Jul 6, 2023
Merged

Support for network ID above 255 #1627

merged 1 commit into from Jul 6, 2023

Conversation

pierreluctg
Copy link
Collaborator

No description provided.

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

@pierreluctg pierreluctg merged commit 78d25ff into develop Jul 6, 2023
55 checks passed
@pierreluctg pierreluctg deleted the neovi-NetworkID2 branch July 6, 2023 14:13
@zariiii9003
Copy link
Collaborator

@pierreluctg If you consider this a bug fix, you could also backport this to the 4.2 branch. I'll probably create another release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants