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.

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

Split find into find and decode #227

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

floroks
Copy link
Contributor

@floroks floroks commented Oct 21, 2023

Fix issue with find command requiring data, by splitting the decode and find (by service name) functionalities
into separate commands.

@floroks floroks force-pushed the fix_find_command branch 2 times, most recently from 2180232 to 608f31e Compare October 21, 2023 10:35
"",
"Examples:",
" For displaying the service associated with the request 10 01 & decoding it:",
" odxtools decode ./path/to/database.pdx -d 10 01",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
" odxtools decode ./path/to/database.pdx -d 10 01",
" odxtools decode ./path/to/database.pdx -d '10 01'",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, yes, thanks, i fixed it to work with -d 10 01 again. -D is a modifier to additionally decode it, although maybe it might be better to just differentiate between -d 10 01 and -D 10 01 - thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

allowing the data to be specified as multiple arguments is a bad idea IMO, see below...

" For more information use:",
" odxtools decode -h",
]),
help="Find & print services by hex-data, or name. Can also decode the request.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

that seems to be not correct after this PR anymore?

Copy link
Contributor Author

@floroks floroks Oct 21, 2023

Choose a reason for hiding this comment

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

oops, yes, thanks, i fixed it to be accurate again

UtilFunctions.run_find_tool(service_names=["headstand"], allow_unknown_bit_lengths=True)
UtilFunctions.run_find_tool(
service_names=["headstand"], allow_unknown_bit_lengths=True, no_details=True)
# UtilFunctions.run_find_tool(service_names="3e00")
Copy link
Collaborator

Choose a reason for hiding this comment

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

avoid commented-out code if there's not a very good reason for it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, I removed it

parser.add_argument(
"-d",
"--data",
nargs="*",
Copy link
Collaborator

@andlaus andlaus Oct 21, 2023

Choose a reason for hiding this comment

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

I'd vastly prefer to make this a simple string, because right now specifying positional arguments after -d makes things quite iffy (e.g. odxtools decode -d 01 01 my_file.pdx will fail while odxtools decode -d "01 01" my_file.pdx will work just fine if --data was a simple string parameter).

Suggested change
nargs="*",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it

@floroks floroks force-pushed the fix_find_command branch 4 times, most recently from 8f70b3f to ea8ff17 Compare October 22, 2023 17:38
@andlaus
Copy link
Collaborator

andlaus commented Oct 23, 2023

okay, looks good to me now. A small thing which I've noticed while playing with it: if the data is specified using an illegal format (e.g. -d "3e:00"), odxtools decode only bails out after it has parsed the PDX file, which can take a few seconds. It would probably be good to reverse this order because parsing the data is basically instantaneous...

@floroks
Copy link
Contributor Author

floroks commented Oct 23, 2023

fixed, I also remove all non-hex-chars from the string, to be more fault-tolerant to the input

@andlaus
Copy link
Collaborator

andlaus commented Oct 23, 2023

ok, thanks! merging.

@andlaus andlaus merged commit b02f4f3 into mercedes-benz:main Oct 23, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants