created initial rust based design draft for mw::diag by MonikaKumari26 · Pull Request #78 · eclipse-opensovd/opensovd

MonikaKumari26 · 2026-02-02T13:06:13Z

I have tested my changes locally
I have added or updated documentation
I have linked related issues or discussions
I have added or updated tests

Notes for Reviewers

docs/design/rust_abstraction_layer_api_for_user_applications.puml

stmuench · 2026-02-03T13:05:01Z

docs/design/rust_abstraction_layer_api_for_user_applications.puml

+        {abstract} +unlock(&mut self) -> Result<()>
+        {abstract} +kind(&self) -> DiagnosticEntityKind
+        {abstract} +supported_modes(&self) -> Result<Vec<DiagnosticEntityMode>>
+        {abstract} +apply_mode(&mut self, mode_id: &str, mode_value: &str, expiration_timeout: Option<Duration>) -> Result<()>


since we were discussing async APIs, maybe it would make sense here to use one

Makes sense to be async since applying a diagnostic mode might take time. supported_modes() could be also async if querying supported modes requires reading from vehicle systems.

docs/design/rust_abstraction_layer_api_for_user_applications.puml

lh-sag

@MonikaKumari26 thanks for your contribution!

docs/design/rust_abstraction_layer_api_for_user_applications.puml

lh-sag · 2026-02-05T19:48:24Z

@MonikaKumari26 I suggest to move this PR out of draft status to get more feedback.

darkwisebear

Not done, but maybe these comments are helpful still. One general comment: This library allocates a lot. We need to see whether we can cut back on allocations (or avoid them somehow?) and we need to introduce an Allocator trait, as the canonical one from std isn't stabilized yet.

docs/design/rust_abstraction_layer_api_for_user_applications.md

docs/design/rust_abstraction_layer_api_for_user_applications.puml

darkwisebear · 2026-02-10T07:40:37Z

docs/design/rust_abstraction_layer_api_for_user_applications.puml

+        +with_read_resource<T: ReadOnlyDataResource + Send + Sync + 'static>(self, id: impl Into<String>, resource: T) -> Self
+        +with_write_resource<T: WritableDataResource + Send + Sync + 'static>(self, id: impl Into<String>, resource: T) -> Self
+        +with_data_resource<T: DataResource + Send + Sync + 'static>(self, id: impl Into<String>, resource: T) -> Self
+        +with_operation<T: Operation + Send + Sync + 'static>(self, id: impl Into<String>, operation: T) -> Self


What's the reason for these types to be Sync?

My understanding is that these references must be Sync to allow concurrent read access from multiple threads, ensuring safe shared diagnostic operations and no compiler errors when shared via &T.

But that'd only be relevant if I wanted to execute the same operation concurrently. Is that really the case? Imo executing an operation should be exclusive, which means that T needs to be Send (so that an instance of T can be executed in different threads), but not Sync (because no concurrent execution is required).

Regarding the concurrency constraints, I initially used uniform Send + Sync requirement for simplicity, but I have now refined this to be more precise based on resource behavior:

Operation Trait: Retained Send + Sync. Since it exposes both &self and &mut self methods, Sync is required to allow concurrent status queries via immutable references.

Write-only resources/services, RoutineControl and UdsService to be Send only, as they don't benefit from concurrent access.

Read-only Resources: Retained Send + Sync to explicitly support concurrent reads.

Design may need further adjustments if these Sync requirements conflict with specific downstream use cases.

&self / &mut self are static ownership properties that help the compiler to find out whether it's possible to allow for modifications of the instance state in a certain context (imo &mut should have been called &unique).

In conjunction with concurrency traits, it helps the compiler to know whether copying a reference across thread boundaries preserves these properties. However, in case of e.g. Operation types, even if they're Sync, the method signature of e.g. execute requires &mut self, so you statically demand a unique ownership. This either requires a mutex or something similar to go from shared to unique ownership if this exclusivity is enforced at runtime, or even a static unique ownership, which is hard to obtain unless you completely handle all diag events in a single thread. But since you already demand unique ownership, requiring the type to be Sync is asked too much as you do not need to synchronize anyway if ownership is already unique.

The bottom line for me is that if we have &mut self methods, requiring Sync doesn't help at all. Sync only makes sense if most / all methods are &self so that you can call them from multiple threads truly concurrently. Deciding this means to think whether synchronization shall be done by all implementors of Operation and the like (which makes these types Sync, but allows for fine grained synchronization if required), or whether the framework will take care, which makes implementing the diagnostic structs easier, but the synchronization will be more coarse grained.

