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 all 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
46 changes: 33 additions & 13 deletions canopen/sdo/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@
break

def read_response(self):
"""Wait for an SDO response and handle timeout or remote abort.

:raises canopen.SdoAbortedError:
When receiving an SDO abort response from the server.
:raises canopen.SdoCommunicationError:
After timeout with no response received.
"""
try:
response = self.responses.get(
block=True, timeout=self.RESPONSE_TIMEOUT)
Expand All @@ -89,11 +96,11 @@
except SdoCommunicationError as e:
retries_left -= 1
if not retries_left:
self.abort(0x5040000)
self.abort(0x0504_0000)

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

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L99 

Added line #L99 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 +309,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 312 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L312 

Added line #L312 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 315 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L315 

Added line #L315 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 +371,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 374 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L374 

Added line #L374 was not covered by tests
raise SdoCommunicationError(
f"Unexpected response 0x{res_command:02X}")
else:
Expand Down Expand Up @@ -390,6 +400,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 403 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L403 

Added line #L403 was not covered by tests
raise SdoCommunicationError(
f"Unexpected response 0x{res_command:02X}")
bytes_sent = len(b)
Expand All @@ -414,6 +425,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 428 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L428 

Added line #L428 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 +499,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 +556,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 559 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L559 

Added line #L559 was not covered by tests
raise SdoCommunicationError("CRC is not OK")
logger.info("CRC is OK")
self.pos += len(data)
Expand All @@ -564,8 +576,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 580 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L579-L580 

Added lines #L579 - L580 were not covered by tests

def _ack_block(self):
request = bytearray(8)
Expand All @@ -576,15 +588,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 595 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L593-L595 

Added lines #L593 - L595 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 599 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L599 

Added line #L599 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 603 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L603 

Added line #L603 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 +670,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 673 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L673 

Added line #L673 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 +750,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 757 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L757 

Added line #L757 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