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

Add WWH OBD DTC message handling #264

Merged
merged 1 commit into from
Mar 27, 2025
Merged

Conversation

jacobs-rvw
Copy link
Contributor

Add support for 0x42 reportWWHOBDDTCByMaskRecord.

Closes: #245

@pylessard
Copy link
Owner

The quality of this work is pleasant.
Will wait on CI. Will review the implementation against the standard tonight.

Do you need it released right away? Or do you plan on making additional PRs?

@jacobs-rvw
Copy link
Contributor Author

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.

@pylessard
Copy link
Owner

just restarted ci. venv creation failed.. I don't think it's coming from you.

@pylessard
Copy link
Owner

pylessard commented Mar 24, 2025

Possibly this : 9ff5f4a

I think my master branch was passing because it was reusing a venv created previously.
I fixed the master branch. You can merge or rebase your PR.

Don't forget to remove the debug prints

@pylessard
Copy link
Owner

I will leave this here. It's a change made in setuptools.

apache/spark#43891
apache/spark#50371

@pylessard
Copy link
Owner

Can you add constants to define the common values of FunctionalGroupIdentifiers. There seems to be only 3 value defined.

Emissions-system group : 0x33
Safety-system group: 0xD0
VOBD system: 0xFE

You can add a class inside common/dtc.py So we can have Dtc.FunctionalGroupIdentifiers. Inside your method docstring, you can mention that the values can comes from :class:`Dtc.FunctionalGroupIdentifiers<Dtc.FunctionalGroupIdentifiers>`

Finally, add an autoclass in doc/source/udsoncan/helper_classes.rst. Sphinx will connect your docstring to it automatically.

@jacobs-rvw
Copy link
Contributor Author

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.

@pylessard
Copy link
Owner

Will do a final check later.
I only do squash-merge btw, so do 't worry too much about the history

@pylessard
Copy link
Owner

You have my 2 final comments.
Call the shot and I will merge.
Let me know iwhen you need the release. It takes less than a minute to make

- New toplevel client function get_wwh_obd_dtc_by_status_mask()
- Added constants for FunctionalGroupId in Dtc helper
- Updated documentation
- Updated unit tests
@Krash-2504
Copy link

@jacobs-rvw If possible could you add support for report wwhobd dtc with permanent status?

@jacobs-rvw
Copy link
Contributor Author

@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.

@pylessard pylessard merged commit 9fb5c6a into pylessard:master Mar 27, 2025
1 check 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.

No suppport for WWHOBD
4 participants