Skip to content
This repository has been archived by the owner on Dec 18, 2024. It is now read-only.

Add timestamps to actuator target values #474

Merged
merged 1 commit into from
Feb 14, 2023
+21 −10
Merged

Add timestamps to actuator target values #474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions kuksa_databroker/databroker/src/broker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,15 @@ pub struct Datapoint {
pub value: DataValue,
}

#[derive(Debug, Clone)]
pub struct ActuatorTarget {
pub target_datapoint: Datapoint,
}

#[derive(Debug, Clone)]
pub struct Entry {
pub datapoint: Datapoint,
pub actuator_target: Option<DataValue>,
pub actuator_target: Option<ActuatorTarget>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@lukasmittag lukasmittag Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@argerus argerus Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lukasmittag lukasmittag Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pub metadata: Metadata,
}

Expand Down Expand Up @@ -145,7 +150,7 @@ pub struct EntryUpdate {
// Actuator target is wrapped in an additional Option<> in
// order to be able to convey "update it to None" which would
// mean setting it to `Some(None)`.
pub actuator_target: Option<Option<DataValue>>, // only for actuators
pub actuator_target: Option<Option<ActuatorTarget>>, // only for actuators

// Metadata
pub entry_type: Option<EntryType>,
Expand Down Expand Up @@ -176,8 +181,8 @@ impl Entry {
if let Some(datapoint) = &update.datapoint {
self.validate_value(&datapoint.value)?;
}
if let Some(Some(value)) = &update.actuator_target {
self.validate_value(value)?;
if let Some(Some(actuatortarget)) = &update.actuator_target {
self.validate_value(&actuatortarget.target_datapoint.value)?;
}
Ok(())
}
Expand Down
4 changes: 3 additions & 1 deletion kuksa_databroker/databroker/src/grpc/kuksa_val_v1/conversions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,9 @@ impl From<broker::EntryUpdate> for proto::DataEntry {
None => None,
},
actuator_target: match from.actuator_target {
Some(Some(actuator_target)) => Option::<proto::Datapoint>::from(actuator_target),
Some(Some(actuator_target)) => {
Option::<proto::Datapoint>::from(actuator_target.target_datapoint)
}
Some(None) => None,
None => None,
},
Expand Down
8 changes: 6 additions & 2 deletions kuksa_databroker/databroker/src/grpc/kuksa_val_v1/val.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,9 @@ fn proto_entry_from_entry_and_fields(
};
let actuator_target = if fields.contains(&proto::Field::ActuatorTarget) {
match entry.actuator_target {
Some(actuator_target) => Option::<proto::Datapoint>::from(actuator_target),
Some(actuator_target) => {
Option::<proto::Datapoint>::from(actuator_target.target_datapoint)
}
None => None,
}
} else {
Expand Down Expand Up @@ -418,7 +420,9 @@ impl broker::EntryUpdate {
};
let actuator_target = if fields.contains(&proto::Field::ActuatorTarget) {
match &entry.actuator_target {
Some(datapoint) => Some(Some(broker::DataValue::from(datapoint.value.clone()))),
Some(datapoint) => Some(Some(broker::ActuatorTarget {
target_datapoint: broker::Datapoint::from(datapoint.clone()),
})),
None => Some(None),
}
} else {
Expand Down
6 changes: 3 additions & 3 deletions kuksa_databroker/databroker/src/grpc/sdv_databroker_v1/broker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,9 @@ impl proto::broker_server::Broker for broker::DataBroker {
broker::EntryUpdate {
path: None,
datapoint: None,
actuator_target: Some(Some(broker::DataValue::from(
&datapoint,
))),
actuator_target: Some(Some(broker::ActuatorTarget {
target_datapoint: broker::Datapoint::from(&datapoint),
})),
entry_type: None,
data_type: None,
description: None,
Expand Down