Skip to content
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

Bug fixes, send/receive debug logs and Unsigned 24 support #267

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Bug fixes, send/receive debug logs and Unsigned 24 support #267

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5141b19
test if callback exist before removing it
TzviRonen Sep 6, 2021
3ce8ab4
Added send message debug log
TzviRonen Sep 6, 2021
bc06df4
Added debug logs and pause before read
TzviRonen Sep 6, 2021
3fb146e
fixed get_data var len calc
TzviRonen Sep 6, 2021
659899d
Added Unsigned24 data type
TzviRonen Sep 6, 2021
9b9efb7
Added PyCharm to gitignore
TzviRonen Sep 6, 2021
a8fcd87
request_response RETRY_DELAY
TzviRonen Sep 8, 2021
d0d9bba
made RETRY_DELAY default zero
TzviRonen Sep 8, 2021
b14f43e
removed callback test
TzviRonen Sep 8, 2021
56b206a
fixed uint24 support comments
TzviRonen Sep 8, 2021
a108eee
t
TzviRonen Sep 8, 2021
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
6 changes: 6 additions & 0 deletions canopen/sdo/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ class SdoClient(SdoBase):
#: Seconds to wait before sending a request, for rate limiting
PAUSE_BEFORE_SEND = 0.0

# Seconds to wait before next read attempt for response in queue. For delayed responses.
PAUSE_BEFORE_READ = 0.2
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps RETRY_DELAY or something would be more easily understandable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the benefit of this mechanism? read_response() will already wait up to RESPONSE_TIMEOUT seconds, so this additional delay only slows down communication in case a reply is already available shortly after the first timeout.

If there is a special case that requires such behavior, we should maybe make it optional by defaulting the constant to zero?

Copy link
Author

@TzviRonen TzviRonen Sep 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@acolomb If it fails to respond to the send_request that may be a sign that the device isn't ready and not necessarily a communication error.
Why not use RESPONSE_TIMEOUT? I want to know about it in the log's as soon as it happens and yet, wait more time before the next read_response().
I used it only for debugging. I agree with you it should be optional by defaulting it to zero.
And I agree with @christiansandberg about the naming. I changed the name to RETRY_DELAY.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a very slight notion, but time.sleep(0) will allow switching to a different thread AFAIK, while not calling it at all does not. I suppose that doesn't make much difference, but maybe better check the constant first and skip sleep if it's zero?


def __init__(self, rx_cobid, tx_cobid, od):
"""
:param int rx_cobid:
Expand All @@ -42,6 +45,7 @@ def __init__(self, rx_cobid, tx_cobid, od):
self.responses = queue.Queue()

def on_response(self, can_id, data, timestamp):
logger.debug(f"received response in {can_id} data {data}.")
Copy link
Owner

Choose a reason for hiding this comment

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

As I understand it, it is preferable to use old-style string interpolation for logging because then the final message will only be constructed if needed.

https://docs.python.org/3/howto/logging.html#logging-variable-data

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, we should keep a consistent style here, meaning %-style format strings and arguments to the debug() function.

self.responses.put(bytes(data))

def send_request(self, request):
Expand All @@ -50,6 +54,7 @@ def send_request(self, request):
try:
if self.PAUSE_BEFORE_SEND:
time.sleep(self.PAUSE_BEFORE_SEND)
logger.debug(f"sending to {self.rx_cobid} data {request} ")
self.network.send_message(self.rx_cobid, request)
except CanError as e:
# Could be a buffer overflow. Wait some time before trying again
Expand Down Expand Up @@ -88,6 +93,7 @@ def request_response(self, sdo_request):
if not retries_left:
self.abort(0x5040000)
raise
time.sleep(self.PAUSE_BEFORE_READ)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If an additional delay is appropriate here (see above), it should wait after logging, not before.

logger.warning(str(e))

def abort(self, abort_code=0x08000000):
Expand Down