Min/Max Support

[erikbosch]

This PR adds the following functionality

• When reading a VSS JSON file min/max values are read and stored as well.
• When requesting metadata you also get min/max metadata
• When setting a datapoint min/max check is performed (min <= value <= max)

Some tests have been performed using a custom VSS JSON file.

[codecov]

Codecov Report

Attention: Patch coverage is 81.85452% with 227 lines in your changes missing coverage. Please review.

Project coverage is 58.84%. Comparing base (a8df2f6) to head (78e1adc).
Report is 4 commits behind head on feature/databroker-api-v2.

Files with missing lines	Patch %	Lines
databroker/src/grpc/kuksa_val_v2/conversions.rs	23.71%	74 Missing ⚠️
databroker/src/grpc/kuksa_val_v1/val.rs	37.14%	66 Missing ⚠️
databroker/src/broker.rs	93.48%	54 Missing ⚠️
databroker/src/vss.rs	52.00%	12 Missing ⚠️
databroker/src/main.rs	0.00%	9 Missing ⚠️
databroker/src/grpc/sdv_databroker_v1/collector.rs	0.00%	6 Missing ⚠️
databroker/src/grpc/kuksa_val_v2/val.rs	98.78%	2 Missing ⚠️
databroker/src/grpc/sdv_databroker_v1/broker.rs	0.00%	2 Missing ⚠️
databroker/src/viss/v2/server.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@                      Coverage Diff                      @@
##           feature/databroker-api-v2      #79      +/-   ##
=============================================================
+ Coverage                      55.33%   58.84%   +3.51%     
=============================================================
  Files                             33       33              
  Lines                          14677    15531     +854     
=============================================================
+ Hits                            8121     9139    +1018     
+ Misses                          6556     6392     -164     

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

[lukasmittag]

should we add some tests?

[erikbosch]

should we add some tests?

Some tests coming soon.

[erikbosch]

I have now added some test cases, but they do not cover all possible VSS/Databroker Datatypes. We have quite much duplication in code (and tests) due to the strong type checks in Rust, so not that easy to reuse. I think the level of tests are quite reasonable now.

[rafaeling]

Looks good to me, apart from some general comments like why DataValue does not implement Int8 and Uint8, are we upcasting values to int32/64 and uint32/64?
Also looks good unit test coverage for kuksa.val.v2
Thanks Erik :)

[rafaeling]

Just saw that greater_than function:

kuksa-databroker/databroker/src/types.rs

Line 85 in 671fc59

(DataValue::Int32(value), DataValue::Int32(other_value)) => Ok(value > other_value),

does not support all possible data types in DataType

kuksa-databroker/databroker/src/types.rs

Line 20 in 671fc59

Int8,

Even DataValue does not support all of them:

kuksa-databroker/databroker/src/types.rs

Line 59 in 671fc59

pub enum DataValue {

What is the reason for that? Is that on purpose?

[erikbosch]

That is better a question for @argerus. I have some theories - one is that we do not "save" that much memory by using u32 instead of u16/u8 and i32 instead of i8/i16, another is that it simplifies implementation as we do not need to handle that many combinations.

[lukasmittag]

https://groups.google.com/g/protobuf-c/c/UzcyFvEOvMY

[lukasmittag]

https://protobuf.dev/programming-guides/proto3/#scalar

[lukasmittag]

there's no int8 in proto3

[argerus]

That is better a question for @argerus. I have some theories - one is that we do not "save" that much memory by using u32 instead of u16/u8 and i32 instead of i8/i16, another is that it simplifies implementation as we do not need to handle that many combinations.

Exactly. It reduces the combinatorial explosion in the implementation and doesn't really cost anything memory wise since they are all part of the same enum anyway. Using 32 & 64 bit integers might even be marginally more efficient cpu wise (probably doesn't matter much). And as @lukasmittag mentions, it would need to be converted anyway when using protobuf.

The VSS data type is enforced when setting a value, and is not encoded by the value enum itself. This simplifies setting a value to NotAvailable as well.

[rafaeling]

Got it, thanks