Databroker API V2 #33

rafaeling · 2024-05-16T10:00:51Z

No description provided.

codecov · 2024-05-16T10:06:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.77%. Comparing base (e52b690) to head (84477a2).
Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #33      +/-   ##
==========================================
- Coverage   50.92%   50.77%   -0.15%     
==========================================
  Files          31       31              
  Lines       11878    11913      +35     
==========================================
  Hits         6049     6049              
- Misses       5829     5864      +35

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

proto/kuksa/val/v2/val.proto

proto/kuksa/val/v2/types.proto

proto/kuksa/val/v2/val.proto

proto/kuksa/val/v2/types.proto

proto/kuksa/val/v2/val.proto

proto/kuksa/val/v2/types.proto

proto/kuksa/val/v2/val.proto

BjoernAtBosch · 2024-06-20T10:15:57Z

proto/kuksa/val/v2/val.proto

+  // Unique request id that can be used to identify the response.
+  int32 request_id = 1;


Do we need that in the unary publish call?

argerus · 2024-06-20T10:17:37Z

proto/kuksa/val/v2/types.proto

+message Error {
+  uint32 code    = 1;
+  string reason  = 2;
+  string message = 3;
+}


Suggested change

message Error {

uint32 code = 1;

string reason = 2;

string message = 3;

}

message Error {

ErrorCode code = 1;

string message = 2;

}

message ErrorCode {

INVALID_ARGUMENT = 3;

NOT_FOUND = 5;

PERMISSION_DENIED = 7;

}

proto/kuksa/val/v2/val.proto

BjoernAtBosch · 2024-06-21T15:57:34Z

proto/kuksa/val/v2/val.proto

