eclipse / kuksa.val

kuksa.val
Apache License 2.0
89 stars 52 forks source link

Add actuator filtering to kuksa-client target methods #687

Closed rafaeling closed 8 months ago

rafaeling commented 8 months ago

After fixing permission handling (https://github.com/eclipse/kuksa.val/pull/654), the actuator filtering was removed from the broker. We decided that the broker should not throw an error in this case or contain any filter logic on its side. Instead, the responsibility for this kind of filtering should be from the kuksa-client.

The current output on the kuksa-client when calling getTargetValue, getTargetValue includes a combination of actuators, sensors, and attributes. The necessary fix to filter only actuators should be implemented:

Test Client> getTargetValues Vehicle.PowerOptimizeLevel Vehicle.IsMoving
[
    {
        "path": "Vehicle.PowerOptimizeLevel",
        "actuator_target": {
            "value": 8,
            "timestamp": "2023-10-19T08:02:09.639186+00:00"
        }
    },
    {
        "path": "Vehicle.IsMoving"
    }
]

Output expected or something similar:

Test Client> getTargetValues Vehicle.PowerOptimizeLevel Vehicle.IsMoving
[
    {
        "path": "Vehicle.PowerOptimizeLevel",
        "actuator_target": {
            "value": 8,
            "timestamp": "2023-10-19T08:02:09.639186+00:00"
        }
    }
]
SebastianSchildt commented 8 months ago

I see that on getTarget there may be arguments for both types of behavior (as I think there was a discussion waht about (corner?) cases where on get (on GRPC API Level) requests both value and targetValue for something that is a sensor.

Just for clarification though: when we SET target Values I would still very mich expect an error when this is attempted on an attribute or sensor. Is that still the case?

lukasmittag commented 8 months ago
 {
      "path": "Vehicle.IsMoving"
 }

but this would have been what would be returned before so what's the problem?

argerus commented 8 months ago

Just for clarification though: when we SET target Values I would still very mich expect an error when this is attempted on an attribute or sensor. Is that still the case?

Yes. Setting anything but an actuator will return an error.

rafaeling commented 8 months ago
 {
      "path": "Vehicle.IsMoving"
 }

but this would have been what would be returned before so what's the problem?

As it was initially implemented here (https://github.com/eclipse/kuksa.val/commit/9b7861db4a8d36dd47f0e55d8c5d53b3ca98f0bc#diff-ae8c606603352f073807e33a1cebe4a184a2eb69fe92453536878c44857498c1R100) and later removed, I believe the purpose of these lines was just then to reduce the number of irrelevant signals (sensors and attributes) sent when only retrieving actuator values.

However, it appears that I was mistaken, as it seems that kuksa-client handles actuator filtering correctly here (https://github.com/eclipse/kuksa.val/blob/master/kuksa-client/kuksa_client/grpc/aio.py#L159). Perhaps some refinement would be good to prevent displaying an empty path when there is no actuator_target value.

SebastianSchildt commented 8 months ago

So I checked with 0.4.0 couple of databroker and with master . The behavior has not changed. So this at least not a bug. (we might discuss to change the behavior, but again at least not criticial it seems)

I would not filter it in the kuksa-client. Currently the client shows what it gets from GRPC API and this is how it should be, i.e. IF we want filter it in the client, that would more of a hint to me to change it in databroker.

So maybe this can be closed?

Or did I miss(understood) something?

rafaeling commented 8 months ago

I think can be closed