Skip to content

sdo.client: Add missing abort messages #594

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 9 commits into from
Jul 13, 2025
Merged

sdo.client: Add missing abort messages #594

Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fb5ea00
adding missing sdo aborts for sdo client
samsamfire Mar 15, 2023
551e20a
adding some missing sdo aborts (timeout, unknown command)
samsamfire Mar 15, 2023
14ecf12
Consistently format abort code literals.
acolomb Jun 17, 2025
080d873
Fix typo (grammatical error in exception message).
acolomb Jun 17, 2025
8beeba4
Avoid duplicate SDO abort messages on timeout.
acolomb Jun 18, 2025
cf34d24
Move SDO abort message sending to _block_ack() and _end_upload().
acolomb Jun 30, 2025
e020208
Ditch unused timeout_abort parameter again.
acolomb Jun 30, 2025
3de9bce
Another literal format.
acolomb Jun 30, 2025
d02f3a7
Add docstring to read_response() and detail exceptions.
acolomb Jul 13, 2025
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
39 changes: 26 additions & 13 deletions canopen/sdo/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,11 @@
except SdoCommunicationError as e:
retries_left -= 1
if not retries_left:
self.abort(0x5040000)
self.abort(0x0504_0000)

Check warning on line 92 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L92 

Added line #L92 was not covered by tests
raise
logger.warning(str(e))

def abort(self, abort_code=0x08000000):
def abort(self, abort_code=0x0800_0000):
"""Abort current transfer."""
request = bytearray(8)
request[0] = REQUEST_ABORTED
Expand Down Expand Up @@ -302,8 +302,10 @@
response = self.sdo_client.request_response(request)
res_command, = struct.unpack_from("B", response)
if res_command & 0xE0 != RESPONSE_SEGMENT_UPLOAD:
self.sdo_client.abort(0x0504_0001)

Check warning on line 305 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L305 

Added line #L305 was not covered by tests
raise SdoCommunicationError(f"Unexpected response 0x{res_command:02X}")
if res_command & TOGGLE_BIT != self._toggle:
self.sdo_client.abort(0x0503_0000)

Check warning on line 308 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L308 

Added line #L308 was not covered by tests
raise SdoCommunicationError("Toggle bit mismatch")
length = 7 - ((res_command >> 1) & 0x7)
if res_command & NO_MORE_DATA:
Expand Down Expand Up @@ -362,6 +364,7 @@
response = sdo_client.request_response(request)
res_command, = struct.unpack_from("B", response)
if res_command != RESPONSE_DOWNLOAD:
self.sdo_client.abort(0x0504_0001)

Check warning on line 367 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L367 

Added line #L367 was not covered by tests
raise SdoCommunicationError(
f"Unexpected response 0x{res_command:02X}")
else:
Expand Down Expand Up @@ -390,6 +393,7 @@
response = self.sdo_client.request_response(request)
res_command, = struct.unpack_from("B", response)
if res_command & 0xE0 != RESPONSE_DOWNLOAD:
self.sdo_client.abort(0x0504_0001)

Check warning on line 396 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L396 

Added line #L396 was not covered by tests
raise SdoCommunicationError(
f"Unexpected response 0x{res_command:02X}")
bytes_sent = len(b)
Expand All @@ -414,6 +418,7 @@
response = self.sdo_client.request_response(request)
res_command, = struct.unpack("B", response[0:1])
if res_command & 0xE0 != RESPONSE_SEGMENT_DOWNLOAD:
self.sdo_client.abort(0x0504_0001)

Check warning on line 421 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L421 

Added line #L421 was not covered by tests
raise SdoCommunicationError(
f"Unexpected response 0x{res_command:02X} "
f"(expected 0x{RESPONSE_SEGMENT_DOWNLOAD:02X})")
Expand Down Expand Up @@ -487,7 +492,7 @@
res_command, res_index, res_subindex = SDO_STRUCT.unpack_from(response)
if res_command & 0xE0 != RESPONSE_BLOCK_UPLOAD:
self._error = True
self.sdo_client.abort(0x05040001)
self.sdo_client.abort(0x0504_0001)
raise SdoCommunicationError(f"Unexpected response 0x{res_command:02X}")
# Check that the message is for us
if res_index != index or res_subindex != subindex:
Expand Down Expand Up @@ -544,7 +549,7 @@
if self._done:
if self._server_crc != self._crc.final():
self._error = True
self.sdo_client.abort(0x05040004)
self.sdo_client.abort(0x0504_0004)

