Support for network ID above 255 #1627
Conversation
| @@ -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( | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That would mean having a dictionary with the arguments, what's the difference with using partial, or what do you dislike about partial?
There was a problem hiding this comment.
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,
...
)There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Looking at the CPython partial implementation, I believe this overhead will be negligible.
|
@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. |
No description provided.