buttplug icon indicating copy to clipboard operation
buttplug copied to clipboard

RFP: Clarification on Generic Subcommand Index Handling

Open qdot opened this issue 7 years ago • 6 comments

We don't currently have any rules about how we handle repeated subindexes in Generic Commands.

For instance, with VibrateCmd, say someone creates the following packet:

{
"VibrateCmd":
{
"Speeds": [
{ "Index": 0, "Speed": 0.5 },
{ "Index": 0, "Speed": 0.75 }
]
},
"Id": 10,
"DeviceIndex": 0
}

Our implementations in both C# and JS would currently send 2 commands to the device back to back, which is silly. We should specify whether either the last value for each index should be chosen, or whether we should error out on this case.

I don't have strong feelings either direction. Erroring out would require actual library logic though, I don't believe we'd easily be able to catch that via the schema.

qdot avatar Jun 25 '18 04:06 qdot

It should probably be disallowed, implemented at the message handling layer within the drivers (since that's were we disallow too many and out of bounds subcommands).

Given that this logic is repeated so often, maybe a generic utility function should be written to handle these rules?

blackspherefollower avatar Jun 25 '18 06:06 blackspherefollower

Mostly looking for spec clarification here, library specifics can be handled in the relevant repos.

My comment about the schema was mostly about the ease of the possible fix for this, because until we get a reference set of messages that we can use to test libraries (buttplugio/buttplug-schema#2), it's hard to make sure everything works and is at parity.

qdot avatar Jun 25 '18 06:06 qdot

Ah, got it. Then in my opinion, we should document that only one subcommand per feature should be sent.

blackspherefollower avatar Jun 25 '18 07:06 blackspherefollower

I can't even imagine a scenario where someone would want to send a message like you're opening example. Just what could it be intended to accomplish? I think it should be an error too, since most times it would probably be a silent bug otherwise.

hiss-remi avatar Jun 27 '18 04:06 hiss-remi

Yup, only reason I was RFP'ing this in the first place is that we already shipped the message in January and I wanted to make sure I wasn't missing anything. I think we can refine the spec without rolling the version on this, since the logic will be required in the server instead of the schema anyways.

I'll update the spec tomorrow and land then, will close this with the commit message for it.

qdot avatar Jun 27 '18 06:06 qdot

This should be checked as part of is_valid() for all generic device messages. Filing for v6.1

qdot avatar Aug 29 '22 00:08 qdot