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

Some improvements on xcp commands discovery #108

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Some improvements on xcp commands discovery #108

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 26 additions & 14 deletions caringcaribou/modules/xcp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

The function parameter is named msg, not data. This change makes the comment incorrect (which even triggers a warning in IDEs like JetBrains PyCharm)

:msg: True if data is a valid XCP response,
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation of the expected return value(s) of a function should be specified via :return. This is broken by the suggested change.

False otherwise
"""
data = msg.data
Expand Down Expand Up @@ -234,33 +234,39 @@ def response_analyser(msg):
def discovery_end(s):
print("\r{0}: Found {1} possible matches.".format(s, hit_counter))

can_wrap.bruteforce_arbitration_id([0xff], response_analyser_wrapper,
can_wrap.bruteforce_arbitration_id([0xff, 0x00], response_analyser_wrapper,
min_id=min_id, max_id=max_id, callback_end=discovery_end)


def xcp_command_discovery(args):
timeout = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

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 🙂

Copy link
Author

Choose a reason for hiding this comment

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

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,

"""Attempts to call all XCP commands and lists which ones are supported."""
global connect_reply, command_reply
send_arb_id = args.src
rcv_arb_id = args.dst
connect_message = [0xff, 0, 0, 0, 0, 0, 0, 0]
connect_message = [0xFF, 0, 0, 0, 0, 0, 0, 0]

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, this wording is better 👍

print("-" * 33)
with CanActions(send_arb_id) as can_wrap:
can_wrap.bus.set_filters([
{"can_id": rcv_arb_id, "can_mask": 0x7FF, "extended": False},
{"can_id": send_arb_id, "can_mask": 0x7FF, "extended": False}
])
# 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:]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Excluding disconnect makes sense 👍

# Connect
connect_reply = False
can_wrap.send_single_message_with_callback(connect_message, connect_callback_handler)
connect_timestamp = datetime.now()
while not connect_reply and datetime.now() - connect_timestamp < timedelta(seconds=3):
while not connect_reply and datetime.now() - connect_timestamp < timedelta(seconds=timeout):
pass
if not connect_reply:
print("ERROR: Connect timeout")
Expand All @@ -273,14 +279,20 @@ def connect_callback_handler(msg):
def callback_handler(msg):
global command_reply
if msg.arbitration_id == rcv_arb_id:
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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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])))
Copy link
Contributor

Choose a reason for hiding this comment

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

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):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 😁

pass
if not command_reply:
print("ERROR: Command timeout")
Expand Down Expand Up @@ -331,7 +343,7 @@ def handle_get_id_reply(msg):
can_wrap.send_single_message_with_callback([0xf5, msg.data[4]], callback_wrapper(print_msg_as_text))

# Define probe messages
probe_msgs = [ProbeMessage([0xff], decode_connect_response), # Connect
probe_msgs = [ProbeMessage([0xff, 0x00], decode_connect_response), # Connect
ProbeMessage([0xfb], decode_get_comm_mode_info_response), # GetCommMode
ProbeMessage([0xfd], decode_get_status_response), # GetStatus
ProbeMessage([0xfa, 0x00], handle_get_id_reply), # GetId ASCII text
Expand Down Expand Up @@ -447,14 +459,14 @@ def handle_connect_reply(msg):
else:
print("Unexpected connect reply: {0}\n".format(msg))

# Calculate address bytes (4 bytes, the least significant first)
# Calculate address bytes (4 bytes, least significant first)
r = []
n = start_address
bytes_left = length
# Calculate start address (r is automatically reversed after connect if needed)
n &= 0xffffffff
n &= 0xFFFFFFFF
for i in range(4):
r.append(n & 0xff)
r.append(n & 0xFF)
n >>= 8
# Make sure dump_file can be opened if specified (clearing it if it already exists)
if dump_file:
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the padding removed from this specific connect message?

Copy link
Author

Choose a reason for hiding this comment

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

Nice question. I think I forgot to put it back as I was testing something.

# Idle timeout handling
timeout_start = datetime.now()
while not dump_complete and datetime.now() - timeout_start < timedelta(seconds=3):
Expand Down