Doip server initial code #4
Conversation
|
Is this code supposed to build? When I ran cargo check and cargo build, I encountered some errors. IMO, code should build successfully. |
a11f029 to
c674ff3
Compare
build is successful is now. |
|
fix minor formatting inconsistencies |
arvindr19
left a comment
There was a problem hiding this comment.
Only header_parser.rs has any tracing at all. Every other file has no tracing.
darkwisebear
left a comment
There was a problem hiding this comment.
I didn't go into details yet, but from what I see there are some general things that need cleanup before I will do that:
- Consolidate error types and use
thiserroreverywhere. - Introduce a trait for message types and use everywhere
- Deduplicate common parsing code
| let eid: [u8; 6] = payload | ||
| .get(..Self::LEN) | ||
| .and_then(|s| s.try_into().ok()) | ||
| .ok_or(Error::PayloadTooShort { | ||
| expected: Self::LEN, | ||
| actual: payload.len(), | ||
| })?; |
There was a problem hiding this comment.
Such code could be factored out, as there is similar code also in alive_check.rs. I think there should be a submodule messages with common definitions and then types for each message.
81e9ff5 to
9fa0c6e
Compare
|
Please run cargo fmt --check; it currently fails. Once the CI workflow PR is merged, this PR will need to be reworked, so it’s better to fix it beforehand. |
lh-sag
left a comment
There was a problem hiding this comment.
Just had a brief look at the PR.
@VinaykumarRS1995 did you had a chance to evaluate any of these crates to ease development?
- https://github.com/samp-reston/doip-definitions
- https://github.com/samp-reston/doip-codec
- https://github.com/samp-reston/doip-sockets
- https://github.com/samp-reston/doip-server
Adding @alexmohr and @theswiftfox to share their experience with some of the crates above.
| fn parse_vin(s: &str) -> anyhow::Result<[u8; 17]> { | ||
| let bytes = s.as_bytes(); | ||
| if bytes.len() != 17 { | ||
| anyhow::bail!("VIN must be exactly 17 characters"); |
There was a problem hiding this comment.
Optional: I would rather return a proper error type than a string.
We're only using codec and definitions in the CDA as we had some issues with sockets. But using codecs and definitions would already prevent re-implemeting some things here. The sockets connections can be re-used from CDA. Currently both doip libraries we use are forked by @theswiftfox. I believe it would be a good idea to use her fork as a base to officially provide an eclipse fork we use in all of our projects. |
As we've also run into some issues with the original code, I think we need should fork them into the eclipse-opensovd repo, so we can adapt them quickly to our needs. I'll put it on the agenda for the next architecture board. |
@lh-sag personally, I would be reluctant with using creates where there is only exactly one maintainer and no updates in the github repo since quite some time. This is risk in my eyes. What's your opinion on this situation? |
The consideration for integrating DoIP crates is tracked in #9 - will revisit once they are officially part of the OpenSOVD project. Not blocking this PR. |
| pub struct PositiveAck { | ||
| source_address: u16, | ||
| target_address: u16, | ||
| ack_code: AckCode, |
There was a problem hiding this comment.
That seems redundant. The pure existence of the instance indicates success.
There was a problem hiding this comment.
Merged PositiveAck and NegativeAck into a single DiagnosticAck with result: AckResult (Positive | Negative(NackCode)).
c8ccfbf to
0c12dc0
Compare
| /// Override this to enable pre-allocated buffers in [`to_bytes`], avoiding | ||
| /// incremental `BytesMut` reallocations for large messages. | ||
| fn serialized_len(&self) -> Option<usize> { | ||
| None |
There was a problem hiding this comment.
Returning None in the default implementation doesn’t seem correct to me. If a user calls this method on a type that doesn’t override it, they still get a valid Option, which may be misleading.
There was a problem hiding this comment.
Done, Changes are pushed
| }) | ||
| } | ||
|
|
||
| // Re-export core types and constants for convenient access. |
There was a problem hiding this comment.
Consider keeping the header at the start of the file. If it’s user-facing, re-exporting it via lib.rs could be a good option.
There was a problem hiding this comment.
Done. Moved all pub use re-exports to top mod.rs
|
In general I would like to see structured logs instead of strings. This is done in CDA and Core. Can you please apply this to your logs? With structured logging (simplified for demonstration purpose): |
- Consolidate errors into DoipError; add TryFrom<u8> on all enums - Strongly type activation_type field (ActivationType, not u8) - Add DoipParseable / DoipSerializable traits + DoipPayload dispatch enum - Override serialized_len() on all impls; fix array size magic numbers - Make ParseError pub(crate); add serde(default), tracing
- Split header_parser.rs into header.rs + codec.rs - Merge ParseError into DoipError (single error type) - Add parse_fixed_slice helper, remove duplicated parse patterns - Add DoipParseable / DoipSerializable traits - Make struct fields private, add public getters - Set default-features = false on all dependencies - Trim tokio features to io-util only - Remove dead test-handlers feature from Cargo.toml
Replace std::result::Result<_, DoipError> with crate::DoipResult across all files; add pub use error::DoipResult in lib.rs - remove redundant AckCode enum - remove redundant ack_code field from PositiveAck - merge PositiveAck + NegativeAck into DiagnosticAck with AckResult enum (Positive | Negative(NackCode))
- structured tracing logging; error! for parse failures - rename DoipResult → Result - single RwLock<SessionManagerInner> + AtomicU64 - merge DiagnosticAck + AckResult; remove AckCode - doc comments on all public API items
af05179 to
1eb7c2b
Compare
| /// a tester is still connected. | ||
| /// | ||
| /// This message carries no payload (zero-length body per ISO 13400-2:2019 §7.6). | ||
| #[derive(Debug, Clone, PartialEq, Eq, Default)] |
There was a problem hiding this comment.
DoipPayoad derives close and wraps this type so clone is required
| /// Alive Check Request carries no payload bytes per ISO 13400-2; | ||
| /// nothing is written to the buffer by design. | ||
| fn write_to(&self, _buf: &mut BytesMut) { | ||
| // Intentionally empty: Alive Check Request has a zero-length payload. |
There was a problem hiding this comment.
maybe lets put some warn log so if someone called by mistake then with logging can be identified for alive check no write support.
| fn decode( | ||
| &mut self, | ||
| src: &mut BytesMut, | ||
| ) -> std::result::Result<Option<Self::Item>, Self::Error> { |
There was a problem hiding this comment.
you have alias for result in error file, why can't use same.
| }) | ||
| } | ||
|
|
||
| fn parse_vin(s: &str) -> crate::Result<[u8; 17]> { |
There was a problem hiding this comment.
Its VIN length -> using VIN_LEN=17
|
|
||
| /// Session states per ISO 13400-2:2019 connection lifecycle | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum SessionState { |
There was a problem hiding this comment.
Fixed — changed to pub(crate). External callers use is_routing_active() instead; no need to expose the internal state machine variants publicly.
|
|
||
| /// The logical source address of the tester | ||
| #[must_use] | ||
| pub fn source_address(&self) -> u16 { |
There was a problem hiding this comment.
name of function should not be same as field name, use may be get
bharatGoswami8
left a comment
There was a problem hiding this comment.
@VinaykumarRS1995 ,
Here is some general feedback to address:
- Enhance the PR summary with more detail and include sequence or class diagrams.
- Review the visibility of all APIs and types and restrict the scope of those not meant for external use.
- Evaluate the use of directly imported free functions.
- check exported types using derive macros (e.g., Default, Clone) to ensure they are necessary.
- Use interface defined Result instead of using std::Result or crate::Result.
- Ensure test-related functions are not public and verify their access levels.
- Refine type naming to more accurately reflect their specific functionality.
- Narrow internal API visibility: DoipParseable→pub(crate), Session::new/activate_routing/connection_state→pub(crate), DoipMessage::with_raw_payload_type→pub(crate), SessionState→pub(crate); external callers use is_routing_active() - Rename protocol enums for domain clarity: NackCode->DiagnosticNackCode, AckResult->DiagnosticAckResult, ResponseCode->ActivationResponseCode, SyncStatus->GidSyncStatus - Restore warn!() in AliveCheckRequest::write_to - Promote VIN_LEN to module-level pub(crate) const (ISO 3779 ref, removes magic 17 literals from struct field, DEFAULT_VIN, parse_vin) - Add comment in codec.rs explaining io::Result vs crate::Result - Add Copy derive to AliveCheckRequest - Add RoutingActivationResponse::with_oem_specific() builder - Consistent crate::Result alias in mod.rs, payload.rs, config.rs - Add PlantUML architecture diagrams (classDia.puml, seq.puml, flowchart.puml)
bharatGoswami8
left a comment
There was a problem hiding this comment.
@VinaykumarRS1995
Please check for following points-
- check exported types using derive macros (Debug, Clone, PartialEq, Eq) to ensure they are necessary, if it is for test then we need remove from type level.
- check if test related function can be implemented in test module.
- Add the svg for design diagram.
- Add design / sequence diagram for interface / public APIs
| /// Payload: SA(2) + TA(2) + code(1) + optional `previous_diag_data` | ||
| /// | ||
| /// `Clone` is derived because [`DoipPayload`](super::payload::DoipPayload) wraps this type and itself derives `Clone`. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] |
There was a problem hiding this comment.
Is making DoipPayload clonable actually necessary ?
There was a problem hiding this comment.
Checke Payload is not cloned in codebase - Removed
| pub struct Request; | ||
| /// | ||
| /// `Clone` is derived because [`DoipPayload`](super::payload::DoipPayload) wraps this type and itself derives `Clone`. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] |
There was a problem hiding this comment.
Is making DoipPayload clonable actually necessary
| /// The logical address of the tester that sent this response. | ||
| #[must_use] | ||
| pub fn source_address(&self) -> u16 { | ||
| pub fn tester_address(&self) -> u16 { |
There was a problem hiding this comment.
Change to fn get_address(&self) -> u16
There was a problem hiding this comment.
suggestion: if such impl is required for test only, we could impl such member functions inside mod tests {}
There was a problem hiding this comment.
Doen. Renamed get_address() moved to tests
| source_address: u16, | ||
| target_address: u16, | ||
| result: AckResult, | ||
| result: DiagnosticAckResult, |
There was a problem hiding this comment.
you already have struct name DiagnosticAck so no need to mention on field name.
There was a problem hiding this comment.
Done. DiagnosticAckResult -> AckResult -- removed prefix
| /// Panics if `payload.len()` exceeds `u32::MAX`. | ||
| #[cfg(test)] | ||
| pub fn with_raw_payload_type(payload_type: u16, payload: Bytes) -> Self { | ||
| pub(crate) fn with_raw_payload_type(payload_type: u16, payload: Bytes) -> Self { |
There was a problem hiding this comment.
can't we create impl for this type on test module and implement test related function rather then using #cfg(test) on regular impl block.
There was a problem hiding this comment.
Done. No #cfg(test) on the regular impl blocks
| // ============================================================================ | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::indexing_slicing)] |
There was a problem hiding this comment.
Please add justification comment for clippy supression.
| @@ -38,6 +42,9 @@ impl DoipSerializable for Request { | |||
| /// nothing is written to the buffer by design. | |||
There was a problem hiding this comment.
suggestion: if AliveCheckRequest carries no payload and nothing has to written to the buffer then there shouldn't be any write_to implemented for it in first place.
| /// The logical address of the tester that sent this response. | ||
| #[must_use] | ||
| pub fn source_address(&self) -> u16 { | ||
| pub fn tester_address(&self) -> u16 { |
There was a problem hiding this comment.
suggestion: if such impl is required for test only, we could impl such member functions inside mod tests {}
| /// Create a new Alive Check Response with the given tester source address. | ||
| #[must_use] | ||
| pub fn new(source_address: u16) -> Self { | ||
| Self { source_address } |
There was a problem hiding this comment.
question: I don't see any usage of source_address. Do we plan to add some validation in future on the source_address to verify if we received such response for a request that we sent?
There was a problem hiding this comment.
Its intentionally retained. Will be used in follow up PRs
There was a problem hiding this comment.
issue: Folder & file naming is not consistent with rust naming convention and rest of the changes in this PR.
There was a problem hiding this comment.
suggestion: File name can be sequence.puml or if it covers only specific sequence name it accordingly.
| Io(#[from] io::Error), | ||
|
|
||
| #[error("Configuration error: {0}")] | ||
| #[error("configuration error: {0}")] |
There was a problem hiding this comment.
question: Is this string case sensitive? How does it make a difference changing "Configuration" to "configuration"? (for the other instance as well in this file)
There was a problem hiding this comment.
The strings are not case-sensitive for matching purposes—the change is a style convention.
| use crate::doip::diagnostic_message::NackCode as DiagnosticNackCode; | ||
| use crate::doip::{ | ||
| header::GenericNackCode, routing_activation::ResponseCode as RoutingActivationCode, | ||
| }; | ||
|
|
||
| /// UDS Negative Response Codes (ISO 14229-1:2020) | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| #[repr(u8)] | ||
| enum UdsNrc { | ||
| GeneralReject = 0x10, | ||
| ServiceNotSupported = 0x11, | ||
| SubFunctionNotSupported = 0x12, | ||
| IncorrectMessageLength = 0x13, | ||
| BusyRepeatRequest = 0x21, | ||
| ConditionsNotCorrect = 0x22, | ||
| RequestSequenceError = 0x24, | ||
| RequestOutOfRange = 0x31, | ||
| SecurityAccessDenied = 0x33, | ||
| InvalidKey = 0x35, | ||
| ExceededNumberOfAttempts = 0x36, | ||
| RequiredTimeDelayNotExpired = 0x37, | ||
| ResponsePending = 0x78, | ||
| ServiceNotSupportedInActiveSession = 0x7F, | ||
| } | ||
|
|
||
| impl UdsNrc { | ||
| #[must_use] | ||
| const fn as_u8(self) -> u8 { | ||
| self as u8 | ||
| } | ||
| } |
There was a problem hiding this comment.
question: What was the reason for adding this enum, impl block here earlier and removing it now? (And also, the tests)
There was a problem hiding this comment.
UdsNrc was scaffolded in error.rs as a placeholder during early development. It was removed
| #[test] | ||
| fn uds_nrc_wire_values() { | ||
| // Verify UDS NRC codes (ISO 14229-1:2020 Table A.1) match their wire values. | ||
| let cases: &[(u8, u8)] = &[ | ||
| (0x10, 0x10), // GeneralReject | ||
| (0x11, 0x11), // ServiceNotSupported | ||
| (0x12, 0x12), // SubFunctionNotSupported | ||
| (0x13, 0x13), // IncorrectMessageLength | ||
| (0x21, 0x21), // BusyRepeatRequest | ||
| (0x22, 0x22), // ConditionsNotCorrect | ||
| (0x24, 0x24), // RequestSequenceError | ||
| (0x31, 0x31), // RequestOutOfRange | ||
| (0x33, 0x33), // SecurityAccessDenied | ||
| (0x35, 0x35), // InvalidKey | ||
| (0x36, 0x36), // ExceededNumberOfAttempts | ||
| (0x37, 0x37), // RequiredTimeDelayNotExpired | ||
| (0x78, 0x78), // ResponsePending | ||
| (0x7F, 0x7F), // ServiceNotSupportedInActiveSession | ||
| ]; | ||
| for &(nrc, expected) in cases { | ||
| assert_eq!(nrc, expected); | ||
| } |
There was a problem hiding this comment.
question: What is the use of such test?
There was a problem hiding this comment.
question: What is u64 in these 2 HashMaps? If it is for SessionId, then there must be a NewType for it.
There was a problem hiding this comment.
Agree - newType is introduced.
There was a problem hiding this comment.
Design suggestion(P1): As per the doip-server/Doc/sequence.puml my suggestion is create a type connectionBuilder and once connection is successful return another type DoipHandler with this type user can call RoutingActivationRequest , DiagnosticMessage APIs and this should support disconnection API as well, when user call disconnect , will return connectionBuilder if required for connection request again or cache some info like IP, Port address.
So basically it is based on builder pattern.
Note- suggested naming is just for example.
| } | ||
|
|
||
| participant "Tester\n(TCP Client)" as T | ||
| participant "DoipCodec\ncodec.rs" as C |
There was a problem hiding this comment.
Design(P0) : if this is interface crate then name of file not correct, please create another file which do the interface work.
| T -> C : TCP connect | ||
| C -> SM : create_session(peer_addr) | ||
| SM -> S ** : Session::new(id, addr)\nstate=Connected | ||
|
|
There was a problem hiding this comment.
Design(P0) : i don't see any type return to user if not then in that case without connection user can call Routing Activation methods / APIs without connection or session creation.
| H -> SM : remove_session(id) | ||
| SM -> S !! : Session dropped\nstate=Closed | ||
|
|
||
| @enduml No newline at end of file |
|
|
||
| == Disconnection == | ||
|
|
||
| T ->x C : TCP close / timeout |
There was a problem hiding this comment.
Disconnection part is not fully cleared, who is requesting for disconnection i guess it should be user ?
- Remove DoipSerializable impl + test from AliveCheckRequest - Rename DiagnosticAckResult -> AckResult - Move test-only helpers into mod tests (header, alive_check) - Add #[non_exhaustive] to DoipError - Make Session pub(crate); add SessionId newtype - Rename Doc/ -> doc/; update all diagrams + regenerate SVGs - Clarify sequence.puml: no user handle on connect, disconnection paths
9dca27f to
53214b3
Compare
| @@ -0,0 +1,464 @@ | |||
| @startuml ClassDiagram | |||
There was a problem hiding this comment.
file names in the doc/ folder are too generic.
@VinaykumarRS1995, I do not see the design related changes on latest commit, I can only see the flow diagram update, please check if you miss those files to push, |
Summary
This PR implements a DoIP (Diagnostic over Internet Protocol) server that allows automotive diagnostic tools to communicate with ECUs over TCP/UDP networks, following the ISO 13400-2:2019 standard. The implementation includes message parsing, session management, and a pluggable interface for connecting different UDS diagnostic backends.
All code review feedback has been addressed, including proper license headers, code duplication fixes.
Checklist