Skip to content

Conversation

@JimHofman
Copy link

@JimHofman JimHofman commented Oct 6, 2022

Add error handling for PacketHandler and CCSDSPacketHandler for check on the size relative to the configured packet definitions.

Fixes #368

@JimHofman JimHofman requested review from a team as code owners October 6, 2022 21:24
raise ValueError(msg)

self._pkt_defn = tlm_dict[self.packet]
self._pkt_defn = self.tlm_dict[self.packet_type]
Copy link
Contributor

Choose a reason for hiding this comment

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

@JHofman728 Since you are already grabbing the packet definition here, there really should be no need for the new get_packet_lengths() function below.


if not self.packet:
msg = 'PacketHandler: No packet name provided in handler config as key "packet"'
if not self.packet_type:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please update this field to be packet_name?

@nttoole nttoole changed the title Issue 368 Issue #368: Bad packet size handling in built-in server handlers Oct 12, 2022
return pickle.dumps((self._pkt_defn.uid, input_data), 2)

if self._pkt_defn.nbytes != packet.nbytes:
msg = f"PacketHandler: Packet length of packet does not match packet definition."
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please rephrase to: "PacketHandler: Packet data length does not match packet definition"

nttoole
nttoole previously approved these changes Oct 12, 2022
@nttoole
Copy link
Contributor

nttoole commented Oct 13, 2022

@JimHofman Were you able to successfully run pytest on this branch?
I am getting errors like:
_pickle.PicklingError: Can't pickle <class 'ait.core.tlm.PacketDefinition'>: it's not the same object as ait.core.tlm.PacketDefinition
...which I presume is related to one of apparently thousands of environment issues on my system, but wanted to confirm.


if self._pkt_defn.nbytes != packet.nbytes:
msg = f"PacketHandler: Packet data length does not match packet definition."
raise ValueError(msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

@JimHofman Could this be an error log message instead of thrown error (same as CCSDS handler)

Copy link
Author

Choose a reason for hiding this comment

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

Done, will work on pickle error before pushing.

Copy link
Author

Choose a reason for hiding this comment

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

Looks like the pickle error is coming from the unit tests that test bad packet length. It's is set up by passing a 1552 packet to an Ethernet handler, so it makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

This check doesn't even look valid? You should be checking the length of the received input data not the number of bytes in a packet definition.

@nttoole nttoole self-requested a review October 13, 2022 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bad packet size handling in built-in server handlers

4 participants