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.

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

🏗️ Enhance device API for drivers #13

Open
rneswold opened this issue Oct 28, 2022 · 6 comments
Open

🏗️ Enhance device API for drivers #13

rneswold opened this issue Oct 28, 2022 · 6 comments
Assignees
Labels
difficulty:hard Issue will be a challenge to complete enhancement New feature or request help wanted Extra attention is needed refactor Code works but could be written better

Comments

@rneswold
Copy link
Contributor

rneswold commented Oct 28, 2022

This issue will be used to track improvements to the device API used by drivers. There's a final API I'd like to achieve, but it seems a little complicated to do at once. So the migration can be done in phases where each phase will provide a useful improvement. Since there are only a handful of drivers, these changes will be simple to apply -- almost mechanical.

Current API

When a driver defines a read-only device, it uses .add_ro_device() which has the following signature:

async fn add_ro_device(&self, name: Base, units: Option<&str>) ->
    Result<(ReportReading, Option<Value>)>;

ReportReading is a closure which takes a device::Value and sends it to the backend storage. The second value in the returned tuple is the last value reported by the device. If you're using the simple backend, this will be None since the simple backend has no persistence. The Redis backend will return Some value.

To register a settable device, the function is very similar:

async fn add_rw_device(&self, name: Base, units: Option<&str>) ->
    Result<(ReportReading, RxDeviceSetting, Option<Value>)>;

The main difference is the return value is a 3-tuple. The previous value has been moved to the last position and the second element is now an mpsc::Receiver<(device::Value, oneshot::Sender)>. This Receiver accepts setting requests from clients. This API works, but I'd like it to be a little more strict and remove some of the boilerplate (especially needing to verify the correct type.)

Phase 1

The first phase would be to have the driver specify the desired type of the device, instead of having to convert its value into a device::Value. The signatures to both registration methods would look something like:

async fn add_ro_device<T: Into<device::Value>>(&self, name: Base, units: Option<&str>) ->
    Result<(ReportReading<T>, Option<T>)>;

async fn add_rw_device<T: Into<device::Value>>(&self, name: Base, units: Option<&str>) ->
    Result<(ReportReading<T>, RxDeviceSetting<T>, Option<T>)>;

In this API, the return values are strongly typed to a (non-device::Value) value. We add the requirement that the type T must be a type that can be turned into a device::Value. ReportReading is now a closure that accepts the type T and converts it to a device::Value before sending it on. The handle that accepts settings is now an async closure that waits for the settings and tries to convert it type T. If it can't, the closure will return a TypeError so the driver only sees settings that have been converted to the expected type.

Phase 2

Each time a device reading is reported, the system gives it its own timestamp. There are some drivers which compute values based on a reading. It would be nice if those devices all had the same timestamp. For instance, a weather driver that obtains weather information from a website should use the same timestamp for all the information.

For this phase's API change, you can register a tuple of devices. Each element of the tuple has its own device name and units and type. However, the devices are only associated with the tuple so the ReportReading closure only accepts a tuple of values. The backend must guarantee that all values in the tuple get associated with the same timestamp.

Something like:

    let (devices, prev_vals) = core
        .add_ro_devices::<(bool, f64)>(
            (("enable", None), ("brightness", Some("%")))
        )
        .await?;

and to update the devices:

    devices((true, 50.0)).await?;

Only read-only devices can be grouped. There is no equivalent API change for settable devices.

Phase 3

When registering a tuple of devices, any device whose type is Option<T> will be optionally saved to the backend. For instance, in my sump pump driver, the state device is boolean and will change from true to false to true as the sump pump turns on and off. But the duty and in-flow devices only get calculated when the pump turns off. So when the pump turns on, the driver would send (true, None, None) and when it turns off, it sends (false, Some(duty), Some(in_flow)).

Comments

Of these three phases, I think phase 2 will be the hardest. Phase 3 should be easy and, in fact, can probably be done while working on phase 2.

@rneswold rneswold added this to the Second release milestone Oct 28, 2022
@rneswold rneswold added design Formalize specification(s) enhancement New feature or request help wanted Extra attention is needed refactor Code works but could be written better labels Oct 28, 2022
@rneswold rneswold self-assigned this Dec 31, 2022
@rneswold rneswold mentioned this issue Jan 2, 2023
@rneswold rneswold linked a pull request Jan 2, 2023 that will close this issue
@rneswold rneswold removed design Formalize specification(s) help wanted Extra attention is needed labels Jan 5, 2023
@rneswold
Copy link
Contributor Author

rneswold commented Jan 5, 2023

Commit 8c37bdf in pull request #44 completes phase 1.

There was a slight change in design due to an inspiration. Rather than use closures for incoming settings, I used a stream from the tokio_stream crate. It was much easier handling the type conversion and automatic error reply with this approach. So, for settable devices, the function becomes:

async fn add_rw_device<T: Into<device::Value>>(&self, name: Base, units: Option<&str>) ->
    Result<(ReportReading<T>, SettingStream<T>, Option<T>)>;

@rneswold rneswold removed a link to a pull request Jan 5, 2023
@rneswold rneswold added the help wanted Extra attention is needed label Jan 5, 2023
@rneswold
Copy link
Contributor Author

The rest of this issue can be pushed to v0.3.0.

@rneswold
Copy link
Contributor Author

rneswold commented Apr 4, 2023

I want to add another enhancement to this API.

Currently, a driver's life is as follows:

  • Driver manager lets driver register devices and allocate resources.
  • Driver manager invokes the driver's run method.
  • If driver crashes, go back to step one.

With the "logic node" subsystem, restarting at step 1 breaks any logic node trying to set or read a device (because re-registering it makes new handles and I don't think I want to iterate through the logic handlers to fix them.)

The updated API will use 3 stages:

  • Driver manager lets driver register devices.
  • Driver manager lets driver allocate resources.
  • Driver manager invokes the driver's run method. It passes in the registered driver channels.
  • If driver crashes, go to step two.

This keeps the registered driver information safe and always up to date.

@rneswold rneswold added the difficulty:hard Issue will be a challenge to complete label Apr 19, 2023
@rneswold
Copy link
Contributor Author

acd53ad added the 3-phase start-up for drivers.

@rneswold rneswold removed this from the Release v0.3.0 milestone May 14, 2023
@rneswold
Copy link
Contributor Author

I removed the milestone because the remaining features are things that would be nice to have instead of required.

@rneswold rneswold added this to the Release v0.4.0 milestone Jun 13, 2023
@rneswold
Copy link
Contributor Author

rneswold commented Jun 13, 2023

Milestone v0.4.0 has no issues associated with it, so I'm adding this issue because it would be nice to support the tuple API for drivers.

So, to close out this issue, we need:

  • The features described by "Phase 2" in the original post.
  • The features described by "Phase 3" in the original post.
  • And we need to fix something that broke in the 3-part lifetime of drivers.

The last point has to do with when the drivers register a device; they would receive a handle for reporting updates, a handle for receiving settings (for settable devices), and they would receive the previous value so the driver could restore its state.

Right now, registering the devices is a one-time event and there is no driver instance to save the previous values. In other words, the previous value logic needs to be moved to the create_instance part of the driver's lifetime.

@rneswold rneswold changed the title Enhance device API for drivers 🏗️ Enhance device API for drivers Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty:hard Issue will be a challenge to complete enhancement New feature or request help wanted Extra attention is needed refactor Code works but could be written better
Projects
None yet
Development

No branches or pull requests

1 participant