docs/design/rust_abstraction_layer_api_for_user_applications.puml

docs/design/rust_abstraction_layer_api_for_user_applications.md

lh-sag

@MonikaKumari26 if you want broader feedback I would advise you to move this PR from draft to final.

docs/design/rust_abstraction_layer_api_for_user_applications.md

docs/design/rust_abstraction_layer_api_for_user_applications.puml

docs/design/rust_abstraction_layer_api_for_user_applications.md

lh-sag · 2026-02-19T07:31:57Z

docs/design/rust_abstraction_layer_api_for_user_applications.md

+**Important**: This design is intended for automated code generation. Key constraints:
+
+- **Rust Edition**: 2021+ (minimum required)
+- **Async Strategy**: All trait methods are currently **synchronous**. While shown in design for API clarity, the current implementation does not use async. Future versions may add async support using `#[async_trait]` macro.


IMHO it is easier to have async fn first and support blocking on top. What is the rational to start with sync first?

I think that the original statement in the document is too generic. We need to look at the methods one by one and determine their nature:

Operation

Most methods query state; these methods are nonblocking and there is imo no reason to make them async, not even in a second step, as such data acquisition should not require any long running activities. The same is true for the execute method: This one also isn't supposed to block. If it does need to do something that takes longer, it is supposed to spawn a concurrent task or thread and return with a pending state.

The only notable exception in my eyes is stop, because I could imagine that stopping a task might incur some waiting. I'd need to check the SOVD spec whether or not we'd need to delay the response of this method until the task really stopped.

ReadableDataResource

I think it makes sense to make the get call async, as it is absolutely thinkable that data acquisition will be done asynchronously by the implementation.

WritableDataResource

Here, the same argumentation as for the execute call holds, unless it is required by the spec to also tell whether setting the data actually happened (aka a positive response not just confirms data reception but also confirms proper changing of the inner state). In that case, the put method also needs to be async.

Your implications about the system might fit for yours but we need an async interface for ours. It is just a matter of how we implement it.

