-
Notifications
You must be signed in to change notification settings - Fork 7
docs: add design for abstraction layer API to be used by user code #46
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
base: main
Are you sure you want to change the base?
docs: add design for abstraction layer API to be used by user code #46
Conversation
lh-sag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stmuench I have some high level remarks to have a better understanding of this interface and the artifacts you plan to contribute. In case my remarks are too vague I suggest we discuss them in the matching work-stream.
|
|
||
| package "Design Goals" as DesignGoals {} | ||
|
|
||
| package "API Layer between user code and concrete SOVD client implementation" as DesignGoals.Goal01 {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be "and concrete SOVD server implementation"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, since this API layer is intended to be located on top of a specific client implementation (binding) which performs the actual communication with the SOVD server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stmuench so this is not the API layer between SOVD server and Service APP (-> high level architecture)? But this API layer is on Client side?
I´m a bit confused, because this PR was introduced as "This is a proposal on the API between Applications and SOVD Server". Or maybe with "Applications" it was meant applications on SOVD client side...
Please create a reference from this design to the "OpenSOVD High Level Architecture"
| package "hence, user code must provide certain pieces of information as depicted by this design upon registration of SOVD Operations and/or Data Resources" as DesignGoals.Goal02.Note01 {} | ||
|
|
||
| package "user code shall have minimal hassle w.r.t. to creating as well as having to populate native SOVD reply payloads" as DesignGoals.Goal03 {} | ||
| package "ideally, the API layer shall create native SOVD data structures and populate these from the user code's data structures, e.g. to JSON" as DesignGoals.Goal03.Note01 {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we expect, that data conversion also to be considered in the API layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, yes. Feasibility is nonetheless subject to be demonstrated.
| +With<UDSServiceType>(Identifier, Args&&...) : DiagnosticServicesCollectionBuilder& | ||
|
|
||
| .. native SOVD APIs .. | ||
| +With<ReadOnlyDataResourceType>(Identifier, JsonSchemaView, DataCategoryIdentifier, optional<DataGroupIdentifier>, Args&&...) : DiagnosticServicesCollectionBuilder& |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the DataCategoryIdentifier an output from the user in this design?
I guess it should be more a configuration item, as the DataCategory is a mandatory attribute in the SOVD response and will be not provided by the legacy UDS API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess for legacy functionality, we would simply provide some predefined category identifier implicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When querying data items from an SOVD Server, users should also see the UDS ones and not only native SOVD-exposed ones. Hence, we need to find a way to merged them, if we want that.
On the other hand, exposing legacy UDS functionality under a custom predefined category would have significantly less implementation efforts and is anyways only expected for a rather short transition period. Ultimately, all apps are subject to get migrated the native SOVD APIs.
| } | ||
|
|
||
| interface Operation { | ||
| API definition for SOVD operations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this user interface for user code on HPC? On an HPC we have typically no I/O Control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I/O Control seems to be only relevant for ECUs which have dedicated I/O ports. And HPCs typically do not have such.
71b5bd7 to
2c9baca
Compare
2c9baca to
15e2633
Compare
15e2633 to
e34629d
Compare
| <br> | ||
|
|
||
| - user code shall have minimal hassle w.r.t. to creating as well as having to populate native SOVD reply payloads | ||
| - ideally, the API layer shall create native SOVD data structures and populate these from the user code's data structures, e.g. to JSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"e.g. JSON" sounds as if there were other options. Is that the desired message?
| mw::diag::sovd::Result<mw::diag::sovd::JsonDataReply> Get() override | ||
| { | ||
| mw::diag::sovd::JsonDataReply reply; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one would need the memory_resource as well, wouldn't it?
| std::pmr::memory_resource& memory_resource_; | ||
| }; | ||
|
|
||
| mw::diag::Result<std::unique_ptr<mw::diag::DiagnosticServicesCollection>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the unique_ptr instances should carry an allocator with them that makes them pmr aware.
| DataIdentifier --|> ReadDataByIdentifier : inherits | ||
| DataIdentifier --|> WriteDataByIdentifier : inherits | ||
|
|
||
| SerializedReadDataByIdentifier --|> ReadDataByIdentifier : inherits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those guys aren't mentioned in the README. I think you should either remove them here if they're not (yet?) a thing, or mention them in the prose and elaborate on their purpose.
|
|
||
| SOVDResult ..> SOVDError : uses | ||
|
|
||
| DiagResult ..> ErrorCode : uses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrorCode --|> SCORE_ErrorCode : has underlying type
is missing here
lh-sag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this API should move to SCORE instead of opensovd.
| public: | ||
| explicit VinReader(std::pmr::memory_resource& memory_resource) : memory_resource_{memory_resource} {} | ||
|
|
||
| mw::diag::Result<ByteVector> Read() override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this in align with other SCORE projects to roll out your own Result instead of std::expected?
Summary
Checklist
Related
Notes for Reviewers