How to design warnings in trait-based abstraction APIs?

help
1

I have a use case where, from my point of view, I'm in a realm in-between Result::Ok and Result::Err. It may be an xy-problem, so here's the background:

I have a function that takes a fade duration (e.g. when dimming a Zigbee device over time).
Now I have an abstraction API that allows passing in an arbitrary std::time::Duration.
However, Zigbee only allows a u16 of deciseconds, so not all durations are supported.

I decided to not fail the function on the implementation for Zigbee with an Err and bailing out, but to cap the duration to the maximum possible value.
The capped (or non-capped) actual value that has been sent via Zigbee is then returned.
So my function currently looks like this:

/// Trait for devices that can be dimmed.
pub trait Dimming: Protocol {
    /// Dim the light to the specified value.
    ///
    /// # Returns
    ///
    /// Returns the actual duration that may have been capped by the implementor.
    ///
    /// # Errors
    ///
    /// Returns an [`Error`](Self::Error) if dimming fails.
    fn dim(
        &self,
        destination: <Self as Types>::Destination,
        percent: Percent,
        rate: Duration,
    ) -> impl Future<Output = Result<Duration, Self::Error>> + Send;
}

However, I'm not completely satisfied with this design, as this suggests that everything went fine when the value was, in fact, capped.
On the other hand, I don't think that bailing out with an error is the right choice either.

How would you go about conveying to the caller, that the call succeeded but that there was a problem?
Would you do that at all, or would you error out?
If so, how could we convey to the caller what values could be acceptable - it is different for different functions of the respective trait(s) (some are u8, some u16 deciseconds).

2

I’ve done two things in that situation. The one I liked most was to return an enum or struct on the Ok path that includes the details. In your case the enum might be the warning you have in mind. I primarily like this solution because the contract is crystal clear and the caller is free to do something with the warning or ignore it.

I’ve also created a ResultWithWarning that could be easily coerced into a Result. This worked well for command line programs where the warning was likely to be emitted to the console at the appropriate verbosity level.

3

Thank you. I Was thinking about something similar, e.g. returning a Result<Option<Duration>, Error> with Ok(None) meaning success and Ok(Some(capped_duration)) meaning that the duration was capped.

Now that I'm thinking of it an alternative would be to hard-error and provide the max value information to the caller like so:

/// Trait for devices that can be dimmed.
pub trait Dimming: Protocol {
    /// The maximum allowed duration for dimming.
    const MAX_DIM_DURATION: Duration;

    /// Dim the light to the specified value.
    ///
    /// # Errors
    ///
    /// Returns an [`Error`](Self::Error) if dimming fails.
    fn dim(
        &self,
        destination: <Self as Types>::Destination,
        percent: Percent,
        rate: Duration,
    ) -> impl Future<Output = Result<(), Self::Error>> + Send;
}

That would allow the call site to decide to either just go for any value and possibly fail hard, or to call

t.dim(destination, percent, duration.min(T::MAX_DIM_DURATION)).await?;
4

I would either give another param like &mut Diagnostics, which can be used to emit error code + context + level, or a struct. So that you have both the diagnostics and result.

More generally, I believe using Duration is a mistake. I would have defined my own duration and provided TryFrom<Duration> for MyDuration and this way the method would always receive the right duration no matter what.

5

this is a type mismatch.

it might be DimmingDuration, or ZigBeeDuration, or whatever you like to call it, but its definitely not std::time::Duration.

provide mechanism instead of policy. don't make the decision for the user, let the user express their intention precisely and make the decision themselves.

ideally, I'd like the api to support a flexible rate spec instead of a single duration value. you can make a custom Rate type, or maybe you can make use of the standard Range, RangeInclusive, RangeTo or RangeFrom.

I understand this is not always feasible, but even for a single duration value, you can at least make a fallible conversion from std::time::Druation, so the error (or warning?) happens at unit conversion time as opposed to execution time.

for example, with a fugit-style API, I would imagine the user code could look like this:

// fugit-style constructor from literal values

// case 1: specify exact value, should fail if the implementation is not able to execute
let rate = 2.5.secs()?;
// case 2: specify an upper bound, ok if the implemention caps it to some smaller value
let rate = 2.5.secs_at_most()?;
// case 3: specify an allowed range:
let rate = 2.5.secs_plus_minus(0.5)?;

protocol.dim(dest, percent, rate).await?;
6

Thanks. The problem is that the traits in question are an abstraction layer across multiple smart home protocols so there's bound to be compromises.
E.g. on the Zigbee level I have the native Zigbee API as specified by the Zigbee ZCL specification.
I now want to implement the API as exposed by abstracted traits by the smart home abstraction layer APIs and translate those to native Zigbee API calls, so that the user doesn't need to care whether they're using Zigbee or Matter/Thread or whatnot.
Of course there's bound to be type mismatches in the case of different possible ranges that are allowed in the respective protocols and the abstraction layer needs to find a protocol-agnostic compromise.

As for your API suggestion: In the example method in question I cannot provide a range, since this is not meaningful for the command that this is supposed to portray.

