Some improvements on xcp commands discovery #108
Conversation
|
Hi there! First of all, thank you very much for sending a PR! Sorry to keep you waiting, but I will review this as soon as I can 😅 Just wanted to let you know that this has not been forgotten. |
There was a problem hiding this comment.
Hi again and thanks for your patience!
I generally like what I see, and I also noted some things which might need some extra tweaking. Please let me know if anything in my comments is unclear, if you want to discuss my suggestions or if you need help implementing them 😃
| @@ -191,8 +191,8 @@ def is_valid_response(msg): | |||
| """ | |||
| Returns a bool indicating whether 'data' is a valid XCP response | |||
|
|
|||
| :param msg: list of message data bytes | |||
| :return: True if data is a valid XCP response, | |||
| :param data: list of message data bytes | |||
There was a problem hiding this comment.
The function parameter is named msg, not data. This change makes the comment incorrect (which even triggers a warning in IDEs like JetBrains PyCharm)
| :param msg: list of message data bytes | ||
| :return: True if data is a valid XCP response, | ||
| :param data: list of message data bytes | ||
| :msg: True if data is a valid XCP response, |
There was a problem hiding this comment.
The documentation of the expected return value(s) of a function should be specified via :return. This is broken by the suggested change.
| min_id=min_id, max_id=max_id, callback_end=discovery_end) | ||
|
|
||
|
|
||
| def xcp_command_discovery(args): | ||
| timeout = 5 |
There was a problem hiding this comment.
I really like that you moved the magic number 3 from the timedelta function call in line 269 (that is, line 263 in the old file) to a variable definition clearly defining its meaning.
As for changing it from 3 to 5 seconds, I guess that increasing the connection timeout could prove beneficial when working with slower/busy ECUs (if such cases actually exist), without too much negative impact (waiting two extra seconds) in cases where no connection can be established. In cases like these, anecdotal evidence (even with sample size 1) can be enough to motivate a value change. Question: Have you actually observed cases where an ECU takes more than 3 but less than 5 seconds to respond to an XCP connection request? If so I agree of increasing the value to 5, but otherwise we might just as well stick with 3 🙂
There was a problem hiding this comment.
There is no reason for that 5, I just put a small number when I moved the variable. So, yes, 3 is better. But basically I never saw such a slow response. It either is immediately or you get no response,
|
|
||
| def connect_callback_handler(msg): | ||
| global connect_reply | ||
| if msg.arbitration_id == rcv_arb_id: | ||
| connect_reply = True | ||
|
|
||
| print("XCP command discovery\n") | ||
| print("COMMAND{0}SUPPORTED".format(" " * 17)) | ||
| print("COMMAND{0}RESPONSE".format(" " * 17)) |
There was a problem hiding this comment.
I agree, this wording is better 👍
| # Bruteforce against list of commands (excluding connect) | ||
| for cmd_code, cmd_desc in XCP_COMMAND_CODES[1:]: | ||
| # Don't try Connect and Disconnect discovery | ||
| for cmd_code, cmd_desc in XCP_COMMAND_CODES[2:]: |
There was a problem hiding this comment.
Excluding disconnect makes sense 👍
| print("{0:<23} {1}".format(cmd_desc, msg.data[0] != 0xfe)) | ||
| if msg.data[0] == 0xFF: | ||
| print("{0:<23} AVAILABLE".format(cmd_desc)) | ||
| elif (msg.data[0] == 0xFE) and (msg.data[1] == 0x20): |
There was a problem hiding this comment.
Trying to read from msg.data[1] will raise an IndexError: bytearray index out of range if msg only contains one byte.
A length check (e.g. len(msg.data) > 1) is needed before trying to read this index.
There was a problem hiding this comment.
Yes, a length check is better. I didn't put it because it was working as it is in my case
| print("{0:<23} NOT AVAILABLE".format(cmd_desc)) | ||
| else: | ||
| print("{0:<23} {1} ({2})".format(cmd_desc,XCP_ERROR_CODES[msg.data[1]][0], | ||
| hex(msg.data[1]))) |
There was a problem hiding this comment.
Same thing here with message length as above - length check needed before accessing index 1. This also means there will be a need for a final else case or other handling, when the conditions on this one are not met (i.e. the corner case when msg.data[0] is not 0xFF and msg.data is only 1 byte long).
| command_reply = True | ||
|
|
||
| command_reply = False | ||
| # Send, wait for reply, clear listeners and move on | ||
| can_wrap.send_single_message_with_callback(cmd_msg, callback=callback_handler) | ||
| command_timestamp = datetime.now() | ||
| while not command_reply and datetime.now() - command_timestamp < timedelta(seconds=3): | ||
| while not command_reply and datetime.now() - command_timestamp < timedelta(seconds=timeout): |
| @@ -468,7 +480,7 @@ def handle_connect_reply(msg): | |||
| with CanActions(arb_id=send_arb_id) as can_wrap: | |||
| print("Attempting XCP memory dump") | |||
| # Connect and prepare for dump | |||
| can_wrap.send_single_message_with_callback([0xff, 0, 0, 0, 0, 0, 0, 0], handle_connect_reply) | |||
| can_wrap.send_single_message_with_callback([0xff], handle_connect_reply) | |||
There was a problem hiding this comment.
Why is the padding removed from this specific connect message?
There was a problem hiding this comment.
Nice question. I think I forgot to put it back as I was testing something.
|
Ok. Thanks for you review. When I have the time I will do the changes that you suggested. |
Sounds great! If you cannot find time and would like assistance in fixing these changes, just let me know 🙂 |
|
Hi there, just checking in on the status of this PR 🙂 I aim to have a go on fixing the suggested changes from the review and merging shortly (unless you happen to already have them ready, of course!). |
Hello. I added some improvements on XCP discovery.
I had issues with the original one, as I need to pass the connection type (0x00) to the CONNECT command or it will not be accepted. Also I changed how the results as displayed, not a simple True/False but also the error code, because some commands could be supported but they fail if you don't provide the right parameters.
Also I added a filtering to the messages, so normal CAN traffic will not interfere with the discovery.