buttplugio / buttplug

Rust Implementation of the Buttplug Sex Toy Control Protocol
https://buttplug.io
Other
867 stars 61 forks source link

Rethink how Client Device Actuators are handled/exposed #552

Open qdot opened 1 year ago

qdot commented 1 year ago

From @Yoooi0 on discord:

you probably thought about this before but I'll repeat my suggestion for v4 i posted here some time ago, i think all actuators should be collapsed to one Actuators list instead of having them be in 3 separate lists, and each actuator would have an additional Capabilities flags or whatever it would be called that would hold the data/message types the actuator accepts

so you could have a Rotate type actuator that supports position based commands (ScalarCmd), or speed + direction commands (RotateCmd), then UI would use actuator type + actuator capabilities to display stuff instead of using actuator type and in which list it exists in (ScalarCmd/RotateCmd/LinearCmd)

: another example is OSR which has L0 linear actuator that theoretically accepts position only (ScalarCmd) and position + duration (LinearCmd) commands, with the capabilities system you would have one actuator with two ways to send data

the same thing could also be applied to sensors if possible (read/subscribe capabilities), and will also fix the ID mismatch issue

i implemented each actuator type as a separate class: https://github.com/Yoooi0/Buttplug.Net/blob/master/source/Buttplug.Net/ButtplugDeviceActuator.cs

then the device holds a list of all actuators: https://github.com/Yoooi0/Buttplug.Net/blob/master/source/Buttplug.Net/ButtplugDevice.cs#L44-L47

so instead of:

foreach(var device in client.Devices)
foreach(var actuator in device.RotateActuators)
client.RotateAsync(new RotateCommand(actuator.Index, speed, clockwise), token);

you can do something like this:

var rotateActuators = client.Devices.SelectMany(d => d.RotateActuators);
foreach(var actuator in rotateActuators)
actuator.RotateAsync(speed, clockwise, token);
Yoooi0 commented 1 year ago

Some examples/explanation for the proposal.

Current sample device ```json { "name": "Sample Device", "messages": { "ScalarCmd": [ { "ActuatorType": "Vibrate" }, { "ActuatorType": "Position" } ], "RotateCmd": [ { "ActuatorType": "Rotate" } ], "LinearCmd": [ { "ActuatorType": "Position" } ], "SensorReadCmd": [ { "SensorType": "Battery" } ], "SensorSubscribeCmd": [ { "SensorType": "Pressure" }, { "SensorType": "Battery" } ] } } ```

Given the above sample device there are a few issues:


Proposed sample device ```json { "name": "Sample Device", "actuators": [ { "ActuatorType": "Vibrate", "MessageTypes": [ "ScalarCmd" ] }, { "ActuatorType": "Rotate", "MessageTypes": [ "RotateCmd" ] }, { "ActuatorType": "Position", "MessageTypes": [ "ScalarCmd", "LinearCmd" ] } ], "sensors": [ { "SensorType": "Battery", "MessageTypes": [ "Read", "Subscribe" ] }, { "SensorType": "Pressure", "MessageTypes": [ "Subscribe" ] } ] } ```

Given the above proposed device:

Possible issues:

It can also be expanded to raw commands:

"endpoints": [
  {
    "Name": "endpoint"
    "MessageTypes": [
      "Read",
      "Write"
      "Subscribe"
    ]
  }
]

instead of having 3 separate lists for read/write/subscribe endpoint commands.

qdot commented 1 year ago

A solution for part of the rotate issue in terms of a Client API implementation: The only difference between ScalarCmd Rotate and RotateCmd is that RotateCmd has a 2d direction (clockwise/counter-clockwise). Under a first-class actuator system, a Rotate actuator can just have a "HasDirection" check. If it's a actuator that maps to ScalarCmd rotate, that's false, if it's RotateCmd, that's true. Then it's up to the user to check, but the Rotate method of that actuator could take (-1.0, 1.0) and throw an error if x < 0 when it's a ScalarCmd Rotate.

In the protocol, v4 will probably fix this by getting rid of RotateCmd and adding negatives in the definable ranges for ScalarCmd, I just wasn't quite ready to make that jump yet when I put out v3.

Yoooi0 commented 1 year ago

Yea, I also thought about removing RotateCmd and having it be replaced with ScalarCmd with (-1, 1) range but I think there would also have to be a new actuator type like Motor so there is a way to tell which rotate actuator is which.

So Rotate type would be able to support ScalarCmd and LinearCmd messages, while Motor type would only be able to support ScalarCmd.

qdot commented 9 months ago

Annnnd 9 months later, finally starting to think through this for implementation, and I have a small tweak that I think should cover everything.