Check warning on line 552 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L552 

Added line #L552 was not covered by tests
raise SdoCommunicationError("CRC is not OK")
logger.info("CRC is OK")
self.pos += len(data)
Expand All @@ -564,8 +569,8 @@
self._ackseq = seqno
return response
self._error = True
self.sdo_client.abort(0x05040000)
raise SdoCommunicationError("Some data were lost and could not be retransmitted")
self.sdo_client.abort(0x0504_0000)
raise SdoCommunicationError("Some data was lost and could not be retransmitted")

Check warning on line 573 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L572-L573 

Added lines #L572 - L573 were not covered by tests

def _ack_block(self):
request = bytearray(8)
Expand All @@ -576,15 +581,19 @@
self._ackseq = 0

def _end_upload(self):
response = self.sdo_client.read_response()
try:
response = self.sdo_client.read_response()
except SdoCommunicationError:
self.abort(0x0504_0000)
Comment on lines +593 to +594
Copy link
Collaborator

Choose a reason for hiding this comment

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

This assumes that read_response() only generates SdoCommunicationError on timeout. It does so now, so it works, but this assumption might be a little fragile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm... I don't see that as fragile, it's even in the same source file to check against.

You're right in the opposite direction though. We don't process other exceptions by sending a different abort message. But that's a different improvement to be made IMHO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If someone changes the behavior in read_response() where SdoCommunicationError might be used for something else in addition to timeout, then the code here will not be correct. This implicit, undocumented, expectancy is what's fragile. So how can we mitigate that?

Either put a comment in read_response() stating that "the callers of this function assumes SdoCommunicationError is generated for timeout only", or a comment in this function stating that "This assumes read_response() only return SdoCommunicationError on timeout".

The code as it is is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a docstring to the method, explicitly mentioning the timeout as reason.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you. Looks good.

raise

Check warning on line 588 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L586-L588 

Added lines #L586 - L588 were not covered by tests
res_command, self._server_crc = struct.unpack_from("<BH", response)
if res_command & 0xE0 != RESPONSE_BLOCK_UPLOAD:
self._error = True
self.sdo_client.abort(0x05040001)
self.sdo_client.abort(0x0504_0001)

Check warning on line 592 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L592 

Added line #L592 was not covered by tests
raise SdoCommunicationError(f"Unexpected response 0x{res_command:02X}")
if res_command & 0x3 != END_BLOCK_TRANSFER:
self._error = True
self.sdo_client.abort(0x05040001)
self.sdo_client.abort(0x0504_0001)

Check warning on line 596 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L596 

Added line #L596 was not covered by tests
raise SdoCommunicationError("Server did not end transfer as expected")
# Return number of bytes not used in last message
return (res_command >> 2) & 0x7
Expand Down Expand Up @@ -654,7 +663,7 @@
response = sdo_client.request_response(request)
res_command, res_index, res_subindex = SDO_STRUCT.unpack_from(response)
if res_command & 0xE0 != RESPONSE_BLOCK_DOWNLOAD:
self.sdo_client.abort(0x05040001)
self.sdo_client.abort(0x0504_0001)

Check warning on line 666 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L666 

Added line #L666 was not covered by tests
raise SdoCommunicationError(
f"Unexpected response 0x{res_command:02X}")
# Check that the message is for us
Expand Down Expand Up @@ -734,14 +743,18 @@

def _block_ack(self):
logger.debug("Waiting for acknowledgement of last block...")
response = self.sdo_client.read_response()
try:
response = self.sdo_client.read_response()
except SdoCommunicationError:
self.sdo_client.abort(0x0504_0000)
Comment on lines +755 to +756
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto

raise

Check warning on line 750 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L750 

Added line #L750 was not covered by tests
res_command, ackseq, blksize = struct.unpack_from("BBB", response)
if res_command & 0xE0 != RESPONSE_BLOCK_DOWNLOAD:
self.sdo_client.abort(0x05040001)
self.sdo_client.abort(0x0504_0001)
raise SdoCommunicationError(
f"Unexpected response 0x{res_command:02X}")
if res_command & 0x3 != BLOCK_TRANSFER_RESPONSE:
self.sdo_client.abort(0x05040001)
self.sdo_client.abort(0x0504_0001)
raise SdoCommunicationError("Server did not respond with a "
"block download response")
if ackseq != self._blksize:
Expand Down
Loading