Create a ClientTrait for RustSDK #142

lukasmittag · 2025-02-26T09:57:11Z

For handling the Rust SDK and preparing to make it an own crate this introduces a ClientTrait. With this the maintenance of the soon-to-be Rust SDK will be easy and manageable.

codecov · 2025-02-26T10:32:55Z

Codecov Report

Attention: Patch coverage is 17.52988% with 207 lines in your changes missing coverage. Please review.

Project coverage is 59.47%. Comparing base (c8c1a60) to head (df25af6).

Files with missing lines	Patch %	Lines
databroker-cli/src/kuksa_cli.rs	0.00%	90 Missing ⚠️
lib/sdv/src/lib.rs	0.00%	65 Missing ⚠️
lib/kuksa/src/lib.rs	46.31%	51 Missing ⚠️
databroker-cli/src/sdv_cli.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
+ Coverage   59.38%   59.47%   +0.09%     
==========================================
  Files          34       34              
  Lines       15137    15085      -52     
==========================================
- Hits         8989     8972      -17     
+ Misses       6148     6113      -35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wba2hi

no functional test has been done yet.

wba2hi · 2025-02-26T12:46:42Z

lib/kuksa/src/lib.rs

+    type DatapointType = HashMap<String, proto::v1::Datapoint>;
+    type PathType = Vec<String>;
+    type SubscribeType = Self::PathType;
+    type PublishResponseType = ();
+    type GetResponseType = Vec<DataEntry>;
+    type SubscribeResponseType = tonic::Streaming<proto::v1::SubscribeResponse>;
+    type ProvideResponseType = tonic::Streaming<proto::v1::SubscribeResponse>;
+    type ActuateResponseType = ();
+    type MetadataResponseType = Vec<DataEntry>;


I understand that the types are closely coupled to the parameters / return values of the corresponding grpc interface methods but right now the types selected here feel arbritrarily chosen and not really fitting to their "type alias".

Maybe this is just a naming thingy and clarity can be increased by making clear what kind of type they are, like ParameterType, ReturnType,...

Like, why is a DatapointType a HashMap<String, Datapoint>? I would more expect like DatapointType to be proto::v1::Datapoint and then somewhere it uses HashMap<String, DatapointType> or similar.
A PathType could be a String, and sometimes (e.g. get) it's simply used as PathType and sometimes (e.g. subscribe) it's used as Vec<PathType>. However I did not play this through if this model would hold for all the implementors (sdv, val.v1, val.v2).

Please think a bit more about how to increase clarity e.g. by renaming the ...Typeor by aligning the implementations.

wba2hi · 2025-02-26T13:10:08Z

lib/common/src/lib.rs

+    type MetadataResponseType;
+
+    // from deeply embedded layer providing sensor values (to keep backwards compatibility the naming is different for the corresponding interfaces)
+    // if we do not want to put in the effort just give an unimplented error for the function


Suggested change

// if we do not want to put in the effort just give an unimplented error for the function

// if we do not want to put in the effort just give an unimplemented error for the function

It's a weird suggestion to make :D do we expect anywhere that this would apply, e.g. in sdv? shouldn't we at least make sure for val.v1 and val.v2 that the function set should be similar and implemented (exceptions of course do apply e.g. for the target_value thingies which no longer exist in v2, but maybe this means we shouldn't expose it in ClientTrait anymore?)

If I understood correctly the ClientTrait methods will just call the existing methods, like ClientTrait#publish would call val.v1 set with Field.VALUE while ClientTrait#actuate would call set with Field.TARGET_VALUE

wba2hi · 2025-02-26T13:12:08Z

lib/common/src/lib.rs

+    ) -> Result<Self::PublishResponseType, ClientError>;
+
+    // from application getting sensor values (to keep backwards compatibility the naming is different for the corresponding interfaces)
+    // if we do not want to put in the effort just give an unimplented error for the function


Suggested change

// if we do not want to put in the effort just give an unimplented error for the function

// if we do not want to put in the effort just give an unimplemented error for the function

wba2hi · 2025-02-26T13:13:11Z

lib/common/src/lib.rs

+    // from povider side pick up actuation requests (to keep backwards compatibility the naming is different for the corresponding interfaces)
+    // if we do not want to put in the effort just give an unimplented error for the function


Suggested change

// from povider side pick up actuation requests (to keep backwards compatibility the naming is different for the corresponding interfaces)

// if we do not want to put in the effort just give an unimplented error for the function

// from provider side: pick up actuation requests (to keep backwards compatibility the naming is different for the corresponding interfaces)

// if we do not want to put in the effort just give an unimplemented error for the function

wba2hi · 2025-02-26T13:16:06Z

lib/common/src/lib.rs

+    // from povider side pick up actuation requests (to keep backwards compatibility the naming is different for the corresponding interfaces)
+    // if we do not want to put in the effort just give an unimplented error for the function


Suggested change

// from povider side pick up actuation requests (to keep backwards compatibility the naming is different for the corresponding interfaces)

// if we do not want to put in the effort just give an unimplented error for the function