+}
+
+message ListValuesResponse {
+  repeated Datapoint datapoints = 1;


I think, here we need a map<string, Datapoint> as we cannot know beforehand which and how many datapoints will be returned

True, and I have to admit that I had to double check the Datapoint that it does not include the signal name because this would somehow be my assumption when reading "Datapoint".

BjoernAtBosch · 2024-06-21T16:01:14Z

proto/kuksa/val/v2/val.proto

+}
+
+message GetValueRequest {
+  SignalID signal_id = 1;


Maybe it would be better having two separate requests: GetValue(ByName)Request and GetValueByIdRequest.

I mean, we introduced the single signal get for making life easier for a developer, but know we put "the burden" on them to make this extra set step for the oneof ...

BjoernAtBosch · 2024-06-21T16:01:29Z

proto/kuksa/val/v2/val.proto

+}
+
+message GetValuesRequest {
+  repeated SignalID signal_ids = 1;


proto/kuksa/val/v2/types.proto

proto/kuksa/val/v2/val.proto

BjoernAtBosch · 2024-06-21T16:23:51Z

proto/kuksa/val/v2/val.proto

+    PublishValuesRequest publish_values_request = 2;
+    // Sent to acknowledge the acceptance of a batch actuate
+    // request.
+    BatchActuateResponse batch_actuate_response = 3;


Not sure, if we should re-use the actuate requests/responses for the provider stream: At least for the response of the provider we need to define an error message in case the provider needs to reject something.

BjoernAtBosch · 2024-06-21T16:29:38Z

proto/kuksa/val/v2/val.proto

+  // Returns (GRPC error code):
+  //   NOT_FOUND if any of the signals are non-existant.
+  //   PERMISSION_DENIED
+  //       - if access is denied for any of the signals.
+  //       - if the signal is already provided by another provider.
+  //   INVALID_ARGUMENT
+  //       - if the data type used in the request does not match
+  //            the data type of the addressed signal
+  //       - if the published value is not accepted,
+  //            e.g. if sending an unsupported enum value
+  rpc PublishValue(PublishValueRequest) returns (PublishValueResponse);


I would like to have a "oneshot" publish for multiple values: With that a provider can easily publish a whole bunch of attributes (I'd prefer the "batch" version over the single one on the provider side)

BjoernAtBosch · 2024-06-21T16:33:07Z

proto/kuksa/val/v2/val.proto

+
+message PublishValuesResponse {
+  int32 request_id          = 1;
+  map<string, Error> status = 2;


Why do we return errors per signal in this operation? In the other cases we said, we just want to have one overall error ...

mikehaller · 2024-07-04T15:33:38Z

proto/kuksa/val/v2/val.proto

+
+import "kuksa/val/v2/types.proto";
+
+service VAL {


Ugly service name.

what would you suggest :D

Databroker, Broker, ValBroker (in that order of preferance)

proto/kuksa/val/v2/types.proto

BjoernAtBosch · 2024-07-10T12:17:34Z

proto/kuksa/val/v2/val.proto

+
+message PublishValuesRequest {
+  int32 request_id                  = 1; /// Unique request id for the stream that can be used to identify the response.
+  map<string, Datapoint> datapoints = 2;


To allow both, id-based and path-based signal referencing, could we have something like this:

Suggested change

map<string, Datapoint> datapoints = 2;

oneof mapping {

map<string, Datapoint> datapointsByPPath = 2;

map<int32, Datapoint> datapointsById = 3;

}

or a separate request called PublishValuesByIdRequest

Why not map<SignalD, Datapoint> datapoints ? Maybe I am thinking too easy here.

In protobuf the key of a map must be either an integral type or a string; see: https://protobuf.dev/programming-guides/proto3/#maps

eriksven · 2024-07-12T16:46:10Z

proto/kuksa/val/v2/val.proto

+    // Response to a provide actuator request.
+    ProvideActuatorResponse provide_actuator_response              = 1;
+    // Acknowledgement that a published value was received.
+    PublishValuesResponse publish_value_response                   = 2;


Should this be publish_values_response with an s?

eriksven · 2024-07-12T17:03:33Z

proto/kuksa/val/v2/types.proto

+  }
+}
+
+message SignalID {


Could we call the message signal? For me it was hard to follow during the usage of the SingalID message that this actually either an id or the path.

The term "signal" would be more than just the identifier of it. Therefore, I like the name "SignalId": "Id" is short for "identifier" which not necessarily has to be a number. It could be anything capable of serving as a unique id - therefore also a string.

eriksven · 2024-07-12T17:20:47Z

proto/kuksa/val/v2/val.proto

+message BatchActuateStreamResponse {
+}
+
+message OpenProviderStreamRequest {


What is the rational of cramping multiple concerns into a single function call or stream? Is the idea to "save" on the number of open (TCP) connections?

To me this sounds like there could be problems on both side of the stream of correlating the different operations handled over the same stream.
That is why it would feel more intuitive for me to have PublishValues as Stream and a ProvideActuation function here.

But these calls most likely would be transmitted over the same gRCP channel anyway when they originate from the same client, right?

lukasmittag · 2024-08-13T13:05:12Z

proto/kuksa/val/v2/val.proto

+}
+
+message PublishValueResponse {
+  Error error = 1;


Does error make sense here? We give back a general grpc error?

Yes, this is a "request in a request". If using a gRPC error, it would terminate the overall ProviderStream ...

lukasmittag · 2024-08-13T13:05:24Z

proto/kuksa/val/v2/val.proto

+
+message PublishValuesResponse {
+  int32 request_id         = 1;
+  map<int32, Error> status = 2;


Does error make sense here? We give back a general grpc error?

Yes, this is a "request in a request". If using a gRPC error, it would terminate the overall ProviderStream ...

SebastianSchildt · 2024-08-14T13:37:32Z

Hi I have looked into upgradeing on of our use cases to this API, and either the API misses efficient actuation or I missed it :D

Let's say I have a control loop controlling e.g. the SteeringAngle, I understand on the Provider Side I can use
OpenProviderStream to have a provider listening for SteeringAngle "target" value changes, and do something wiht them.

I am unclear how it works on application level. I see there is an actuate call, but that is only an rpc, so can't be used for fast updates, l(like StreamedUpdate in v1). Do I miss something? I expected Something like StreamedActuation?
@rafaeling @lukasmittag
fyi @eriksven

BjoernAtBosch · 2024-08-14T14:23:55Z

Hi I have looked into upgradeing on of our use cases to this API, and either the API misses efficient actuation or I missed it :D

Let's say I have a control loop controlling e.g. the SteeringAngle, I understand on the Provider Side I can use OpenProviderStream to have a provider listening for SteeringAngle "target" value changes, and do something wiht them.

I am unclear how it works on application level. I see there is an actuate call, but that is only an rpc, so can't be used for fast updates, l(like StreamedUpdate in v1). Do I miss something? I expected Something like StreamedActuation? @rafaeling @lukasmittag fyi @eriksven

As long as the data broker is not "realtime capable" imho we shouldn't do something like control loops via data broker. I think, consumers should set target values with a low frequency only. Otherwise we would have to guarantee a max latency.

rafaeling · 2024-08-14T14:26:46Z

Hi I have looked into upgradeing on of our use cases to this API, and either the API misses efficient actuation or I missed it :D

Let's say I have a control loop controlling e.g. the SteeringAngle, I understand on the Provider Side I can use OpenProviderStream to have a provider listening for SteeringAngle "target" value changes, and do something wiht them.

I am unclear how it works on application level. I see there is an actuate call, but that is only an rpc, so can't be used for fast updates, l(like StreamedUpdate in v1). Do I miss something? I expected Something like StreamedActuation? @rafaeling @lukasmittag fyi @eriksven

I recall that this topic was on our minds for future discussion. However, we initially decided that the first version would only actuate a signal using a unary call, as there weren't that many use cases for real time actuating on an actuator using a gRPC stream. I remember we discussed also an example of raising the car window by pressing a button, but that was the extent of our conversation.

SebastianSchildt · 2024-08-14T15:54:05Z

Hi I have looked into upgradeing on of our use cases to this API, and either the API misses efficient actuation or I missed it :D
Let's say I have a control loop controlling e.g. the SteeringAngle, I understand on the Provider Side I can use OpenProviderStream to have a provider listening for SteeringAngle "target" value changes, and do something wiht them.
I am unclear how it works on application level. I see there is an actuate call, but that is only an rpc, so can't be used for fast updates, l(like StreamedUpdate in v1). Do I miss something? I expected Something like StreamedActuation? @rafaeling @lukasmittag fyi @eriksven

As long as the data broker is not "realtime capable" imho we shouldn't do something like control loops via data broker. I think, consumers should set target values with a low frequency only. Otherwise we would have to guarantee a max latency.

It is not about realtime (as in guaranteed latency), and I think not so mich different whether operating open loop or closed loop. It it is more a CPU load topic: In the usecase we had a factor 6 reduction in CPU load (comparing unary set to streams), so it would be bad if the "new" API would be 6 times more inefficient. For example there is also a subscribe call, instead of forcing users to poll via get.

SebastianSchildt · 2024-08-14T16:04:31Z

I recall that this topic was on our minds for future discussion. However, we initially decided that the first version would only actuate a signal using a unary call, as there weren't that many use cases for real time actuating on an actuator using a gRPC stream. I remember we discussed also an example of raising the car window by pressing a button, but that was the extent of our conversation.

One specific example is the control of the FlexCar Chassis (https://arena2036.de/de/flexcar) via a remote connection. This is done in the https://sofdcar.de/ project via KUKSA. This has

low cycle times for safety reasons (and user experience)
has no real-time" requirements (i.e. this is a system doing over Internet/cellular like all typical Remote Driving applications, so you have no guaranteed realtime capabilities already on the network side, the safety model depends on "Observing" the achieved latencies and reacting, but if the rate can not be achieved in the vehicle due an inefficient stack/slow vehicle computer (pick your explanation :) ), it is not good.

BjoernAtBosch · 2024-08-14T17:43:59Z

Hi I have looked into upgradeing on of our use cases to this API, and either the API misses efficient actuation or I missed it :D
Let's say I have a control loop controlling e.g. the SteeringAngle, I understand on the Provider Side I can use OpenProviderStream to have a provider listening for SteeringAngle "target" value changes, and do something wiht them.
I am unclear how it works on application level. I see there is an actuate call, but that is only an rpc, so can't be used for fast updates, l(like StreamedUpdate in v1). Do I miss something? I expected Something like StreamedActuation? @rafaeling @lukasmittag fyi @eriksven

As long as the data broker is not "realtime capable" imho we shouldn't do something like control loops via data broker. I think, consumers should set target values with a low frequency only. Otherwise we would have to guarantee a max latency.

It is not about realtime (as in guaranteed latency), and I think not so mich different whether operating open loop or closed loop. It it is more a CPU load topic: In the usecase we had a factor 6 reduction in CPU load (comparing unary set to streams), so it would be bad if the "new" API would be 6 times more inefficient. For example there is also a subscribe call, instead of forcing users to poll via get.

Yes, understood. But even in an stream-optimized version we would not be able to guarantee latency, which means safety-critical - or at least "timing-critical" - operations like remote steering or control loops should not be done via the broker. What happens if you want to stop something, but for some reason the latency is temporary > 1 second? I think, it's better setting a target value and trust on the local ECUs trying to gain that in a reliable and safe way.

But maybe there are use cases, I'm not aware of, which would require these frequent actuator updates.

BjoernAtBosch · 2024-09-26T12:41:09Z

proto/kuksa/val/v2/types.proto

+  oneof value_state {
+    ValueFailure failure = 2;
+    Value value          = 3;
+  }


@argerus @rafaeling
Why I like the "oneof":
Assuming the ValueFailure represents the reason why a "real" value is missing (maybe we should rename it to "MissingReason"?), the oneof has some advantages over two separate fields (e.g. value and value_state):

If having a separate status with the additional state "OK" - meaning a valid value is present -, this could potentially be in mis-sync with the value field:

What, if status is "OK", but no value is present?

Or vice versa, status is not "OK", but a value is present?

On server side it's not possible to set both in parallel (prevented by the generated code - at least in C++)

It gives a small optimization during transmission via grpc: There either the value or the "MissingReason" is transferred.
I don't see many disadvantages:

Code-wise (imho) it is easier, because I can better trust, that those invalid combinations of value and value_state described above cannot occur.
Just checking if a value is there and otherwise ignoring the reason why it's missing is as easy as just having a value field or having an additional value_state field. At least in C++:

kuksa::val::v2::Datapoint datapoint; if (datapoint.has_value()) { auto value =datapoint.value(); } else { // handle missing value, e.g.: throw MissingValueException(); }

This C++ code would be the same - regardless if we have:
(1)

message Datapoint { google.protobuf.Timestamp timestamp = 1; Value value = 2; }

or (2)

message Datapoint { google.protobuf.Timestamp timestamp = 1; Value value = 2; ValueState state = 3; }

or (3)

message Datapoint { google.protobuf.Timestamp timestamp = 1; oneof value_state { Value value = 2; MissingReason reason = 3; } }

Besides that, I think transporting additional information about a present value (like some quality information (like "outdated" or other things we discussed today), doesn't belong into an information field describing reasons why a value is missing. Those informations aren't related.

erikbosch · 2024-10-10T08:26:49Z

Closing this one as it has an obsolete source branch. Current API v2 work is ongoing in https://github.com/eclipse-kuksa/kuksa-databroker/tree/feature/databroker-api-v2 but no PR back to main branch exist as of today

rafaeling commented May 17, 2024

View reviewed changes

proto/kuksa/val/v2/val.proto Show resolved Hide resolved

BjoernAtBosch reviewed May 17, 2024

View reviewed changes

BjoernAtBosch reviewed Jun 3, 2024

View reviewed changes

argerus requested changes Jun 4, 2024

View reviewed changes

BjoernAtBosch reviewed Jun 5, 2024

View reviewed changes

proto/kuksa/val/v2/val.proto Outdated Show resolved Hide resolved

BjoernAtBosch suggested changes Jun 18, 2024

View reviewed changes

BjoernAtBosch reviewed Jun 20, 2024

View reviewed changes

argerus reviewed Jun 20, 2024

View reviewed changes

rafaeling and others added 14 commits June 20, 2024 18:56

Init Databroker API V2

75b440f

Added oneof to Datapoint and some changes

2e103a6

Rename oneof Datapoint

c01314a

Add oneof for Stream Provider response

50ca9a4

Update provide stream response and add files to the build project

ce13516

Update ProvideStream and Actuate calls

32c79d8

Add Start/Stop Signal

761d9c0

Remove Start/Stop Signal and add batch methods

ce7cfc1

Use correct numeration

3221203

Clean up and rename messages

5afc533

Move ValueFailure to bottom

233138e

Add proposed API

48a603f

Updates after review session

982968d

format

6f12990

rafaeling force-pushed the feature/databroker-api-v2 branch from a3e7b29 to 6f12990 Compare June 20, 2024 16:57

pre commit

70c41c4

rafaeling requested review from argerus and BjoernAtBosch June 20, 2024 17:12

BjoernAtBosch reviewed Jun 21, 2024

View reviewed changes

rafaeling added 2 commits June 24, 2024 12:28

Improve documentation

5ce44ce

Avoid reusing messages among calls

49f2a8f

mikehaller reviewed Jul 4, 2024

View reviewed changes

proto/kuksa/val/v2/types.proto Outdated Show resolved Hide resolved

BjoernAtBosch reviewed Jul 10, 2024

View reviewed changes

eriksven reviewed Jul 12, 2024

View reviewed changes

rafaeling added 2 commits July 15, 2024 10:34

Add signal i32 id to metadata

077d488

Fix year comment

d11f942

lukasmittag reviewed Aug 13, 2024

View reviewed changes

rafaeling added 2 commits August 15, 2024 13:40

Introduce subscribe i32 parameter due to performance reasons

440b82a

Correct parameter name

84477a2

BjoernAtBosch reviewed Sep 26, 2024

View reviewed changes

erikbosch closed this Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Databroker API V2 #33

Databroker API V2 #33

rafaeling commented May 16, 2024

codecov bot commented May 16, 2024 •

edited

Loading

BjoernAtBosch Jun 20, 2024

argerus Jun 20, 2024

BjoernAtBosch Jun 21, 2024

eriksven Jul 12, 2024

BjoernAtBosch Jun 21, 2024

BjoernAtBosch Jun 21, 2024

BjoernAtBosch Jun 21, 2024

BjoernAtBosch Jun 21, 2024

BjoernAtBosch Jun 21, 2024

mikehaller Jul 4, 2024

lukasmittag Jul 12, 2024

BjoernAtBosch Jul 12, 2024

BjoernAtBosch Jul 10, 2024 •

edited

Loading

eriksven Jul 12, 2024

BjoernAtBosch Aug 7, 2024

eriksven Jul 12, 2024

eriksven Jul 12, 2024

BjoernAtBosch Aug 7, 2024 •

edited

Loading

eriksven Jul 12, 2024 •

edited

Loading

eriksven Jul 12, 2024

lukasmittag Aug 13, 2024

BjoernAtBosch Aug 13, 2024

lukasmittag Aug 13, 2024

BjoernAtBosch Aug 13, 2024

SebastianSchildt commented Aug 14, 2024

BjoernAtBosch commented Aug 14, 2024

rafaeling commented Aug 14, 2024

SebastianSchildt commented Aug 14, 2024

SebastianSchildt commented Aug 14, 2024 •

edited

Loading

BjoernAtBosch commented Aug 14, 2024

BjoernAtBosch Sep 26, 2024

erikbosch commented Oct 10, 2024

		// Unique request id that can be used to identify the response.
		int32 request_id = 1;

-message Error {
-  uint32 code    = 1;
-  string reason  = 2;
-  string message = 3;
-}
+message Error {
+  ErrorCode code    = 1;
+  string message = 2;
+}
+message ErrorCode {
+  INVALID_ARGUMENT = 3;
+  NOT_FOUND = 5;
+  PERMISSION_DENIED = 7;
+}

-  map<string, Datapoint> datapoints = 2;
+  oneof mapping {
+      map<string, Datapoint> datapointsByPPath = 2;
+      map<int32, Datapoint> datapointsById = 3;
+  }

Databroker API V2 #33

Databroker API V2 #33

Conversation

rafaeling commented May 16, 2024

codecov bot commented May 16, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

BjoernAtBosch Jul 10, 2024 • edited Loading

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

BjoernAtBosch Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

eriksven Jul 12, 2024 • edited Loading

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

SebastianSchildt commented Aug 14, 2024

BjoernAtBosch commented Aug 14, 2024

rafaeling commented Aug 14, 2024

SebastianSchildt commented Aug 14, 2024

SebastianSchildt commented Aug 14, 2024 • edited Loading

BjoernAtBosch commented Aug 14, 2024

Choose a reason for hiding this comment

erikbosch commented Oct 10, 2024

codecov bot commented May 16, 2024 •

edited

Loading

BjoernAtBosch Jul 10, 2024 •

edited

Loading

BjoernAtBosch Aug 7, 2024 •

edited

Loading

eriksven Jul 12, 2024 •

edited

Loading

SebastianSchildt commented Aug 14, 2024 •

edited

Loading