Interface improvements by shsms · Pull Request #23 · frequenz-floss/frequenz-microgrid-rs

shsms · 2026-03-20T16:02:02Z

No description provided.

This makes the high-level interface strongly typed. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

This makes it easy to reuse it in the client. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

This makes it possible to expose proto-types that are only needed for accessing `client` methods, to be exposed from the `client`. Later on, there should be higher level interfaces for all these functions, so using the proto types directly wouldn't be necessary. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Copilot

Pull request overview

This PR expands the public interface of the crate by introducing a generic Bounds type and a new client API to augment metric bounds, while also refining metric/component typing and adding convenience impls for better ergonomics.

Changes:

Add Bounds<Q> and a MicrogridClientHandle::augment_electrical_component_bounds() API (plus actor/instruction/proto plumbing).
Improve component/metric typing (e.g., ElectricalComponentCategory filters, move metric to crate root) and add proto extension/graph trait impls.
Add Display for Sample and extend quantity helpers (const ctors/getters, min/max, etc.).

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
src/sample.rs	Adds `Display` for `Sample` and tightens generic bounds to include `Display`.
src/quantity.rs	Makes generated quantity constructors/helpers `const` and adds `min`/`max`.
src/metric.rs	Refactors imports to align with module visibility changes.
src/logical_meter/logical_meter_handle.rs	Updates `Metric` trait path usage to crate-root `metric`.
src/logical_meter.rs	Adjusts module visibility (`formula`) and stops exporting `logical_meter::metric`.
src/lib.rs	Introduces `Bounds`, makes `client` public, and exports `metric` at crate root.
src/error.rs	Adds `APIServerError` error kind/constructor.
src/client/test_utils.rs	Enhances mock list filtering logic (but needs trait method parity update).
src/client/proto/graph.rs	Implements component-graph `Node`/`Edge` for generated component types.
src/client/proto/electrical_component.rs	Adds helper predicates like `is_pv_inverter()` on `ElectricalComponent`.
src/client/proto.rs	Wires in new proto extension modules (`graph`, `electrical_component`).
src/client/microgrid_client_handle.rs	Updates list filters to use `ElectricalComponentCategory`; adds augment-bounds API + tests.
src/client/microgrid_client_actor.rs	Converts categories to proto ints; adds handling for augment-bounds instruction.
src/client/microgrid_api_client.rs	Extends `MicrogridApiClient` with augment-bounds RPC.
src/client/instruction.rs	Adds new `AugmentElectricalComponentBounds` instruction and updates category typing.
src/client.rs	Adjusts visibility/re-exports for client internals and component types.
src/bounds.rs	New `Bounds<Q>` type with conversions to protobuf bounds for select quantities.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-20T16:08:29Z

