sdo.client: Add missing abort messages #594
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Sending the abort message when |
For each call to SdoClient.read_response(), the timeout abort message can either be sent immediately before raising the exception, or it is sent later, after the exception has been handled e.g. by a retry. Add a boolean parameter to the function, which is usually False to skip the immediate transmission of an SDO abort. Only two cases actually need it, passing the option as True.
|
The last commit introduces such an optional parameter, whether an immediate abort on timeout is appropriate. However, we might want to skip that and just handle timeouts in the two remaining cases directly, by catching the exception, sending the SDO abort there and re-raising. Investigating all these call sites also uncovered a bug in |
canopen/sdo/client.py
Outdated
| if timeout_abort: | ||
| self.abort(0x0504_0000) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
|
@sveinse could you have another look please? This is basically the last thing I'm waiting on for the v2.4.0 release. |
| except SdoCommunicationError: | ||
| self.abort(0x0504_0000) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added a docstring to the method, explicitly mentioning the timeout as reason.
| except SdoCommunicationError: | ||
| self.sdo_client.abort(0x0504_0000) |
|
Thanks @sveinse! |
SDO abort messages were sent in some cases when a response times out or has an unexpected server command specifier. But not consistently, thus the following cases are now added:
Mismatched scs:
Toggle bit mismatch (reports as not toggled):
Some reformatting of abort code literals squeezed in there, as well as a typo fix.
This is based on the changes in #352, isolated from the typing noise.