Doip server 1.0#29
Conversation
Document DoIP server block diagram, supported message types, and build instructions. Include PlantUML diagrams for startup, TCP connection, UDP request, and graceful shutdown flows. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Define crate as library and binary. Add tokio, serde, toml, tracing, and thiserror dependencies. Declare top-level modules. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Introduce ServerConfig for TCP, UDP, and ECU identity settings. Provide InMemoryProvider for defaults and TomlProvider for file-based configuration loading. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Implement 8-byte header parsing with version validation. Define TcpPayloadType and UdpPayloadType enums for all supported message types (0x0001-0x8003) per ISO 13400-2. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Introduce PayloadHandler<PayloadType, Request> trait for handler implementations. Build Dispatcher with HashMap-based routing from payload type to handler, generic over transport. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Define SovdProxy trait with forward() for UDS byte translation. StubProxy returns NRC 0x11 pending real backend integration. MockProxy echoes input for unit testing. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Add handlers for VehicleIdentification (0x0001-0x0003), VIN request (0x0004), and EntityStatus (0x4001). Vehicle ID handlers share common announcement-building logic. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Add AliveCheck (0x0007) responding to keep-alive pings. RoutingActivation (0x0005) returns 0x10 as placeholder until full state-machine logic is implemented. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Add DiagnosticsHandler (0x8001) forwarding UDS bytes via SovdProxy and returning DiagnosticMessagePositiveAck. Wire all handlers into build_tcp_dispatcher() and build_udp_dispatcher(). Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Add TCP listener with per-connection task spawning. DoIP framer reconstructs complete messages from partial reads. UDP uses simple recv-parse-dispatch loop with no framing needed. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Enforce max concurrent TCP connections using atomic counter. SessionSlot acts as RAII guard that auto-releases on drop, ensuring cleanup on disconnection or task cancellation. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Add Server struct that starts both listeners concurrently and coordinates graceful shutdown via cancellation token. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
Wire all layers in main(): load config, build dispatchers, start transports, handle SIGINT shutdown. Include DoIP tester for manual integration testing against a running server. Signed-off-by: VinaykumarRS1995 <Vinaykumarrs1995@gmail.com>
| session resources released automatically | ||
| end note | ||
|
|
||
| @enduml No newline at end of file |
bharatGoswami8
left a comment
There was a problem hiding this comment.
I believe we should consider providing users with greater flexibility to build each component independently. By components, I am referring to the server application, DoIP library, and client example application.
Additionally, I request the following improvements:
- Please update the README to clearly reflect the intended usage and component structure.
- Review the codebase to identify areas where naming conventions and error handling can be improved for better clarity and consistency.
- Ensure that types defined within a crate are not unnecessarily exposed using pub unless required.
There was a problem hiding this comment.
where is your lib build ?
I would suggest keep seperate toml file for both app and lib and from main toml you can build both.
There was a problem hiding this comment.
Thanks for adding separate toml file for app in same way we need to create toml file for src folder which is library build,
| It accepts UDS requests over DoIP (Diagnostics over IP), resolves the corresponding SOVD service | ||
| using the diagnostic description (MDD) of the ECU, and translates them into SOVD REST API calls. | ||
| The SOVD responses are then encoded back into UDS format and returned to the requesting tool. | ||
| It accepts UDS requests over DoIP (Diagnostics over IP, [ISO 13400-2](https://www.iso.org/standard/74785.html)), resolves the corresponding SOVD service using the diagnostic description (MDD) of the ECU, and translates them into SOVD REST API calls. The SOVD responses are then encoded back into UDS format and returned to the requesting tool. |
There was a problem hiding this comment.
diagnostic description (MDD) ? what is M here ?
There was a problem hiding this comment.
this will be updated in future integration, deleting for now
| //! from different sources (in-memory, TOML file). | ||
|
|
||
| pub mod in_memory; | ||
| pub mod toml; |
There was a problem hiding this comment.
toml is not ideal name in my opinion.
There was a problem hiding this comment.
I have aliased as Toml_provider.
| }; | ||
|
|
||
| /// 17 (VIN) + 2 (addr) + 6 (EID) + 6 (GID) + 1 (action byte) = 32 | ||
| pub(super) const VI_RESPONSE_LEN: usize = 32; |
There was a problem hiding this comment.
how about keeping in constants ?
There was a problem hiding this comment.
Good point. VI_RESPONE_LEN is only used in create_vi_response() in common.rs so its local implementation rather than protocol level constant.
There was a problem hiding this comment.
Agree. It doesn't need to public: I've made as Private.
Thank you
There was a problem hiding this comment.
Agreed. A separate Cargo.toml has been added for the client, It's now its own binary crate in the workspace.
| pub const NACK_OUT_OF_MEMORY: u8 = 0x03; | ||
|
|
||
| /// Payload length field does not match actual payload size. | ||
| pub const NACK_INVALID_PAYLOAD_LENGTH: u8 = 0x04; |
There was a problem hiding this comment.
thought: These are different variation of Nack. I think it is better to group them in an enum and belong to message.rs like TcpPayloadType & UdpPayloadType.
There was a problem hiding this comment.
Agree. Grouping the NACK codes into an enum would be cleaner. Will add as a future improvement.
There was a problem hiding this comment.
I think introducing an enum makes code better with respect to readability, collection of similar things into one, keeps related concerns together and avoid disinformation. We can consider incorporating this instead of differing if not a huge effort.
There was a problem hiding this comment.
Yes, I’ve created the Enum in message.rs and grouped the NACK codes there for better readability and type safety.
| let result = dispatcher.dispatch(make_req(TcpPayloadType::DiagnosticMessage)); | ||
| assert!(matches!(result, Err(Error::UnknownPayloadType(0x8001)))); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: There are no tests to show that we can't register UdpHandler to TcpDispatcher or vice-versa.
There was a problem hiding this comment.
If you try to register a UDP handler on a TCP dispatcher, the code simply won't compile — Rust's type system enforces this at compile time. I can add a compile_fail doc test to document this. Open for suggestions.
There was a problem hiding this comment.
That would help future developer as registering a handler to dispatcher is critical part of extension.
There was a problem hiding this comment.
nitpick: I guess you didn't need to remove the existing entries in this file.
VinaykumarRS1995
left a comment
There was a problem hiding this comment.
I have incorporated the review feedback and pushed the updates. Thanks for taking the time to review the changes.
| session resources released automatically | ||
| end note | ||
|
|
||
| @enduml No newline at end of file |
| It accepts UDS requests over DoIP (Diagnostics over IP), resolves the corresponding SOVD service | ||
| using the diagnostic description (MDD) of the ECU, and translates them into SOVD REST API calls. | ||
| The SOVD responses are then encoded back into UDS format and returned to the requesting tool. | ||
| It accepts UDS requests over DoIP (Diagnostics over IP, [ISO 13400-2](https://www.iso.org/standard/74785.html)), resolves the corresponding SOVD service using the diagnostic description (MDD) of the ECU, and translates them into SOVD REST API calls. The SOVD responses are then encoded back into UDS format and returned to the requesting tool. |
There was a problem hiding this comment.
this will be updated in future integration, deleting for now
| let src = u16::from_be_bytes([req.payload()[0], req.payload()[1]]); | ||
| let tgt = u16::from_be_bytes([req.payload()[2], req.payload()[3]]); | ||
| self.forward(src, tgt, &req.payload()[4..]) |
There was a problem hiding this comment.
IMO, variable names must be descriptive and there shouldn't any scope for (mis)interpretation. It is okay to use abbreviations if it is well known (widely used and adopted) acronym like TCP or UDP.
14f01bf to
c89333a
Compare
| } | ||
| </style> | ||
|
|
||
| participant "Main" as Main |
There was a problem hiding this comment.
I think the Main you mentioned which is from app (main) which is like doip server app, please change from main to server app or some other proper name.
There was a problem hiding this comment.
agree. Updated as doip server app
There was a problem hiding this comment.
I think you no need to create sequence for shutdown because we do not have signal handling.
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub(super) mod fixtures { |
There was a problem hiding this comment.
I did not get this why test related data is in pub
There was a problem hiding this comment.
The test data is inside a cfg(test) module and marked pub(super) only so other test modules(in request.rs , request_by_eid.rs and request_by_vin.rs) can reuse it without duplicating the same VIN/EID/GID setup. That visibility is limited to the parent module tree, so it is not exposed outside the vehicle identification tests.
| /// [25..31] GID | ||
| /// [31] further action required (0x00 = none) | ||
| /// ``` | ||
| pub(super) fn create_vi_response( |
There was a problem hiding this comment.
no test for this function, and why this is free function ?
There was a problem hiding this comment.
Since this is a helper function, it only combines the EcuConfig and logical address into a response payload.
I agree that there is no direct unit test for this function, but its behavior is indirectly covered by the handler tests.
|
|
||
| //! Handler for VehicleIdentificationRequest (0x0001, ISO 13400-2 §7.6.1). | ||
|
|
||
| use super::common::create_vi_response; |
There was a problem hiding this comment.
Why we want to use free function ?
There was a problem hiding this comment.
Because the logic is stateless, small, and shared across multiple handlers, it is kept in one place to avoid duplication. To keep the code simple and maintainable, we implemented it as a free function.
| req.payload()[5], | ||
| ]); | ||
| if requested != self.ecu_config.eid() { | ||
| return Err(Error::NoMatch); |
There was a problem hiding this comment.
| return Err(Error::NoMatch); | |
| return Err(Error::EIDNotMatched); |
There was a problem hiding this comment.
I agree, clearer naming.
| ``` | ||
| 2. Or with a TOML config file (see [`sample-doip-server.toml`](sample-doip-server.toml)): | ||
| ```sh | ||
| cargo run -p doip-server -- path/to/config.toml |
There was a problem hiding this comment.
| cargo run -p doip-server -- path/to/config.toml | |
| cargo run -p doip-server -- <path of config toml file> |
| ### testing | ||
| ### Testing | ||
|
|
||
| #### unit tests | ||
| #### Unit Tests | ||
|
|
||
| Unittests are placed in the relevant module as usual in rust: | ||
| Unit tests are placed in the relevant module as usual in Rust: | ||
| ```rust | ||
| ... | ||
| #[cfg(test)] | ||
| mod test { | ||
| mod tests { | ||
| ... | ||
| } | ||
| ``` | ||
|
|
||
| Run unit tests with: | ||
| ```shell | ||
| ```sh | ||
| cargo test --locked --lib |
There was a problem hiding this comment.
I don't know whether we should keep this in README file or not, please check once.
There was a problem hiding this comment.
I placed same place as CDA
|
|
||
| Start the proxy, then run the E2E tester: | ||
| ```sh | ||
| cargo run -p doip-server & |
There was a problem hiding this comment.
why &, you should ask user to run in separate terminal to debug and check logs.
bharatGoswami8
left a comment
There was a problem hiding this comment.
@VinaykumarRS1995 , please check some latest comment and reply to older comment which is not addressed.
- Convert to Cargo workspace (lib + app + client) - Narrow tokio features to specific set - Refactor handlers to accept EcuConfig directly - Tighten visibility (pub(super), pub(in crate::...)) - Remove DEFAULT_ prefix in defaults module - Add compile_fail doc test for transport segregation - Add TODO comments for future improvements - Update README (headings, commands, abbreviations) - Add sample-doip-server.toml - Regenerate SVGs from updated puml sources
c89333a to
a0fd1bf
Compare
Summary
This PR adds a DoIP server (ISO 13400-2) that sits between a UDS diagnostic tester and a SOVD backend. It handles vehicle discovery over UDP and diagnostic sessions over TCP, parsing the DoIP wire format and dispatching to per-message-type handlers.
The SOVD proxy layer is stubbed for now — it returns NRC 0x11 (serviceNotSupported). The idea is that once the real SOVD backend is available, we just swap in a concrete implementation of the
SovdProxytrait without touching any protocol or transport code.README.md has the full architecture diagram, message type table, and build/run instructions.
Checklist
Related
Future Improvement #9
Notes for Reviewers
I've structured the commits to be reviewable layer by layer. If you follow the commit order, each one builds on the previous without jumping between concerns:
7a. UDP handlers (vehicle identification, entity status)
7b. Stateless TCP handlers (alive check, routing activation)
7c. DiagnosticsHandler + dispatcher factory wiring
8a. Async TCP/UDP transport with DoIP framing
8b. Session manager (atomic counter + RAII guard)
8c. Server struct tying both transports together