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
acolomb 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 @@ def send_request(self, request):
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 @@ def request_response(self, sdo_request):
except SdoCommunicationError as e:
retries_left -= 1
if not retries_left:
self.abort(0x5040000)
self.abort(0x0504_0000)
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 @@ def read(self, size=-1):
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)
Comment thread
sveinse marked this conversation as resolved.
raise SdoCommunicationError(f"Unexpected response 0x{res_command:02X}")
if res_command & TOGGLE_BIT != self._toggle:
self.sdo_client.abort(0x0503_0000)
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 @@ def __init__(self, sdo_client, index, subindex=0, size=None, force_segment=False
response = sdo_client.request_response(request)
res_command, = struct.unpack_from("B", response)
if res_command != RESPONSE_DOWNLOAD:
self.sdo_client.abort(0x0504_0001)
raise SdoCommunicationError(
f"Unexpected response 0x{res_command:02X}")
else:
Expand Down Expand Up @@ -390,6 +400,7 @@ def write(self, b):
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)
raise SdoCommunicationError(
f"Unexpected response 0x{res_command:02X}")
bytes_sent = len(b)
Expand All @@ -414,6 +425,7 @@ def write(self, b):
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)
raise SdoCommunicationError(
f"Unexpected response 0x{res_command:02X} "
f"(expected 0x{RESPONSE_SEGMENT_DOWNLOAD:02X})")
Expand Down Expand Up @@ -487,7 +499,7 @@ def __init__(self, sdo_client, index, subindex=0, request_crc_support=True):
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 @@ def read(self, size=-1):
if self._done:
if self._server_crc != self._crc.final():
self._error = True
self.sdo_client.abort(0x05040004)
self.sdo_client.abort(0x0504_0004)
raise SdoCommunicationError("CRC is not OK")
logger.info("CRC is OK")
self.pos += len(data)
Expand All @@ -564,8 +576,8 @@ def _retransmit(self):
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")

def _ack_block(self):
request = bytearray(8)
Expand All @@ -576,15 +588,19 @@ def _ack_block(self):
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

@sveinse sveinse Jul 6, 2025

Copy link
Copy Markdown
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.

@acolomb acolomb Jul 6, 2025

Copy link
Copy Markdown
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.

@sveinse sveinse Jul 11, 2025

Copy link
Copy Markdown
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.

@acolomb acolomb Jul 13, 2025

Copy link
Copy Markdown
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.

@sveinse sveinse Jul 13, 2025

Copy link
Copy Markdown
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
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)
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)
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 @@ def __init__(self, sdo_client, index, subindex=0, size=None, request_crc_support
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)
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 tell(self):

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

@sveinse sveinse Jul 6, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ditto

raise
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