// from provider side: pick up actuation requests (to keep backwards compatibility the naming is different for the corresponding interfaces)

// if we do not want to put in the effort just give an unimplemented error for the function

wba2hi · 2025-02-26T13:33:23Z

databroker-cli/src/kuksa_cli.rs

-            None
-        }
-    };
+    let datapoint_entries = handle_get_metadata(vec![path], client).await.unwrap();


Wouldn't simply unwrapping the result here crash the application in case of an err? might be a rare case though as this only happens when cli::print_error(...) or prin_resp_err_fm fails - might be neglectable.

wba2hi · 2025-02-26T13:48:00Z

databroker-cli/src/kuksa_cli.rs

@@ -329,20 +331,9 @@ pub async fn kuksa_main(_cli: Cli) -> Result<(), Box<dyn std::error::Error>> {

                    let pattern = vec!["**"];


optional: could be inlined

databroker-cli/src/kuksa_cli.rs

wba2hi · 2025-02-26T14:01:47Z

databroker-cli/src/kuksa_cli.rs

+                                            );
+                                        } else {
+                                            let name = entry.path.clone();
+                                            println!("No entry metadata for {name}");


Suggested change

println!("No entry metadata for {name}");

println!("No metadata entry for {name}");

wba2hi · 2025-02-26T14:07:01Z

lib/kuksa/src/lib.rs

+        &mut self,
+        paths: Self::PathType,
+    ) -> Result<Self::ProvideResponseType, ClientError> {
+        println!("Actuation concept has changed with kuksa.val.v2 only supported by it! Defaulting to subscribing to target values.");


should the sdk use println! or better one of the log macros, like warn!? mutiple occurrences

hmm warn would be more suitable yes.

rafaeling · 2025-02-28T09:29:56Z

lib/common/src/lib.rs

+    type MetadataResponseType;
+
+    // from deeply embedded layer providing sensor values (to keep backwards compatibility the naming is different for the corresponding interfaces)
+    // if we do not want to put in the effort just give an unimplemented error for the function


Really like the idea of having traits here, just one thing:
what about splitting interfaces? like creating a generic interface which cover common cases from all the APIs like subscribe, publish, etc and an specific traits which covers the target_values use cases, etc?
Something like:
trait GenericSDK { fn subscribe(&self); }
trait TargetValueSDK: GenericSDK { fn set_target_values(&self); }

If I am a sdk user for kuksa_val_v2, it would be confuse to see set_target_values, etc methods

hmm yes but I thought a little bit about managing backwards compatibility. For the sdk agree that it looks strange for people. And for now we might not need backwards compatibility. However I will think about generic sdk and additions.

lukasmittag added 2 commits February 26, 2025 11:24

Create a ClientTrait for RustSDK

83ab5ab

Fix clippy finding

a48ef1e

lukasmittag force-pushed the feature/rust_sdk_client_trait branch from 46bd034 to ddbc538 Compare February 26, 2025 10:24

Use ClientTrait in integration tests for databroker

bae6f53

lukasmittag force-pushed the feature/rust_sdk_client_trait branch from ddbc538 to bae6f53 Compare February 26, 2025 10:27

wba2hi requested changes Feb 26, 2025

View reviewed changes

lukasmittag added 2 commits February 27, 2025 09:04

Fix comments

0d25a30

Rename types for Actuation and Sensor providing

df25af6

rafaeling reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a ClientTrait for RustSDK #142

Create a ClientTrait for RustSDK #142

lukasmittag commented Feb 26, 2025

codecov bot commented Feb 26, 2025 •

edited

Loading

wba2hi left a comment

wba2hi Feb 26, 2025

wba2hi Feb 26, 2025

wba2hi Feb 26, 2025

wba2hi Feb 26, 2025

wba2hi Feb 26, 2025

wba2hi Feb 26, 2025

wba2hi Feb 26, 2025

wba2hi Feb 26, 2025

wba2hi Feb 26, 2025

lukasmittag Feb 26, 2025

rafaeling Feb 28, 2025 •

edited

Loading

lukasmittag Mar 3, 2025

	// if we do not want to put in the effort just give an unimplented error for the function
	// if we do not want to put in the effort just give an unimplemented error for the function

		// from povider side pick up actuation requests (to keep backwards compatibility the naming is different for the corresponding interfaces)
		// if we do not want to put in the effort just give an unimplented error for the function

		@@ -329,20 +331,9 @@ pub async fn kuksa_main(_cli: Cli) -> Result<(), Box<dyn std::error::Error>> {

		let pattern = vec!["**"];

	println!("No entry metadata for {name}");
	println!("No metadata entry for {name}");

Create a ClientTrait for RustSDK #142

Are you sure you want to change the base?

Create a ClientTrait for RustSDK #142

Conversation

lukasmittag commented Feb 26, 2025

codecov bot commented Feb 26, 2025 • edited Loading

Codecov Report

wba2hi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaeling Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 26, 2025 •

edited

Loading

rafaeling Feb 28, 2025 •

edited

Loading