Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add allowed values to metadata #69

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Add allowed values to metadata #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c511caa
Add allowed values to metadata
argerus Sep 24, 2024
9741da2
Merge branch 'main' into feature/add_allowed_to_metadata
erikbosch Oct 15, 2024
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
3 changes: 3 additions & 0 deletions databroker-cli/src/sdv_cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1262,6 +1262,7 @@ mod test {
entry_type: proto::v1::EntryType::Sensor.into(),
change_type: proto::v1::ChangeType::OnChange.into(),
description: "".into(),
allowed: None,
},
proto::v1::Metadata {
id: 2,
Expand All @@ -1270,6 +1271,7 @@ mod test {
entry_type: proto::v1::EntryType::Sensor.into(),
change_type: proto::v1::ChangeType::OnChange.into(),
description: "".into(),
allowed: None,
},
proto::v1::Metadata {
id: 3,
Expand All @@ -1278,6 +1280,7 @@ mod test {
entry_type: proto::v1::EntryType::Sensor.into(),
change_type: proto::v1::ChangeType::OnChange.into(),
description: "".into(),
allowed: None,
},
]
.to_vec();
Expand Down
68 changes: 67 additions & 1 deletion 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 @@ -792,7 +792,73 @@ fn proto_entry_from_entry_and_fields(
}
if all || fields.contains(&proto::Field::MetadataValueRestriction) {
metadata_is_set = true;
// TODO: Add to Metadata
metadata.value_restriction = match entry.metadata().allowed.as_ref() {
Some(allowed) => match allowed {
broker::DataValue::StringArray(vec) => Some(proto::ValueRestriction {
r#type: Some(proto::value_restriction::Type::String(
proto::ValueRestrictionString {
allowed_values: vec.clone(),
},
)),
}),
broker::DataValue::Int32Array(vec) => Some(proto::ValueRestriction {
r#type: Some(proto::value_restriction::Type::Signed(
proto::ValueRestrictionInt {
allowed_values: vec.iter().cloned().map(i64::from).collect(),
min: None, // TODO: Implement
Copy link
Contributor

Choose a reason for hiding this comment

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

So for now just creating a placeholder for supporting min/max?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were already defined in kuksa.val.v1.ValueRestriction.. this PR doesn't change anything regarding them, it's limited to adding support for allowed values.

But maybe it would be better to add support for min / max as well in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no strong opinion, depends on how much time that would take I think. If it is quick go for it, if not keep the TODOs and do it later.

max: None, // TODO: Implement
},
)),
}),
broker::DataValue::Int64Array(vec) => Some(proto::ValueRestriction {
r#type: Some(proto::value_restriction::Type::Signed(
proto::ValueRestrictionInt {
allowed_values: vec.clone(),
min: None, // TODO: Implement
max: None, // TODO: Implement
},
)),
}),
broker::DataValue::Uint32Array(vec) => Some(proto::ValueRestriction {
r#type: Some(proto::value_restriction::Type::Unsigned(
proto::ValueRestrictionUint {
allowed_values: vec.iter().cloned().map(u64::from).collect(),
min: None, // TODO: Implement
max: None, // TODO: Implement
},
)),
}),
broker::DataValue::Uint64Array(vec) => Some(proto::ValueRestriction {
r#type: Some(proto::value_restriction::Type::Unsigned(
proto::ValueRestrictionUint {
allowed_values: vec.clone(),
min: None, // TODO: Implement
max: None, // TODO: Implement
},
)),
}),
broker::DataValue::FloatArray(vec) => Some(proto::ValueRestriction {
r#type: Some(proto::value_restriction::Type::FloatingPoint(
proto::ValueRestrictionFloat {
allowed_values: vec.iter().cloned().map(f64::from).collect(),
min: None, // TODO: Implement
max: None, // TODO: Implement
},
)),
}),
broker::DataValue::DoubleArray(vec) => Some(proto::ValueRestriction {
r#type: Some(proto::value_restriction::Type::FloatingPoint(
proto::ValueRestrictionFloat {
allowed_values: vec.clone(),
min: None, // TODO: Implement
max: None, // TODO: Implement
},
)),
}),
_ => None,
},
None => None,
}
}
if all || fields.contains(&proto::Field::MetadataActuator) {
metadata_is_set = true;
Expand Down
48 changes: 48 additions & 0 deletions databroker/src/grpc/sdv_databroker_v1/conversions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,54 @@ impl From<&broker::Metadata> for proto::Metadata {
data_type: proto::DataType::from(&metadata.data_type) as i32,
change_type: proto::ChangeType::Continuous as i32, // TODO: Add to metadata
description: metadata.description.to_owned(),
allowed: match metadata.allowed.as_ref() {
Some(broker::DataValue::StringArray(vec)) => Some(proto::Allowed {
values: Some(proto::allowed::Values::StringValues(proto::StringArray {
values: vec.clone(),
})),
}),
Some(broker::DataValue::Int32Array(vec)) => Some(proto::Allowed {
values: Some(proto::allowed::Values::Int32Values(proto::Int32Array {
values: vec.clone(),
})),
}),
Some(broker::DataValue::Int64Array(vec)) => Some(proto::Allowed {
values: Some(proto::allowed::Values::Int64Values(proto::Int64Array {
values: vec.clone(),
})),
}),
Some(broker::DataValue::Uint32Array(vec)) => Some(proto::Allowed {
values: Some(proto::allowed::Values::Uint32Values(proto::Uint32Array {
values: vec.clone(),
})),
}),
Some(broker::DataValue::Uint64Array(vec)) => Some(proto::Allowed {
values: Some(proto::allowed::Values::Uint64Values(proto::Uint64Array {
values: vec.clone(),
})),
}),
Some(broker::DataValue::FloatArray(vec)) => Some(proto::Allowed {
values: Some(proto::allowed::Values::FloatValues(proto::FloatArray {
values: vec.clone(),
})),
}),
Some(broker::DataValue::DoubleArray(vec)) => Some(proto::Allowed {
values: Some(proto::allowed::Values::DoubleValues(proto::DoubleArray {
values: vec.clone(),
})),
}),
Some(broker::DataValue::BoolArray(_))
| Some(broker::DataValue::NotAvailable)
| Some(broker::DataValue::Bool(_))
| Some(broker::DataValue::String(_))
| Some(broker::DataValue::Int32(_))
| Some(broker::DataValue::Int64(_))
| Some(broker::DataValue::Uint32(_))
| Some(broker::DataValue::Uint64(_))
| Some(broker::DataValue::Float(_))
| Some(broker::DataValue::Double(_))
| None => None,
},
}
}
}
Expand Down
15 changes: 13 additions & 2 deletions proto/sdv/databroker/v1/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,16 +143,27 @@ message Datapoint {
}
}

message Allowed {
oneof values {
StringArray string_values = 1;
Int32Array int32_values = 3;
Int64Array int64_values = 4;
Uint32Array uint32_values = 5;
Uint64Array uint64_values = 6;
FloatArray float_values = 7;
DoubleArray double_values = 8;
}
}

message Metadata {
// Id to be used in "get" and "subscribe" requests. Ids stay valid during
// one power cycle, only.
int32 id = 1;
EntryType entry_type = 2;
string name = 4;
DataType data_type = 5;
ChangeType change_type = 6; // CONTINUOUS or STATIC or ON_CHANGE
string description = 7;

Allowed allowed = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we ever used the metadata on the lines below? Even if the lines below are commented I assume it would be better to not have the same number (10) on this line and the line below, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They have never been used. I guess it's better to just remove them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then just remove them I would say

// int32 min_update_hz = 10; // Only for CONTINUOUS
// int32 max_update_hz = 11; // Only for CONTINUOUS
};