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

Support ELM327 error messages #87

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

Support ELM327 error messages #87

wants to merge 13 commits into from

Conversation

ffries
Copy link

@ffries ffries commented Jun 27, 2017

Dear friends,

This is a first attempt to display debug information when an error message is returned by the ELM327. Please review with care, as I am new to the project and this is a way to learn more about it.

Kind regards,
Kellogs

@ffries
Copy link
Author

ffries commented Jun 27, 2017

This also sent my previous patch.
Sorry I should have made a fresh checkout.

@ffries
Copy link
Author

ffries commented Jun 28, 2017

Please review this patch. If you don't like it, just cancel it and ask for modifications or propose your own.

[b"STOPPED", "STOPPED: ELM327 reports that OBD operation was interrupted by a received RS232 character, or by a low level on the RTS pin."],
[b"UNABLE TO CONNECT", "UNABLE TO CONNECT: ELM327 tried all available protocols, and could not detect a compatible one."],
[b"ERR", "ERR: ELM327 reports an error number following."]
]
Copy link
Owner

Choose a reason for hiding this comment

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

This is awesome, thanks!

Copy link
Owner

Choose a reason for hiding this comment

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

Can this be turned into a dictionary for clarity?

{
    b"?" : "?: ELM327 reports misundersting command received on the RS232 input.",
    ...
}


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.

Would you mind rebasing-out these changes? (assuming that they aren't co-dependant) That way, the two changes can cleanly come in via two PR's. Same with the addition of the get_ prefixes; Discuss that separately.

break

buffer.extend(data)

# end on chevron (ELM prompt character)
if self.ELM_PROMPT in buffer:
break

#Check errors received by ELM327 and write debug
for iError in self._ERROR_MESSAGES:
Copy link
Owner

Choose a reason for hiding this comment

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

minor nit-pick: in python, we typically prefer lower-case, underscore separated names. For this, simply error would suffice (especially if we used the dictionary approach I mentioned above, which iterates over keys).

]

__misc__ = [
# name description cmd bytes decoder ECU fast
OBDCommand("ELM_VERSION" , "ELM327 version string" , b"ATI", 0, raw_string, ECU.UNKNOWN, False),
OBDCommand("ELM_VOLTAGE" , "Voltage detected by OBD-II adapter" , b"ATRV", 0, elm_voltage, ECU.UNKNOWN, False),
OBDCommand("FAKE_COMMAND_1" , "Non-existing command returning error" , b"ATXYZ", 0, raw_string, ECU.UNKNOWN, False),

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.

Is this used for anything here besides development? I don't think python-OBD needs to be shipped with a fake command.

# OBDCommand("VIN" , "Get Vehicle Identification Number" , b"0902", 20, raw_string, ECU.ENGINE, True),
OBDCommand("PIDS_9A" , "Supported PIDs [01-20]" , b"0900", 4, pid, ECU.ENGINE, True),
OBDCommand("VIN_MESSAGE_COUNT" , "VIN Message Count" , b"0901", 1, uas(0x01), ECU.ENGINE, True),
OBDCommand("VIN" , "Get Vehicle Identification Number" , b"0902", 20, raw_string, ECU.ENGINE, True),
Copy link
Owner

Choose a reason for hiding this comment

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

Were you able to get VIN to work?

@alistair23
Copy link
Collaborator

Hello Kellogs,

Are you able to make the requested changes? If you aren't do you mind if I pickup the patches and get them merged?

Copy link

@robert-jf-close robert-jf-close left a comment

Choose a reason for hiding this comment

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

Just need to remove the test code Brendan mentioned

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.

4 participants