I don't think so. Let's take Operation: If you opt for an API design (and that's how it looks) where you map SOVD requests 1:1 to API calls, the resulting API and the way how you implement the trait should give you an idea about what color your function is.

If you disagree with my assessments, please give your view of that particular method and why it should or shouldn't be async, or you also mention why the suggested API design philosophy (have a thin layer above the REST calls) is not the correct one.

Yes. And our operations/API is fully async this does not mean write will work differently as you suggested but our core is async. This is completely orthogonal to SOVD.

Please note that I do not enforce either way sync or async, just that we have an async interface as our system underneath work differently than yours.

Imo the API should already be usable for you as well without having to jump through hoops; either we will have a truly async method anyway (e.g. get of ReadableDataResource), or the methods are ordinary ones without suspension points, but these shall be (see my comment above) nonblocking, so that they can also be called in an async context. So even if you are fully async, this shouldn't prevent you from being able to implement this API.

On a side note, we shouldn't actually talk about "your system" or "our system". In the end we want to build something that can be used in many different contexts, and if there are contexts where the API is not a good fit, I'd need to understand what exactly isn't fitting so that we can develop a common solution that fits.

On a side note, we shouldn't actually talk about "your system" or "our system". In the end we want to build something that can be used in many different contexts, and if there are contexts where the API is not a good fit, I'd need to understand what exactly isn't fitting so that we can develop a common solution that fits.

We do agree here. But since our systems differs we are biased, I doubt we would have argued if this is not the case. Therefore my initial remark was whether we have a fully sync and async interface.

Where we differ is having a mixed one e.g. I do not see why for example Operation::status should not be colored async. The question about async/sync interface is independent of how SOVD works.

I will join next weeks UDS2SOVD meeting in case you want to discuss this outside of this PR.

docs/design/rust_abstraction_layer_api_for_user_applications.md

docs/design/rust_abstraction_layer_api_for_user_applications.puml

MonikaKumari26 · 2026-02-27T13:52:50Z

docs/design/rust_abstraction_layer_api_for_user_applications.puml

+    interface Operation <<trait>> {
+        API definition for SOVD operations
+
+        .. standard SOVD capabilities ..
+        {abstract} +async info(&self, request: DiagnosticRequest) -> Result<OperationInfoReply>
+        {abstract} +async status(&self, request: DiagnosticRequest) -> Result<OperationStatusReply>
+        {abstract} +async execute(&mut self, request: DiagnosticRequest) -> Result<ExecuteOperationReply>
+        {abstract} +async resume(&mut self, request: DiagnosticRequest) -> Result<ExecuteOperationReply>
+        {abstract} +async reset(&mut self, request: DiagnosticRequest) -> Result<ExecuteOperationReply>
+        {abstract} +async stop(&mut self, request: DiagnosticRequest) -> Result<()>
+


The API must follow synchronous non-blocking pattern here. The implementation should avoid the overhead of a multi-threaded async runtime while also ensuring functions do not halt execution while waiting. Instead, operations should return immediately with a status (WouldBlock or InProgress), requiring the caller to poll for completion.

Keeping info() and status() as async makes sense because info may involve remote metadata fetches and status polling aligns with the REST async‑polling pattern and works naturally as async.

docs/design/rust_abstraction_layer_api_for_user_applications.puml

MonikaKumari26 · 2026-03-02T06:44:30Z

docs/design/rust_abstraction_layer_api_for_user_applications.puml

+    interface WritableDataResource <<trait>> {
+        API definition for writable SOVD data resources
+
+        {abstract} +async put(&mut self, request: DiagnosticRequest) -> Result<()>


Write should return status immediately and long‑running operations should expose a status that callers can poll. Convert write to a synchronous, non‑blocking API that returns a status enum.

Suggested change

{abstract} +async put(&mut self, request: DiagnosticRequest) -> Result<()>

fn write(&mut self, request: DiagnosticRequest) -> Result<WriteStatus>

enum WriteStatus {

Completed, // Write finished successfully

InProgress, // Write initiated, still processing

WouldBlock, // Cannot start now, retry later

}

Who would consume/poll the write status?

I would also like to understand why we drop async. It can have the same semantic as you mentioned here but allows for async i/o. We will have a sync meeting tomorrow. feel free to discuss this further.

stmuench reviewed Feb 3, 2026

View reviewed changes

lh-sag reviewed Feb 4, 2026

View reviewed changes

MonikaKumari26 force-pushed the rust_abstract_layer_api_for_user_code branch 2 times, most recently from a87978f to e9e5e95 Compare February 5, 2026 11:38

lh-sag self-requested a review February 5, 2026 19:47

MonikaKumari26 force-pushed the rust_abstract_layer_api_for_user_code branch from e9e5e95 to 82693a9 Compare February 6, 2026 12:29

darkwisebear reviewed Feb 10, 2026

View reviewed changes

MonikaKumari26 force-pushed the rust_abstract_layer_api_for_user_code branch from 82693a9 to 887af25 Compare February 11, 2026 08:56

darkwisebear reviewed Feb 12, 2026

View reviewed changes

docs/design/rust_abstraction_layer_api_for_user_applications.md Outdated Show resolved Hide resolved

MonikaKumari26 force-pushed the rust_abstract_layer_api_for_user_code branch from 887af25 to 7825f43 Compare February 13, 2026 12:42

lh-sag reviewed Feb 19, 2026

View reviewed changes

MonikaKumari26 commented Feb 25, 2026

View reviewed changes

docs/design/rust_abstraction_layer_api_for_user_applications.puml Outdated Show resolved Hide resolved

MonikaKumari26 commented Feb 27, 2026

View reviewed changes

docs/design/rust_abstraction_layer_api_for_user_applications.puml Outdated Show resolved Hide resolved

MonikaKumari26 commented Feb 27, 2026

View reviewed changes

MonikaKumari26 commented Mar 2, 2026

View reviewed changes

created intial rust based design draft for mw::diag

6086c37

MonikaKumari26 force-pushed the rust_abstract_layer_api_for_user_code branch from 7825f43 to 6086c37 Compare March 3, 2026 07:17

-        {abstract} +async put(&mut self, request: DiagnosticRequest) -> Result<()>
+      fn write(&mut self, request: DiagnosticRequest) -> Result<WriteStatus>
+enum WriteStatus {
+    Completed,   // Write finished successfully
+    InProgress,  // Write initiated, still processing
+    WouldBlock,  // Cannot start now, retry later
+}

Conversation

MonikaKumari26 commented Feb 2, 2026

Related

Notes for Reviewers

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lh-sag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lh-sag commented Feb 5, 2026

Uh oh!

darkwisebear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MonikaKumari26 Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lh-sag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MonikaKumari26 Feb 11, 2026 •

edited

Loading