-
-
Notifications
You must be signed in to change notification settings - Fork 215
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
Add WWH OBD DTC message handling #264
Conversation
The quality of this work is pleasant. Do you need it released right away? Or do you plan on making additional PRs? |
If possible sometime later this week. I'm checking with the team to see if there were any other issues we're looking to resolve but for the time being this is the main one. I'll check the CI results in a bit. It didn't seem immediately obvious what the failure was. |
just restarted ci. venv creation failed.. I don't think it's coming from you. |
Possibly this : 9ff5f4a I think my master branch was passing because it was reusing a venv created previously. Don't forget to remove the debug prints |
I will leave this here. It's a change made in setuptools. |
Can you add constants to define the common values of Emissions-system group : 0x33 You can add a class inside common/dtc.py So we can have Finally, add an autoclass in doc/source/udsoncan/helper_classes.rst. Sphinx will connect your docstring to it automatically. |
I updated the docs. Let me know if there's anything else. I'd like to squash and reword once you're happy. I need to validate a few things tomorrow but the unit tests look ok to me and I think the ergonomics are improved. Thanks for the help. |
Will do a final check later. |
You have my 2 final comments. |
- New toplevel client function get_wwh_obd_dtc_by_status_mask() - Added constants for FunctionalGroupId in Dtc helper - Updated documentation - Updated unit tests
@jacobs-rvw If possible could you add support for report wwhobd dtc with permanent status? |
@pylessard - I resolved those two findings, squashed and cleaned up. Tested again and it seems to meet the our needs. If you can merge/release this week it would be appreciated. Thanks for the help. |
Add support for 0x42 reportWWHOBDDTCByMaskRecord.
Closes: #245