Devices will be trees of features, each of which can have both actuators AND sensors:

      /-> Feature -> List of Actuators/Sensors 
Device -> Feature -> List of Actuators/Sensors
      \-> Feature -> List of Actuators/Sensors

For the design in this issue so far, we got through the issue of "multiple ways to command the same actuator on a device, by making the actuator first class", which gets us most of the way there.

What happens when we have a motor we can control, that we also have an encoder for? In v3, as well as in the above design, while we at least collapse context for actuators, we don't have any way to programmatically share context between actuators/sensors.

This is an issue in Buttplug right now. With the KGoal Boost, we get two readings: Raw and calibrated, from the device. These are two readings for the same feature, but that's not obvious. The above solution won't quite fix this, because I still can't define two different SensorRead's on the same feature. Under a feature tree, we could define multiple sensor read/sub messages depending on what version of the data a user wants, but also programmatically show that they're coming from the same basic thing.

I realize we don't usually expect people to close control loops via buttplug so the motor/encoder example is a bit much, but we may run into other situations where this is handy.

For 98% of Buttplug developers, this is going to be too much info and they won't use it. However, for anyone developing a client, this is just an extra level of iteration they can abstract away from the user and still present things as groups of vibrators, rotators, whatever. If people really DO want to get fancy and see what groupings are, they have the option if the feature tree is exposed in whatever the client library is.

For 98% of Buttplug devices, this is going to be pretty much useless too, since they're just a single vibrator. It'll just be an extra layer to unpack. I don't really feel like that's a big deal either. Our goal is wide support for devices and controlling them, so at some point we've gotta have complexity.

For the 2% of either of the above, where things get complicated (which will probably continue to grow as toys get more complex), I think this should cover us.

