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

Consider using automotive_diag crate for UDS constants #63

Open
nyurik opened this issue Jun 26, 2024 · 6 comments
Open

Consider using automotive_diag crate for UDS constants #63

nyurik opened this issue Jun 26, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@nyurik
Copy link

nyurik commented Jun 26, 2024

Hi, would you be interested in collaborating on automotive_diag crate which defines constants for UDS and other standards, and allows both std and no-std usage.

@pd0wm
Copy link
Contributor

pd0wm commented Jun 29, 2024

I have seen your crate and appreciate the work!

I'm not sure if it makes sense to rely on an external dependency for this due to how rust packaging works. If I want to quickly add/modify one of the constants I need to make a PR and wait for a new release of your crate, as you can't push to crates.io when your package depends on a non-released version of a crate.

I do have a refactor planned with a similar structure like you have with a Standard and Extended version to allow proper typing of non-standard commands (instead of just accepting u8 as an argument like I do now).

What did you have in mind for a collaboration? Are there any direct benefits to the end-user for sharing constants between multiple crates?

@nyurik
Copy link
Author

nyurik commented Jun 29, 2024

@pd0wm thx for responding! So a few thoughts:

  • I usually respond the same day -- e.g. Add support for routine control types. nyurik/automotive_diag#5
  • I am more than willing to add more maintainers to my repo -- its a joined effort, so the more ppl work together on this, the better it becomes.
  • One authoritative source of all constants that can be used both in vehicle and in diagnostics software seems like a much more maintainable approach -- DRY. Especially considering download/usage stats. Why splinter the community and maintain same info in multiple places?
  • For something as serious as a core automotive crate, everyone would want at least one more person reviewing your changes. So on one hand you have "I want to add a new constant to my code and publish it the same day" vs "I need to add a new feature and have at least one more person review it so that community can trust my code changes" -- and if its the later, by the time it is reviewed, a new constant can certainly be published (most of my crates auto-publish from CI)

So I think we would all benefit from one common implementation rather than going the "lone hero" route. I certainly don't want to be the only hero maintaining my stuff :)

@pd0wm
Copy link
Contributor

pd0wm commented Jun 30, 2024

I'll check what changes would be needed and consider it. Most likely I wont have time till after Black Hat & Defcon.

Two things I'm missing after a quick glance are a way to iterate over commands (I use strum macros for this), and serde support (although I see a branch with some wip code).

@nyurik
Copy link
Author

nyurik commented Jun 30, 2024

Sure. Could you give an example of how you would use iteration over enum values? Is this for UI display purposes, or something else? Also, I did try serde with it because I saw you were using it, but the expected string here seems a bit strange. Is this the type of serialization you expect?

insta::assert_snapshot!(
  json,
  @r###"{"command":"DiagnosticSessionControl","command_byte":{"Standard":"DiagnosticSessionControl"}}"###
);

@pd0wm
Copy link
Contributor

pd0wm commented Jun 30, 2024

  • Iteration over possible UDS services would be useful for checking the availability of all possible services, same for session types and other sub-function enumerations. E.g. for s in automotive::uds::ServiceIdentifier::iter()
  • serde was a request from a (closed source) dependent of this library. They mostly wanted to include SessionTypes and ServiceIdentifiers into another struct that needed to implement Serialize and Deserialize. I wouldn't worry about the exact serialization, as long as it doesn't break Serialize/Deserialize on downstream structs when you include one of the automotive constants it's fine.

@pd0wm pd0wm added the enhancement New feature or request label Jun 30, 2024
@nyurik
Copy link
Author

nyurik commented Jul 1, 2024

I just released v0.1.8 - should make migration much easier (includes iter & serde & a few other things)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants