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

DIscover and print all information #84

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

DIscover and print all information #84

wants to merge 6 commits into from

Conversation

ffries
Copy link

@ffries ffries commented Jun 26, 2017

According to feature #82

This patch adds a print_discovered() clause which prints all detailed information about a connection.
It can simply be triggered using:

import obd
connection = obd.OBD()
connection.print_discovered()

It also modified some functions with the prefix "get_" for clarity and updates testing accordingly.

Feel free to modify, but we probably need some kind of utility to help and debug users.

@ffries
Copy link
Author

ffries commented Jun 26, 2017

An option would be to write a simpler discover_and_exit() clause, which would auto-connect, discover, print OBD available commands and exit.


else:
print ("Impossible to print discovered information: no connection to the ECU.")

Copy link
Owner

Choose a reason for hiding this comment

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

Hey, thanks for the PR!

While I don't have a problem with the printing code, I don't think the library core is the right place for this. I'd like to separate the concerns of OBD communications and tool-like terminal printing. I've been meaning to remove the print_commands function above (which I've intentionally not documented). Instead, I'd be totally supportive of adding a /tools directory for CLI applications akin to this.


o.interface = FakeELM("A different port name")
assert o.port_name() == o.interface._portname
assert o.get_port_name() == o.interface.get_port_name()
Copy link
Owner

@brendan-w brendan-w Jul 16, 2017

Choose a reason for hiding this comment

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

I suppose it just comes down to preference; I'd prefer to leave these the way they were. I'd argue shorter is usually better. Also, if we decide to change them, lets do that in a separate PR.

Choose a reason for hiding this comment

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

Shorter is never better unless there is a concern for memory, which there is not. More descriptive is better IMO.

if self.interface is None:
return []
else:
return self.interface.get_ecus()
Copy link
Owner

Choose a reason for hiding this comment

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

Do you have a specific use for this function? I couldn't quite justify yet, since we only really label the tx ID of the engine.

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.

3 participants