src/client/test_utils.rs

 #[async_trait::async_trait]
 impl MicrogridApiClient for MockMicrogridApiClient {
    async fn list_electrical_components(
        &mut self,
        _request: impl tonic::IntoRequest<ListElectricalComponentsRequest> + Send,
    ) -> std::result::Result<tonic::Response<ListElectricalComponentsResponse>, tonic::Status> {
+        let ListElectricalComponentsRequest {
+            electrical_component_ids,
+            electrical_component_categories,
+        } = _request.into_request().into_inner();
        Ok(Response::new(ListElectricalComponentsResponse {
            electrical_components: self
                .components
                .iter()
+                .filter(|c| {
+                    (electrical_component_ids.is_empty()
+                        || electrical_component_ids.contains(&c.component.id))
+                        && (electrical_component_categories.is_empty()
+                            || electrical_component_categories.contains(&c.component.category))
+                })
                .map(|c| c.component.clone())
                .collect(),
        }))


MicrogridApiClient gained the augment_electrical_component_bounds method, but this impl MicrogridApiClient for MockMicrogridApiClient does not implement it. This will fail to compile any tests using the mock; add a stub implementation returning a suitable tonic::Status (or a configurable response) so the trait impl is complete.

Copilot · 2026-03-20T16:08:30Z

src/client/microgrid_client_handle.rs

    pub async fn list_electrical_components(
        &self,
        electrical_component_ids: Vec<u64>,
-        electrical_component_categories: Vec<i32>,
+        electrical_component_categories: Vec<ElectricalComponentCategory>,
    ) -> Result<Vec<ElectricalComponent>, Error> {


The list_electrical_components docs still reference ComponentCategory::COMPONENT_CATEGORY_* constants, but the API now takes ElectricalComponentCategory. Update the documentation example names to match the new enum/type so users can copy/paste working code.

Copilot · 2026-03-20T16:08:30Z

src/client/microgrid_client_handle.rs

+    /// If the bounds for a metric are [Symbol’s value as variable is void: lower_1End.
+    /// ---- values here are considered out of range.
+    /// ==== values here are considered within range.
+    ///


The doc comment contains a broken placeholder string ([Symbol’s value as variable is void: lower_1End.) which will render incorrectly in rustdoc and confuses the meaning of the bounds. Replace it with a valid ASCII diagram or remove the diagram entirely.

Suggested change

/// If the bounds for a metric are [Symbol’s value as variable is void: lower_1End.

/// ---- values here are considered out of range.

/// ==== values here are considered within range.

///

///

/// For example, if the bounds for a metric are:

/// ```text

/// -∞ lower upper +∞

/// ---- [====] ----

/// ```

/// then:

/// - `----` values are considered out of range.

/// - `====` values are considered within range.

///

Copilot · 2026-03-20T16:08:30Z

src/client/microgrid_client_handle.rs

+    pub async fn augment_electrical_component_bounds<M, I>(
+        &mut self,
+        electrical_component_id: u64,
+        #[allow(unused_variables)] target_metric: M,
+        bounds: Vec<I>,
+        request_lifetime: Option<TimeDelta>,


augment_electrical_component_bounds takes &mut self even though it only sends on an mpsc::Sender and does not mutate handle state. Making this &self would avoid unnecessary caller-side mutability and match the other handle APIs.

Copilot · 2026-03-20T16:08:30Z

src/client/microgrid_client_handle.rs

+    pub async fn augment_electrical_component_bounds<M, I>(
+        &mut self,
+        electrical_component_id: u64,
+        #[allow(unused_variables)] target_metric: M,


The target_metric parameter is intentionally unused (suppressed via #[allow(unused_variables)]), which is a confusing API surface. Prefer removing the parameter entirely and relying on the type parameter M, or rename it to _target_metric/use PhantomData<M> so callers aren't required to pass a value that is ignored.

Suggested change

#[allow(unused_variables)] target_metric: M,

_target_metric: M,

Copilot · 2026-03-20T16:08:31Z

src/client/microgrid_client_actor.rs

+                        electrical_component_id,
+                        target_metric: target_metric as i32,
+                        bounds: bounds,
+                        request_lifetime: request_lifetime.map(|d| d.as_seconds_f64() as u64),


request_lifetime.map(|d| d.as_seconds_f64() as u64) will silently turn negative TimeDelta values into huge u64s, and also truncates fractional seconds. Validate that the duration is non-negative and convert using an integer seconds API (e.g., num_seconds() with bounds checking) to avoid sending nonsensical lifetimes to the API.

Suggested change

request_lifetime: request_lifetime.map(|d| d.as_seconds_f64() as u64),

request_lifetime: request_lifetime.and_then(|d| {

let secs = d.num_seconds();

u64::try_from(secs).ok()

}),

Copilot · 2026-03-20T16:08:31Z

src/client/microgrid_client_actor.rs

+                .map(|r| {
+                    r.into_inner()
+                        .valid_until_time
+                        .map(|t| DateTime::from_timestamp(t.seconds, t.nanos as u32))
+                        .flatten()


The timestamp conversion casts t.nanos with as u32 and then drops invalid values by returning None (via from_timestamp(...).flatten()). Use a checked conversion for nanos (and ideally validate the 0..=999_999_999 range) and consider logging/returning an error if the server returns an invalid timestamp instead of silently discarding it.

Suggested change

.map(|r| {

r.into_inner()

.valid_until_time

.map(|t| DateTime::from_timestamp(t.seconds, t.nanos as u32))

.flatten()

.and_then(|r| {

let valid_until = r.into_inner().valid_until_time;

match valid_until {

None => Ok(None),

Some(t) => {

let nanos_u32 = u32::try_from(t.nanos).map_err(|_| {

tracing::error!(

"augment_electrical_component_bounds: invalid nanos value {} from server",

t.nanos

);

Error::api_server_error(

"server returned invalid timestamp (nanos out of range)",

)

})?;

if !(0..=999_999_999).contains(&nanos_u32) {

tracing::error!(

"augment_electrical_component_bounds: nanos value {} out of allowed range",

nanos_u32

);

return Err(Error::api_server_error(

"server returned invalid timestamp (nanos out of allowed range)",

));

}

DateTime::from_timestamp(t.seconds, nanos_u32)

.ok_or_else(|| {

tracing::error!(

"augment_electrical_component_bounds: invalid timestamp (seconds={}, nanos={}) from server",

t.seconds,

nanos_u32,

);

Error::api_server_error(

"server returned invalid timestamp",

)

})

.map(Some)

}

}

Copilot · 2026-03-20T16:08:31Z

src/sample.rs

+impl<Q: crate::quantity::Quantity> std::fmt::Display for Sample<Q> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "Sample({}, ", self.timestamp)?;
+
+        if let Some(value) = self.value {
+            write!(f, " {})", value)
+        } else {
+            write!(f, " None)")


Display for Sample<Q> is currently only implemented when Q: crate::quantity::Quantity, even though the formatting only requires Q: Display (and the type already supports non-Quantity Qs via the struct/trait bounds). Consider loosening this to match the Sample<Q> bounds so Sample can be formatted for any Q: Display.

Also, the formatting currently produces extra spaces (e.g. Sample(ts, 1) / Sample(ts, None)).

Suggested change

impl<Q: crate::quantity::Quantity> std::fmt::Display for Sample<Q> {

fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {

write!(f, "Sample({}, ", self.timestamp)?;

if let Some(value) = self.value {

write!(f, " {})", value)

} else {

write!(f, " None)")

impl<Q: std::fmt::Display> std::fmt::Display for Sample<Q> {

fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {

write!(f, "Sample({}, ", self.timestamp)?;

if let Some(value) = self.value {

write!(f, "{})", value)

} else {

write!(f, "None)")

Copilot · 2026-03-20T16:08:32Z

src/bounds.rs

+//! A representation of Bounds for any metric.
+
+use crate::proto::common::metrics::Bounds as PbBounds;
+use crate::quantity::{Current, Power, Quantity, ReactivePower};
+
+/// A set of lower and upper bounds for any metric.


Bounds is documented as working "for any metric", but From<Bounds<Q>> for PbBounds is only implemented for Power, Current, and ReactivePower. As a result, augment_electrical_component_bounds won’t compile for metrics whose QuantityType is Voltage, Frequency, etc. Either add the missing From<Bounds<...>> impls (if supported by the API) or narrow the docs/API bounds to the supported quantity types.

Suggested change

//! A representation of Bounds for any metric.

use crate::proto::common::metrics::Bounds as PbBounds;

use crate::quantity::{Current, Power, Quantity, ReactivePower};

/// A set of lower and upper bounds for any metric.

//! A generic representation of bounds for a metric quantity.

//!

//! `Bounds<Q>` itself can be used with any `Q: Quantity`, but conversion to

//! the protobuf `Bounds` type (`PbBounds`) is currently implemented only for

//! `Power`, `Current`, and `ReactivePower`.

use crate::proto::common::metrics::Bounds as PbBounds;

use crate::quantity::{Current, Power, Quantity, ReactivePower};

/// A set of lower and upper bounds for a metric quantity.

shsms added 10 commits March 16, 2026 15:54

Expect ElectricalComponentCategory enum instead of i32 in client

90b0c76

This makes the high-level interface strongly typed. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Promote metric module to top-level public module

27bf2fe

This makes it easy to reuse it in the client. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Make the Quantity methods const

6570878

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Add min and max methods on Quantity types

63deb53

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Add a Bounds type generic over quantities

773fe54

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Support augmenting bounds from the client

35b40a3

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Expose ElectricalComponentCategory from the client

d847c38

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Implement Display for Sample

6a06637

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Add methods to identify the type of an inverter

abbfdff

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Copilot AI review requested due to automatic review settings March 20, 2026 16:02

Copilot started reviewing on behalf of shsms March 20, 2026 16:02 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

tiyash-basu-frequenz approved these changes Mar 20, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interface improvements#23

Interface improvements#23
shsms wants to merge 10 commits intofrequenz-floss:v0.x.xfrom
shsms:interface-improvements

shsms commented Mar 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-    /// If the bounds for a metric are [Symbol’s value as variable is void: lower_1End.
-    /// ---- values here are considered out of range.
-    /// ==== values here are considered within range.
-    ///
+    ///
+    /// For example, if the bounds for a metric are:
+    /// ```text
+    ///  -∞      lower        upper      +∞
+    ///   ----   [====]       ----
+    /// ```
+    /// then:
+    /// - `----` values are considered out of range.
+    /// - `====` values are considered within range.
+    ///

	#[allow(unused_variables)] target_metric: M,
	_target_metric: M,

-                        request_lifetime: request_lifetime.map(|d| d.as_seconds_f64() as u64),
+                        request_lifetime: request_lifetime.and_then(|d| {
+                            let secs = d.num_seconds();
+                            u64::try_from(secs).ok()
+                        }),

-                .map(|r| {
-                    r.into_inner()
-                        .valid_until_time
-                        .map(|t| DateTime::from_timestamp(t.seconds, t.nanos as u32))
-                        .flatten()
+                .and_then(|r| {
+                    let valid_until = r.into_inner().valid_until_time;
+                    match valid_until {
+                        None => Ok(None),
+                        Some(t) => {
+                            let nanos_u32 = u32::try_from(t.nanos).map_err(|_| {
+                                tracing::error!(
+                                    "augment_electrical_component_bounds: invalid nanos value {} from server",
+                                    t.nanos
+                                );
+                                Error::api_server_error(
+                                    "server returned invalid timestamp (nanos out of range)",
+                                )
+                            })?;
+                            if !(0..=999_999_999).contains(&nanos_u32) {
+                                tracing::error!(
+                                    "augment_electrical_component_bounds: nanos value {} out of allowed range",
+                                    nanos_u32
+                                );
+                                return Err(Error::api_server_error(
+                                    "server returned invalid timestamp (nanos out of allowed range)",
+                                ));
+                            }
+                            DateTime::from_timestamp(t.seconds, nanos_u32)
+                                .ok_or_else(|| {
+                                    tracing::error!(
+                                        "augment_electrical_component_bounds: invalid timestamp (seconds={}, nanos={}) from server",
+                                        t.seconds,
+                                        nanos_u32,
+                                    );
+                                    Error::api_server_error(
+                                        "server returned invalid timestamp",
+                                    )
+                                })
+                                .map(Some)
+                        }
+                    }

Conversation

shsms commented Mar 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants