:building_construction: Enhance device API for drivers

rneswold commented 1 year ago

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 commented 1 year ago

Commit 8c37bdf6313fe255fcb5cfc0e549067e81fe0d64 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 commented 1 year ago

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

rneswold commented 1 year ago

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 commented 1 year ago

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

rneswold commented 1 year ago

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

rneswold commented 1 year ago

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 commented 1 month ago

I refactored the code in #126. The "previous value" issue has been resolved. Now maybe I can make progress on tuples of read-only registers.

DrMemCS / drmem