Add timestamps to actuator target values #474
Conversation
lukasmittag
force-pushed
the
databroker/add_targetvalue_timestamps
branch
from
7745c6f
to
b2211d1
Compare
| #[derive(Debug, Clone)] | ||
| pub struct Entry { | ||
| pub datapoint: Datapoint, | ||
| pub actuator_target: Option<DataValue>, | ||
| pub actuator_target: Option<ActuatorTarget>, |
There was a problem hiding this comment.
This isn't wrong. But the data type of actuator target is Datapoint (in the .proto file) so an alternative would be to just use Option<Datapoint>. I think that would make it easier to reuse the conversion functions already in place for Datapoint (and would make the data type less nested).
There was a problem hiding this comment.
Yes, I thought about this too. Would it make definetly less nested. Then I would suggest to rename the datapoint in value or something else because otherwise I think the naming can be confusing. Or value_datapoint and target_datapoint
There was a problem hiding this comment.
Hmm, not so sure about that.
I think the general terminology in VSS (and elsewhere) is to call "value + timestamp" a datapoint. See for example VISS, here and here.
Now. VSS doesn't have the concept of actuator targets, but if we want to introduce a new thing which is also a "value + timestamp", one way to look at it is that it also has the data type DataPoint.
I suppose you see it as more of a variable name. Both ways to look at it makes sense I think. The alternative is to just keep it as you have already done, or maybe make it a new type ActuatorTarget(Datapoint) instead.
There was a problem hiding this comment.
Okay, I see the point about Option<Datapoint> for the target value too. I will change it to this and keep the actuator_target variable name. Should be enough to understand.
lukasmittag
force-pushed
the
databroker/add_targetvalue_timestamps
branch
from
b2211d1
to
b1d7cae
Compare
lukasmittag
force-pushed
the
databroker/add_targetvalue_timestamps
branch
from
b1d7cae
to
4c61bc9
Compare
No description provided.