By the way: The error currently does happen at unit conversion, namely at the implementation of the abstraction API in the Zigbee crate (which is behind an appropriate feature flag). The question is how to handle those errors.

To clarify, this is the native Zigbee API:


/// Trait for the Level cluster.
pub trait Level {
    /// Move to level command.
    ///
    /// # Errors
    ///
    /// Returns an [`Error`] if execution of the command failed.
    fn move_to_level(
        &self,
        destination: Destination,
        level: u8,
        transition_time: Uint16,
        options: Options,
    ) -> impl Future<Output = Result<(), Error>> + Send;

    ... other trait methods...
}

And this is the Zigbee on-site abstraction layer implementation:

impl Dimming for Coordinator {
    async fn dim(
        &self,
        destination: <Self as Types>::Destination,
        percent: Percent,
        rate: Duration,
    ) -> Result<Duration, Self::Error> {
        let deci_seconds: u16 = rate.as_deci_secs().try_into().unwrap_or(u16::MAX);
        let deci_seconds: Uint16 = deci_seconds.try_into().unwrap_or(Uint16::MAX);

        crate::Level::move_to_level(
            self,
            destination.into(),
            percent.into(),
            deci_seconds,
            Options::default(),
        )
        .await?;

        Ok(Duration::from_deci_secs(deci_seconds.as_u16().into()))
    }
}
7

understandable. but what kind of compromises do you accept?

when some aspect of the protocols behave significant differently, do you prefer:

a) the user of the abstract api to (silently?) experience different behavior even with the same arguments. taking the dimming duration as an example, is it intended that the user can specify the same duration, say 100 seconds, but they will actually get a 30 second dimming effect using protocol A, but 60 seconds using protocal B? or:

b) limit the api so it only accepts the common denominator of all the protocol's capability, e.g. suppose protocal A supports 0 to 60 seconds with a 1 millisec resolution, and protocal B supports 1 to 3600 seconds with a granularity of 0.1 second, then your "portable" api would only accept the range between 1 to 60 seconds, rounded to the nearest 0.1 second?

then make the unit conversion explicit. why would the "action" function to return a "conversion" error type (or warning)?

if you want to go the common denominator path, the dimming duration type can be a concrete type of the abstraction layer, e.g. a newtype wrapper over std::time::Duration with both fallible conversion and infallible capping conversion, the validation is done in the abstraction layer.

if you want to the user to be able to control/observe implementation specific behavior, the duration type can be an associated type, and the abstraction layer define the generic interface, and the validation logic is done by the implementor.

remember, you can always provide additional "convenient" api on top of "core" api for common use cases, but if you bake the policy into the mechanism api design, you cannot change or extend it without a major break.

// the "core" api, i.e. the mechanism

/// valid range between XX to YY seconds
pub type DimmingDuration(Duration);

/// the conversion methods
/// make `DimmingDuration` an trait if we allow implementation specific behavior
impl DimmingDuration {
    // return the capped value in `Err` when the input is out of accepted range
    fn try_from(duration: Duration) -> Result<Self, Duration> {...}
    // make the conversion, alwaya cap to the valid range
    fn from_duration_capped(close_to: Duration) -> Self {...}
}

trait Dimming {
    /// the dim "action", no unit conversion
    async fn dim(&self, ..., rate: DimmingDuration) -> Result<(), Self::Error>; 
}

// optional convenient api, can be free function or extension trait methods

/// uncapped version.
// what's the strategy when argument is invalid, i.e. duration too long?
// example fallible conversion
type DimmingResult<D> = Result<(), DimmingError<<D as Dimming>::Error>>;
async fn dim_in<D: Dimming>(d: &D, rate: Duration) -> DimmingResult<D>;
async fn dim_in_secs<D: Dimming>(d: &D, rate_secs: f32) -> DimmingResult<D>;
async fn dim_in_millis<D: Dimming>(d: &D, rate_millis: u32) -> DimmingResult<D>;

/// capped version, conversion is infallible and return the converted value
async fn dim_in_at_most<D: Dimming)(d: &D, rate: Duration) -> Result<Duration, D::Error>;
async fn dim_in_secs_at_most(...);
async fn dim_in_millis_at_most(...);
8

Neither a, nor b.
With regards to a), I don't want to fail silently really. I'd like the user to get some kind of feedback. And in the case of b) this is not what I want at all. The common API shall be decoupled from possible implementors, so it cannot know the different limits of the protocols that will implement it ahead of time.

That's actually a pretty good idea. I can make the type of duration an associated type of the trait with a bound TryFrom<Duration> or similar, and then require that associated type as the type of the duration parameter in the trait's function calls.

/// Trait for devices that can be dimmed.
pub trait Dimming: Protocol {
    /// The type of duration used in the protocol implementation.
    type Duration: TryFrom<std::time::Duration>;

    /// Dim the light to the specified value.
    ///
    /// # Errors
    ///
    /// Returns an [`Error`](Self::Error) if dimming fails.
    fn dim(
        &self,
        destination: <Self as Types>::Destination,
        percent: Percent,
        rate: Self::Duration,
    ) -> impl Future<Output = Result<(), Self::Error>> + Send;
}