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 5 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: 23 additions & 16 deletions canopen/sdo/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,13 @@
else:
break

def read_response(self):
def read_response(self, timeout_abort: bool = False):
try:
response = self.responses.get(
block=True, timeout=self.RESPONSE_TIMEOUT)
except queue.Empty:
if timeout_abort:
self.abort(0x0504_0000)

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

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L73 

Added line #L73 was not covered by tests
Copy link
Collaborator

@sveinse sveinse Jun 22, 2025

Choose a reason for hiding this comment

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

Verified. But I'm not fond of using 0x0504_0000 directly in the code. They should rather be constants addressed by name. Add another PR for that?

Copy link
Member Author

@acolomb acolomb Jun 22, 2025

Choose a reason for hiding this comment

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

Sure, we can fix up the magic numbers in code in a follow-up. I'm usually very strict about that, but wanted to keep disruption low while we're looking at an actual bugfix.

Copy link
Collaborator

@sveinse sveinse Jun 22, 2025

Choose a reason for hiding this comment

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

Since we're touching the code with this PR, I think I would suggest doing a quick and dirty fix to add them as literals. It can still be made as a minimal disruption change. E.g. we don't need to make literals of all SDO Errors, just the few we touch here.

Copy link
Member Author

@acolomb acolomb Jun 22, 2025

Choose a reason for hiding this comment

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

Let's focus on the logic first. I will follow up with the literals change once this is merged. The bigger question is whether we want to get rid of the extra parameter again, as mentioned in #594 (comment)?

raise SdoCommunicationError("No SDO response received")
res_command, = struct.unpack_from("B", response)
if res_command == RESPONSE_ABORTED:
Expand All @@ -85,11 +87,11 @@
self.send_request(sdo_request)
# Wait for node to respond
try:
return self.read_response()
return self.read_response(timeout_abort=False)
except SdoCommunicationError as e:
retries_left -= 1
if not retries_left:
self.abort(0x5040000)
self.abort(0x0504_0000)

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

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L94 

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

Expand Down Expand Up @@ -302,8 +304,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 307 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L307 

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

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L310 

Added line #L310 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 +366,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 369 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L369 

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

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L398 

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

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L423 

Added line #L423 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 +494,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 @@ -520,7 +527,7 @@
return self.readall()

try:
response = self.sdo_client.read_response()
response = self.sdo_client.read_response(timeout_abort=False)
except SdoCommunicationError:
response = self._retransmit()
res_command, = struct.unpack_from("B", response)
Expand All @@ -544,7 +551,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 554 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L554 

Added line #L554 was not covered by tests
raise SdoCommunicationError("CRC is not OK")
logger.info("CRC is OK")
self.pos += len(data)
Expand All @@ -556,16 +563,16 @@
end_time = time.time() + self.sdo_client.RESPONSE_TIMEOUT
self._ack_block()
while time.time() < end_time:
response = self.sdo_client.read_response()
response = self.sdo_client.read_response(timeout_abort=False)
res_command, = struct.unpack_from("B", response)
seqno = res_command & 0x7F
if seqno == self._ackseq + 1:
# We should be back in sync
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 575 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L574-L575 

Added lines #L574 - L575 were not covered by tests

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

def _end_upload(self):
response = self.sdo_client.read_response()
response = self.sdo_client.read_response(timeout_abort=True)
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 590 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L590 

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

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L594 

Added line #L594 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 +661,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 664 in canopen/sdo/client.py

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L664 

Added line #L664 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 +741,14 @@

def _block_ack(self):
logger.debug("Waiting for acknowledgement of last block...")
response = self.sdo_client.read_response()
response = self.sdo_client.read_response(timeout_abort=True)
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)

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

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L747 

Added line #L747 was not covered by tests
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)

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

View check run for this annotation

Codecov / codecov/patch

canopen/sdo/client.py#L751 

Added line #L751 was not covered by tests
raise SdoCommunicationError("Server did not respond with a "
"block download response")
if ackseq != self._blksize:
Expand Down
Loading