eclipse / kuksa.val

kuksa.val
Apache License 2.0
96 stars 51 forks source link

Subscription: Updates on each setValue #652

Closed nayakned closed 1 week ago

nayakned commented 1 year ago

For quite a few usecases (e.g. heartbeat signals) it is essential that the update messages to a subscription are sent each time the corresponding datapoint is set (irrespective of whether the value changes or not). Currently this is by default the case with target values (which is also not good for applications which need to triggered only on change of values).

Suggestion: Add an optional flag during subscription request to figure out if the subscribing application needs the information when set(Target)Value is called or only when the value changes.

argerus commented 1 year ago

I agree with the problem description.

So I see two distinct observations here.

Values

Suggestion: Add an optional flag during subscription request to figure out if the subscribing application needs the information when set(Target)Value is called or only when the value changes.

This can make sense. But, a related question is whether this is something that should be decided by the client, or if this is an inherent property of the signal/datapoint. If it is an inherent property of the signal, subscribing to it should always behave in accordance with that.

The VHAL implementation in Android has decided that this is an inherent property of the signal, and this is also how the implementation works internally in databroker. If a signal/datapoint is ChangeType::OnChange it will send notifications only when the signal/datapoint changes. If it is ChangeType::Continous, it will send notifications periodically* (regardless of whether the value changes).

Firstly, this information is not part of VSS though. But since VSS supports adding additional metadata to signals, the only thing missing is adding support for reading this extended metadata in databroker. Right now, it defaults to ChangeType::OnChange as you've noticed, (maybe that's the wrong default).

The second part is that a client should be able to supply a preferred update frequency when subscribing to ChangeType::Continuous type of signals, as to not introduce more load to the client or system than necessary.

This "solution" might seem more complicated than just letting the client decide. But another way to look at it, is that this is metadata (just as specifying the data type) that is needed in order to implement correctly behaving providers of these signals. And in a sense, also correctly behaving clients.

Target values

update messages to a subscription are sent each time [...] (irrespective of whether the value changes or not) [...] is by default the case with target values (which is also not good for applications which need to triggered only on change of values).

Setting a target value is distinct from setting a value. It is more akin to calling a function implemented by an actuator provider. If the provider is not available at the time of the call, it will not be notified. Calling the same "function" again, with the same value, when the provider is available should notify the provider even if the target value doesn't change. Because of that, I think it makes sense to send a notification every time (regardless of what the previous target value was). It might make sense to avoid this notification if the target value doesn't differ from the current value, though.

With that said, the whole mechanism around notifying an actuator provider might be reworked in order to enable the provider to respond to these requests (and propagate the response to the caller), see #560.

(*) Currently, every time the value is set.

argerus commented 1 year ago

It might still be useful to be able to subscribe to signals of type OnChange::Continous based on when their value actually changes. So maybe a solution in line with what is suggested

Suggestion: Add an optional flag during subscription request to figure out if the subscribing application needs the information when set(Target)Value is called or only when the value changes.

would be:

SebastianSchildt commented 1 year ago

My 2cts

Maybe "OnChange::Continuous" is a better "default, as it migth increase load, but you would never "loose" anything, and it may be less "unexptected

If the plumbing would already be there, how hard would it be, for databroker, upon parsing the VSS JSON checking for a field x-signal-type that is either "CONTINOUS" or "ON_CHANGE" and set it accordingly (or default back to Continous if nothing is there).

This ability would help a lot in deployment I think, i,e,

BjoernAtBosch commented 6 months ago

I agree with John's elaboration about the difference between (current) values and target values. To achieve what Naresh proposed and in parallel reducing load by avoiding unnecessary notifications, there could be two different parameters on provider side:

Clients could also be allowed to specify something like a "maximum" update frequency used to limit there update load by reducing the number of notifications they will receive. But for that it should be investigated, what will be done with the value updates in-between: Drop, calculate mean value (how?), collect and send as bunch, ...

erikbosch commented 1 week ago

We are about to archive this repo soon. If you consider this issue as important please file a new issue at one of the new Kuksa repos at https://github.com/eclipse-kuksa