[POC] feat: add proxy-sovd crate with SOVD REST client and UDS <->SOVD mapper#12
[POC] feat: add proxy-sovd crate with SOVD REST client and UDS <->SOVD mapper#12arvindr19 wants to merge 21 commits into
Conversation
…nd workspace setup Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
|
@arvindr19 did you took a look at the client in core? https://github.com/eclipse-opensovd/opensovd-core/tree/inc/liebherr/opensovd-client If |
Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
Hi @lh-sag , I briefly glanced over the SOVD client repo, it might be useful in the future. As of now, the goal is to parse UDS requests using the MDD request/response structure and generate a mocked response. The REST API request is just a placeholder for now. I think the proxy might need something like the SOVD client, or we could potentially reuse it here. |
Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
d018b37 to
b230075
Compare
b230075 to
ee9c5c4
Compare
Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
5cf6e04 to
4c373b4
Compare
| .json() | ||
| .await | ||
| .map_err(|e| SovdError::Http(e.to_string()))?; | ||
| info!("[SOVD] Response: HTTP 200 OK"); |
There was a problem hiding this comment.
how about moving this log before data parsing.
| if p.name.contains('/') { | ||
| if let Some(prefix) = &mux_case_prefix { | ||
| if !p.name.starts_with(prefix.as_str()) { | ||
| return false; | ||
| } | ||
| } else { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
| if p.name.contains('/') { | |
| if let Some(prefix) = &mux_case_prefix { | |
| if !p.name.starts_with(prefix.as_str()) { | |
| return false; | |
| } | |
| } else { | |
| return false; | |
| } | |
| } | |
| if p.name.contains('/') { | |
| match &mux_case_prefix { | |
| Some(prefix) if name.starts_with(prefix.as_str()) => {} | |
| _ => return false, | |
| } | |
| } |
| continue; | ||
| } | ||
| // Skip params from non-matching MUX cases. | ||
| if p.name.contains('/') { |
There was a problem hiding this comment.
same here like above mentioned.
| if self.config.mock_gateway { | ||
| let token = "mock-access-token".to_string(); | ||
| *self.access_token.write().await = Some(token.clone()); | ||
| info!("[SOVD] Mock authentication enabled"); |
There was a problem hiding this comment.
you are using tracing::Info in service provider and here you directly using , please keep consistency
There was a problem hiding this comment.
yes, using tracing
bharatGoswami8
left a comment
There was a problem hiding this comment.
@arvindr19
I’ve reviewed proxy-sovd and added some comments and suggestions.
In my opinion, we should ensure tracing is used consistently across the entire codebase
Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
| .process_write_data_request(did, uds_request, &service_name, parsed_data) | ||
| .await | ||
| } | ||
| } |
There was a problem hiding this comment.
question: don't we need tests in this file?
There was a problem hiding this comment.
handle_read_did/write_did require ServiceResolver backed by a CDA EcuManager and an mdd file laoded on disk, there's no way mock for now.
| /// | ||
| /// # Errors | ||
| /// Returns an error if the read request fails. | ||
| pub async fn read_data(&self, component: &str, data_id: &str) -> Result<DataResponse> { |
There was a problem hiding this comment.
What if user interchange str value in input to component and data_id, can we use newType here.
There was a problem hiding this comment.
The call sites are both internal, always passing &self.config.ecu.default_name as the component and service_name.to_lowercase() as the data_id a swap would be immediately obvious at runtime error. Adding newtypes is just boilerplate
| &self, | ||
| component: &str, | ||
| data_id: &str, | ||
| data: serde_json::Map<String, Value>, |
There was a problem hiding this comment.
What if user interchange str value in input to component and data_id, can we use newType here.
Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
4c373b4 to
891dcb0
Compare
| self.ecu_managers.get(ecu_name).or_else(|| { | ||
| warn!( | ||
| "[ECU] Default ECU '{}' not found, falling back to first available", | ||
| ecu_name |
There was a problem hiding this comment.
why to use the first available ecu. in case ecu name is not found, can we return none?
There was a problem hiding this comment.
Removed fallback, return None if ECU name not found
| /// write requests that need request-parameter context. | ||
| pub struct SovdMapper { | ||
| /// Proxy configuration (ECU, SOVD endpoint, mock settings). | ||
| config: Config, |
There was a problem hiding this comment.
config is already stored in sovddiaghandler. why 2nd copy is maintained here?
There was a problem hiding this comment.
Updated, Extract only 4 needed fields at construction time (ecu_name, mock_gateway, gateway_url, api_version)
|
|
||
| fn ecu_manager(&self) -> Option<&ServiceResolver> { | ||
| let ecu_name = &self.config.ecu.default_name; | ||
| self.ecu_managers.get(ecu_name).or_else(|| { |
There was a problem hiding this comment.
why so you need ecu_managers as map when you want to get serviceResolver for your own ECU
If it is for any future requirement extension better to mention in comment
| .ecu_manager() | ||
| .ok_or_else(|| ProxyError::Mdd("No MDD database loaded".to_string()))?; | ||
|
|
||
| let ResolvedService { |
There was a problem hiding this comment.
USD service resolution, processing received data request and forming the response for specific request can be done in more clean way considering extendable solution approach. I understand, this is not current focus of the implementation but consider design approach where different UDS services can be implemented independently without impacting the existing source code, may be command pattern would help here, one command per serivce
There was a problem hiding this comment.
agree that we need something like this. but will plan it in future PRs
|
|
||
| //! Core types and traits shared across all proxy crates. | ||
| //! | ||
| //! Defines configuration, error types, the [`DiagHandler`] trait for |
There was a problem hiding this comment.
It was part of initial changes, removed later. Updated comment
| .or_else(|| response_data.get(short_name)) | ||
| .or_else(|| response_data.get(¶m.name.to_ascii_lowercase())) | ||
| .or_else(|| response_data.get(&short_name.to_ascii_lowercase())) | ||
| .or_else(|| response_data.get("data")); |
There was a problem hiding this comment.
The data fallback is a legitimate SOVD protocol detail: when a service returns a single scalar value, the SOVD ObjectDataItem stores it under the key data regardless of the MDD parameter name. Without this fallback, any response whose MDD param name doesn't match any key in the SOVD JSON would encode as zeroes. The constant replaces the magic string with a named, documented symbol, it doesn't change behaviour.
| /// Returns `None` when no metadata is available. On success returns the | ||
| /// encoded bytes and the effective service name (which may differ when | ||
| /// enriched MUX sibling metadata is used). | ||
| async fn encode_response_from_metadata( |
There was a problem hiding this comment.
This is very huge function, please try to split it in to meaning full smaller functions. Also some part of the code looks repetitive line 210 to 245 (client.rs 270)
There was a problem hiding this comment.
generate_mock_response_data() from client.rs will be removed once sovd-server provides real responses
There was a problem hiding this comment.
Please split this function into meaningful smaller funtions
| &self, | ||
| sid: u8, | ||
| did: u16, | ||
| _uds_bytes: &[u8], |
There was a problem hiding this comment.
does _uds_bytes really needed in future, if not remove
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| */ | ||
|
|
||
| use std::{collections::HashMap, sync::Arc}; |
There was a problem hiding this comment.
Here HashMap is used from collections where as at other places its used from cda_interfaces. No functional change but better to keep consistency with same set of interfaces unless their is any valid reason behind using it differently
There was a problem hiding this comment.
updated, using cda_interfaces
| impl Default for SovdConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| gateway_url: "http://localhost:20002".to_string(), |
There was a problem hiding this comment.
Instead of adding magic values/strings directly, add it as const or macros
There was a problem hiding this comment.
added const for config fields
Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
a4ba199 to
d00c17f
Compare
Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
Signed-off-by: Arvind Rathod <Arvind.RA.Rathod@bti.bmwgroup.com>
c24a3d2 to
d6a04c6
Compare
Summary
Adds the
proxy-sovdcrate: SOVD gateway HTTP client, UDS <-> SOVD JSON translation, and theSovdDiagHandlerimplementation.Depends on: PR #11 (proxy-core)
Changes
SovdClient: OAuth2 auth,GET /data/{service}reads,PUT /configurations/{service}writes, mock gateway mode for offline testing, MDD-driven mock response generationSovdMapper: Translates RDBI/WDBI UDS requests into SOVD REST calls and encodes JSON responses back to UDS bytes via MDD metadataSovdDiagHandler: Concrete struct with inherent pub async fn handle_read_did / handle_write_did — wires ServiceResolver + SovdMapper for end-to-end UDS processing.DataResponse: SOVD response schema (mirrorssovd_interfaces::ObjectDataItemwithDeserialize)Scope
Read (0x22) and Write (0x2E) services only. Mock gateway enabled by default.
Checklist
Related
Notes for Reviewers
Since this part of stacked PR, you could see changes from #11 , until it gets merged
Review required for
proxy-sovdcrate