What I've said so far here only affects device enumeration, though. I'm still figuring out how I want command structure to work around this. We can make every feature and every actuator/sensor under a feature have unique IDs, or we can require sending both feature and actuator/sensor id with a message (i.e. to differentiate when we have 2 SensorRead's with different units or whatever).

@Yoooi0 @blackspherefollower lemme know your thoughts.

Yoooi0 commented 9 months ago

The above solution won't quite fix this, because I still can't define two different SensorRead's on the same feature.

I think you would not define two sensor Read message types for one sensor, but rather have two separate sensors. Obviously then you can't know what each sensor does because they would have the same SensorType, so I think just adding a Name/Description property to sensors and actuators would solve that:

{
  "sensors": [
    {
      "Name": "Battery (%)"
      "SensorType": "Battery",
      "MessageTypes": [ "Read", "Subscribe" ]
    },

    {
      "Name": "Battery (Raw)"
      "SensorType": "Battery",
      "MessageTypes": [ "Read", "Subscribe" ]
    },
  ]
}

Under a feature tree, we could define multiple sensor read/sub messages depending on what version of the data a user wants, but also programmatically show that they're coming from the same basic thing.

If I'm understanding correctly, under the feature tree you would want to have one "calibrated" and one "raw" feature, each feature with one sensor with one Read message type?

If we assume we can have multiple sensors with the same SensorType in the list like above, then features only really group the sensors and actuators but do not provide any benefit from api or client perspective. Unless there is a more defined connection between sensors/actuators then features really just convert:

var motor = device.Actuators[0];
var calibratedSensor = device.Sensors[0];
var rawSensor = device.Sensors[1];

to

var motor = device.Features[???].Actuators[0];
var calibratedSensor = device.Features["Calibrated"].Sensors[0];
var rawSensor = device.Features["Raw"].Sensors[0];

In both cases client has to know the specific device it is working on to know what each feature/sensor/actuator names or indexes actually do.

So you can arrange the data in many ways all it really changes is how you access it in the client. You could even put it sensors inside the actuator to link them more:

{
  "actuators": [
    {
      "ActuatorType": "Position",
      "MessageTypes": [ "ScalarCmd", "LinearCmd" ],
      "Sensors": [
        {
          "SensorType": "Unknown",
          "MessageTypes": [ "Read" ]
        }
      ]
    }
  ]
}
var motor = device.Actuators[0];
var calibratedSensor = motor.Sensors[0];
var rawSensor = motor.Sensors[1];

What if the features are its own separate thing but they only include indexes that map to the actuators/sensors lists:

{
  "features": [
    "raw": {
      "Actuators": [0],
      "Sensors": [0]
    },
    "calibrated": {
      "Actuators": [0],
      "Sensors": [1]
    }
  ]
}

Then the api implementation could have some helper methods that gives the client sensors/actuators based on the feature it wants.

We can make every feature and every actuator/sensor under a feature have unique IDs

That seems like it could be prone to errors, in original solution the ID was just the index in the list. I think better solution is to send feature index and actuator/sensor index in that feature in each command.

qdot commented 9 months ago

@Yoooi0 You're reading slightly too far into my example. The Boost has 2 ways to get the same reading, but that's also currently the only toy that does that. So no, don't wanna take the raw/calibrated and run with it everywhere. It was just a way to talk about getting different units from the same feature.

That's the major issue with our sensor system in general. If this were... pretty much any other type of engineering, we could figure out what units we're working under and standardize to that. No such luck working with sex toys. We're not sure how we're gonna get things like pressure or accelerometer data.

That's why I brought this up.

And yeah, there would definitely be names/descriptions in the features as well as actuators/sensors. So we could say a feature is "motor", positional actuator "Moves motor to position", sensor "Tracks motor position (with some sort of units)", etc...

Yoooi0 commented 9 months ago

So no, don't wanna take the raw/calibrated and run with it everywhere. It was just a way to talk about getting different units from the same feature.

Oh yea, I was just using that in each example, didnt mean that each device would have raw/calibrated stuff.

And yeah, there would definitely be names/descriptions in the features as well as actuators/sensors. So we could say a feature is "motor", positional actuator "Moves motor to position", sensor "Tracks motor position (with some sort of units)", etc...

If actuators/sensor will have names and descriptions then features dont add much, they just nest everything down one more level. They also increase the complexity of each command and library implementation. Just adding multiple sensors/actuators with the same type but different name and description to the list seems like an easy solution.

Such grouping by feature would be nice from client perspective but I think internally they can easily just be a separate thing that contain indexes that point to actual sensors/actuators lists (I guess this can be prone to errors), the library should then implement all the necessary helper methods.

Also for 99.99% of devices features will add an additional json level (also need a default feature name?). If features are its own thing then they would be just an empty array.

qdot commented 9 months ago

What if the features are its own separate thing but they only include indexes that map to the actuators/sensors lists:

Ok having sat with this for a week, I'm warming up to this idea. It's easy to ignore for everyone who doesn't want it and it's easy to leave out for most of the devices that won't need it.

How we define these in the device config is gonna be... interesting, but I was considering giving every device/actuator/sensor its own UUID anyways (which we probably won't ship to the client but it'll make matching easier in the server situation and will just be built in if I ever end up moving device config to sqlite).

Yoooi0 commented 9 months ago

It's easy to ignore for everyone who doesn't want it and it's easy to leave out for most of the devices that won't need it.

Yup that's my thinking too.

I was considering giving every device/actuator/sensor its own UUID anyways (which we probably won't ship to the client but it'll make matching easier in the server situation and will just be built in if I ever end up moving device config to sqlite)

Actually if the library implementation is able to use the actuator/sensor UUID then it can be another way to send commands. So you would be able to either send a command with device index + actuator/sensor index, or with just actuator/sensor UUID.

In such case a Sensor/Actuator class implementation would not need to know its owning device index and its index in the list and could just send commands itself using its UUID. It would send commands directly using the client instead of how I implemented it via the device: (ButtplugDeviceScalarActuator provides actuator index, Device.ScalarAsync provides device index)

https://github.com/Yoooi0/Buttplug.Net/blob/b2ab10296cdc6f2ffd13eac39b3b2095d51a3719/source/Buttplug.Net/ButtplugDeviceActuator.cs#L36-L43

qdot commented 9 months ago

Actually if the library implementation is able to use the actuator/sensor UUID then it can be another way to send commands. So you would be able to either send a command with device index + actuator/sensor index, or with just actuator/sensor UUID.

Yeah, trying to decide on that. Buttplug has this whole thing of supposedly not letting the client know what toys a person might be using, but we never actually followed it due to the device names not being easily changeable.

Also not quite sure how much I want to dive into both rebuilding the message formats AND the references, mostly worried about this being another Forever Project. We'll see how it goes once I get started on it.

CAD97 commented 8 months ago

Random thought: a device could realistically allow both reading and commanding, say, a position, so the differentiation of actuators from sensors at the protocol level seems incidental. If you were to merge those, the protocol could look like

{
    "DeviceName": "Sample Device",
    "DeviceFeatures": [
        {
            "FeatureName": "Battery (%)",
            "FeatureType": "Battery",
            "ValueRange": [[ 0, 100 ]],
            "MessageTypes": [ "Read", "Subscribe" ]
        },
        {
            "FeatureName": "Battery (Raw)",
            "FeatureType": "Battery",
            "ValueRange": [[ 0, 65535 ]],
            "MessageTypes": [ "Read", "Subscribe" ]
        },
        {
            "FeatureName": "Inserted Vibrator",
            "FeatureType": "Vibrate",
            "StepCount": 20,
            "MessageTypes": [ "Scalar" ]
        },
        {
            "FeatureName": "Insertion Depth",
            "FeatureType": "Position",
            "ValueRange": [[ 0, 20 ]],
            "MessageTypes": [ "Scalar", "Linear", "Read" ]
        },
        {
            "FeatureName": "Firmware",
            "FeatureType": "Unknown",
            "MessageTypes": [ "RawRead", "RawWrite" ]
        }
    ]
}

and I don't think that feels over-engineered.

qdot commented 7 months ago

Ok, well, tackling this yet again in the new year.

Good news is, every time I revisit this, the more I'm becoming ok with the simplifications everyone is recommending. Both ID'ing by actuator alone AND simplifying message configurations to just be features that can both read and write are sounding good on their face, both from the perspective of message references and configuration.

The one things missing now is how to actually express those configs, because the json files are just getting ridiculous at this point. I'm looking at my sqlite branch from last April and considering continuing implementation on that, because this is now actively blocking per-actuator limit configs.

qdot commented 4 months ago

Ok, well, after a year of fucking around trying to do this in a RDBMS and learning a bunch about sqlite then chucking it all, I'm implementing this in the old JSON format now. Still looks mostly like @CAD97's idea with a slight variation:

{
    "DeviceName": "Sample Device",
    "DeviceFeatures": [
        {
            "FeatureName": "Battery (%)",
            "FeatureType": "Battery",
            "Sensor": {
              "ValueRange": [[ 0, 100 ]],
              "MessageTypes": [ "SensorReadCmd" ]
            }
        },
        {
            "FeatureName": "Inserted Vibrator",
            "FeatureType": "Vibrate",
            "Actuator": {
              "StepCount": 20,
              "MessageTypes": [ "ScalarCmd" ]
            }
        },
        {
            "FeatureName": "Trackable Linear Motor",
            "FeatureType": "PositionWithDuration",
            "Actuator": {
              "StepCount": 100,
              "MessageTypes": [ "LinearCmd" ]
            },
            "Sensor": {
              "ValueRange": [[0, 100]],
              "MessageTypes": ["SensorReadCmd", "SensorSubscribeCmd"]
            }
        },
        {
            "FeatureName": "Raw I/O",
            "FeatureType": "Raw",
            "Raw": {
              "Endpoints": ["rx", "tx"],
              "MessageTypes": [ "RawReadCmd", "RawWriteCmd", "RawSubscribeCmd"]
            }
        }
  ]

So instead of everything existing on the feature object root, there's now 3 subtypes:

I really don't like having to consider a whole bunch of nullable fields and possibly invalid messages by storing everything on root, but I also get that descriptions on everything is overkill, so this feels like a decent tradeoff.

Right now I'm rebuilding the device config system to both deal with this layout as well as handle user config serialization in a way that isn't shit. I'll probably ship that as part of the Buttplug v7 line while starting work on the new message system, which I may try to beta with some client developers (all 3 or 4 of them that exist :| ) before shoving the new message spec out this time.

Yoooi0 commented 4 months ago

I think that should work, although few things that crossed my mind:

qdot commented 4 months ago

@Yoooi0

Ok, so, questions 1 and 3 overlap a bit.

If we assume a feature will have no more than 1 actuator/sensor/raw, then we'd use the feature index, because no messages will overlap on actuators/sensors (raw is extremely special/rare/weird. It'll only ever be on its own feature, will only ever be one of them. I honestly doubt most client devs will even implement it). The valid message types for actuator/sensor/raw are all disjoint subsets of the device message set.

If we do have multiple adapters/sensors, then yeah, shit gets weird. I've been trying and failing to come up with a situation where there'd be multiple actuators on a feature, and the only situation I can think of right now where we'd have multiple sensors is if we had, say, a motor with position and temperature output or something. At that point I feel like we're starting to operate outside of the simple stuff this library was meant for anyways (i.e. if you're trying to close a control loop and monitor how it affects motor temperature through buttplug, maybe you should be considering an Actual Controls Library :) ).

Agreed on PositionWithDuration. Just using ScalarCmd/LinearCmd does work. Thanks for that callout.

Cmd/Reading is just the suffix I use to denote that a message is a device command (actuator message) or device reading (sensor message return).

qdot commented 4 months ago

Also cc @blackspherefollower because they're the one with all the devices.

Any ideas about single versus multiple actuators/sensors per feature?

CAD97 commented 4 months ago

I think the only cases where multiple actuators or sensors for the "same" feature make sense are either:

The goal should be to hopefully simplify the protocol by flattening it. This is at its core an Io(s)T protocol, and individual devices aren't likely to be complicated enough that just listing it as two separate related features in the array would be a problem. A "feature" is the user-meaningful unit of address here.

(I also don't really see ValueRange and StepCount as being incompatible; the former defines a range and the latter the granularity within that range. They aren't nullable in my sketch as much as defaulted to [[0, 1]] and ValueRange[0][1] - ValueRange[0][0] respectively. While a feature could provide both read/write with different range/granularity from each other, that just seems awful to use. And raw is a special weird edge case, perhaps an untagged enum repr in serde parlance.)

blackspherefollower commented 4 months ago

I'm pretty jetlagged right now, but it took me a second to work out that feature is effectively just the superclass of actuator/sensor and not necessarily a physical part (I've got a device that's both suction and insertable connected by flexible cable and both ends have separate heating elements: the heaters would be a separate feature rather than the insertable element having both heater and vibrator as actuators).

Being able to associate a sensor with an actuator on the same physical part of a device could be handy, but I think it's rare enough for devices to have sensors and multiple physical parts that the description will suffice.

To extend the talk about Position via LinearCmd/ScalarCmd, we also talked about exposing Oscillate/OscillateWithRange on positional devices so that end users didn't have to reinvent the wheel each time they needed to set a positional device to a speed. If these are considered as a different feature, then a way to know they affect the same actuator is going to be needed (otherwise clients will just iterate over all the features and override commands they just applied).

qdot commented 4 months ago

@blackspherefollower Where did the "OscillateWithRange" discussion happen? I can't find anything on discord history nor here, so I don't remember what the context was.

Also, I should say: This isn't going to be set in stone quite yet. Unlike prior releases, my plan for now is to implement this format in our device configs and inside the library, so we can at least make sure it fits internally and that all of our current device configs conform to it. I'd like to actually release another few versions of the library using the current spec while it sits on top of this configuration and we figure things out. Once we're sure this is going to cover our needs then I'll put the messages on top of it and update the spec (and I may include some way to beta the new message versions during that time)

blackspherefollower commented 4 months ago

Where did the "OscillateWithRange" discussion happen? I can't find anything on discord history nor here, so I don't remember what the context was.

The important bit was to do with the idea that positional linear devices could have ScalarCmd Oscillation support exposed and implemented in the buttplug server; this would mean that the same physical actuator might be controlled by either ScarlarCmd or LinearCmd. It'd be important to let the client know not to send both commands because one will override the other.

This last came up in the conversation we had around the depth control on the Lovense Solace (I've copied it into https://github.com/buttplugio/buttplug/issues/611#issuecomment-2034782833 ); it's only mentioned in passing though.

Also, I should say: This isn't going to be set in stone quite yet. Unlike prior releases, my plan for now is to implement this format in our device configs and inside the library, so we can at least make sure it fits internally and that all of our current device configs conform to it. I'd like to actually release another few versions of the library using the current spec while it sits on top of this configuration and we figure things out. Once we're sure this is going to cover our needs then I'll put the messages on top of it and update the spec (and I may include some way to beta the new message versions during that time)

That sounds good. Technically the server could just start supporting the new protocol and without it being documented or exposed by clients the downgrade logic should just make this invisible to existing users (expect of course the overhead of implementing the downgrades upfront). I'd be hitting the engine anyway, so maybe this just goes in a beta branch with CI enabled? Lots of ways to skin this cat.

qdot commented 4 months ago

Technically the server could just start supporting the new protocol and without it being documented or exposed by clients the downgrade logic should just make this invisible to existing users (expect of course the overhead of implementing the downgrades upfront).

This is exactly the plan. Right now I'm mostly worried about how we store device capabilities. Next, we worry about how we communicate them, then finally, how that looks in relation to device command/reading messages going forward.

I'm implementing the config updates now, as well as the internal storage of device configurations to work with the current message system. This will probably end up in me making new top level messages that I'll use as conversion endpoints, but just won't expose in the protocol until we figure out how everything is going to work.

The other thing I don't want happening here is the config formats directly affecting the API. I know there's client authors here who want a simpler interface to make client implementation easier, but I don't want that affecting the project as a whole (either my ability to make the backend work as needed, or the fact that client APIs should cater to their users and don't need to be direct 1:1 mappings of the low level protocol), so I'm trying to balance needs as I can.

All that said: I still have no fucking clue what to do about the goddamn solace. :|