Adding GetValue support #4
Conversation
| @@ -46,7 +46,12 @@ impl From<&proto::Datapoint> for broker::Datapoint { | |||
| impl From<broker::Datapoint> for Option<proto::Datapoint> { | |||
| fn from(from: broker::Datapoint) -> Self { | |||
| match from.value { | |||
| broker::DataValue::NotAvailable => None, | |||
| broker::DataValue::NotAvailable => Some(proto::Datapoint { | |||
There was a problem hiding this comment.
This a discussion point. For GetValue we expect a ValueState:.Failure to be returned if a value not yet exist, but not for subscribe. One solution is to map here but filter higher up (in subscribe)
|
@lukasmittag @rafaeling @argerus My first PR with substantial Rust changes. Seems to do what it shall but we could discuss how we want subscribe to react if the is no valid value - should you get a value first when there actually is a value, i.e. never receive notAvailable? As I am a Rust beginner feel free to propose changes that makes the rust look more compact or improves reuse. |
| @@ -147,18 +159,20 @@ message ValueRestrictionString { | |||
| repeated string allowed_values = 1; | |||
| } | |||
|
|
|||
| // Used to indicate status of a signal in Databroker | |||
| // This is given as an alternative to Value, so if there is a valid value | |||
| // ValueFailure shall not be specified. | |||
There was a problem hiding this comment.
Just a thought - would it be better to name this enum ValueInfo? At least getting NOT_PROVIDED is not necessarily a failure, and could theoretically be used by some provider in PublishValueRequest to indicate that the previous value should be deleted.
There was a problem hiding this comment.
I agree that calling it failure is a bit misleading. I also think having the value inside a value_state is unnecessary as it adds one more level of nesting to get to the actual value.
I think something like this would be more intuitive:
message DataPoint {
google.protobuf.Timestamp timestamp = 1;
Value value = 3;
Status status = 10;
}
enum Status {
STATUS_UNSPECIFIED = 0;
STATUS_OK = 1;
STATUS_VALUE_NOT_AVAILBLE = 2;
}If there is no value, a provider (or databroker) can set the value to None and the corresponding reason for it in the status field.
One "downside" of this is that it's possible to set a value while at the same time setting the status to something other than STATUS_OK (and vice versa setting STATUS_OK while not setting any value).
But doing that can simply be disallowed by databroker in the same way that it doesn't allow setting a value of the wrong data type today.
There was a problem hiding this comment.
I like the proposal.
And if we document (in *.proto) how various combinations/situations should be handled by client and databroker it should not be a problem.
| Scenario | value | status |
|---|---|---|
| Value available for signal | <value> |
STATUS_UNSPECIFIED or STATUS_OK |
| Value NOT available for signal | None |
STATUS_VALUE_NOT_AVAILABLE |
Anyway - will not have time for any update next week, but @rafaeling or @lukasmittag are free to change if they like and gree
There was a problem hiding this comment.
Also agree with that, like this approach more than the current one.
| // The signal is known, but no value is provided currently | ||
| // Can theoretically also be used in write request to "delete" a value |
There was a problem hiding this comment.
Should we possible for each one indicate in which direction they can/shall be used? I can see a use-case for sending NOT_PROVIDED towards databroker, but I see no reason to send INTERNAL_ERROR to Databroker.
We can discuss if we want to keep INTERNAL_ERROR. For single calls I see no need as we can return a GRPC error code. For calls that shall return multiple signals (like GetValues) it could make sense to be able to return other signals. For streaming it makes sense if we want to continue to keep the stream open upon an internal error to be able to continue subscription for other signals. But internal error should never happen right?
There was a problem hiding this comment.
INVALID_VALUE still think should be used when provider receives some invalid value from CAN BUS.
I think that was the idea of keeping it.
But you are right, this still needs to be discussed. We still wanted to clean up this enum.
| @@ -22,7 +22,12 @@ message Datapoint { | |||
| google.protobuf.Timestamp timestamp = 1; | |||
|
|
|||
| oneof value_state { | |||
| // When reading - returned if the signal exists but has no value, | |||
There was a problem hiding this comment.
Thanks for adding some needed documentation, just wondering if here is the right place.
I usually made API changes to https://github.com/SoftwareDefinedVehicle/kuksa-databroker/tree/feature/databroker-api-v2 and then rebase main-branch onto that one and finally dev-branch onto main-branch.
I am confused now with the development process.
There was a problem hiding this comment.
Feel free to sort it out and if needed/wanted move things to another branch/fork
|
@rafaeling - shall this one be changed to target https://github.com/eclipse-kuksa/kuksa-databroker/tree/feature/databroker-api-v2 instead? Did you discuss any of the changes proposed in this PR or in comments, I can update it on Monday |
|
Replaced by eclipse-kuksa#75 |
Work in progress