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

Do we really need floats in the protocol? #674

Open
qdot opened this issue Nov 21, 2024 · 8 comments
Open

Do we really need floats in the protocol? #674

qdot opened this issue Nov 21, 2024 · 8 comments
Assignees
Labels
Breaking Change Change that will break surface APIs, needs major revision to update Features New things to add to the project

Comments

@qdot
Copy link
Member

qdot commented Nov 21, 2024

Continuing my "asking big questions while working toward finishing v4" thing...

I'm starting to question the 0.0-1.0 range at the protocol level. I think it's fine for the API level, but the client can easily handle the conversion from 0.0-1.0 at the API level to steps that it sends back to the server, based on the feature information. It also reduces what we have to send over the line, simplifies the backend conversions, etc...

The reason I'm thinking about this is that I'm working on the idea of collapsing RotateCmd and ScalarCmd together (So RotateCmd will just be a RotateWithDirection actuator, and a -y <= x <= y range or whatever, doesn't even need to be symmetric) into LevelCmd, and making LevelCmd handle full integer ranges, not just positive.

I think I originally designed with 0.0-1.0 so that developers wouldn't have to think about the range math, but... they do anyways. We already ship step amounts and had to start doing so in v1, step ranges wouldn't be that much different, and it gives the client API developer more leeway to rework APIs for possibly better devex.

Also de/serializing doubles just sucks

So, to summarize: Should we just take an integer in LevelCmd/LinearCmd (for position), expect it to be within a range we specify in feature descriptions in DeviceAdded/DeviceList, and bounds check that on the server?

(If someone else already had this idea and I rejected it for some reason, feel free to remind me why and whether or not my reasoning was actually any good. I, as always, have zero short term memory.)

cc @blackspherefollower @Yoooi0

@qdot qdot added the Features New things to add to the project label Nov 21, 2024
@qdot qdot self-assigned this Nov 21, 2024
@qdot qdot added the Breaking Change Change that will break surface APIs, needs major revision to update label Nov 21, 2024
@blackspherefollower
Copy link
Collaborator

I think you had this idea during V3 development, which is why the step count is exposed to the client.

This sounds good, but it does raise a couple of questions:

  • How do range limits affect this? Especially where the min step is raised.
  • Does 0 mean stop for rotation or is it the halfway point between the negative and positive step values?

@Yoooi0
Copy link
Contributor

Yoooi0 commented Nov 21, 2024

As I don't work with the apis that the devices use, will it be possible to translate all device ranges to integers? What would be the integer range for an actuator/device that actually accepts floats in their api?
For example there is a device that accepts values between 0.0-2.0 with 4 steps (0, 0.5, 1, 1.5, 2), or a device that accepts precise 0.0-10.0 float without steps.

Sending 0.0-1.0 float handles all cases because mapping is done on the device protocol implementation side.
If the mapping were to move to the library/client (and assuming there are devices that expect floats) I think you need to send step range (as float) and step count, and LevelCmd would still have to use floats.

I think step range and step count are useful for creating UI for users, but from client perspective sending 0-1 seems easier.
Also for RotateWithDirection you could just send -1.0-1.0 and the asymmetric range mapping would be done server side, it also simplifies the stop value as it will be always 0.

@blackspherefollower
Copy link
Collaborator

@Yoooi0 No device to date has taken a float control value: the server already scales the 0.0-1.0 float to the appropriate uint level that the device takes (in V3 that step count is already surfaced to the client).

The suggestion here is that the client library would do the scaling rather than the server, given the step count included with the device info.

The step count for asymmetric RotateCmd is a weird one, that likely needs a specific positive and negative step count.

@qdot
Copy link
Member Author

qdot commented Nov 22, 2024

Yeah, that's the main issue, nothing takes floats at the protocol and I can't see a feasible situation where that's going to happen that we can't also handle just using i32's.

Meanwhile, de/serializing f64's is a mess, slow, way too much extra data for what we need, etc.

Instead of step counts, DeviceAdded/List will just send step range in the form of [x, y] over, where I32_MIN <= x <= 0, and 0 <= y <= I32_MAX. 0 will always be stop, even in asymmetric instances.

@blackspherefollower
Copy link
Collaborator

So to be clear, for RotationCmd devices in LevelCmd, you'll have a step range like [-20,20] and for basically everything else you'll have [0,20]?

@qdot
Copy link
Member Author

qdot commented Nov 22, 2024

Until such point that we end up needing the range for something else, yeah.

@Yoooi0
Copy link
Contributor

Yoooi0 commented Nov 22, 2024

If there are no protocols that use floats then yea it seems like a reasonable change.
And for example for TCode the range would be [0, 9999] right?

There might be a small issue when libraries/clients convert from floats to ints where due to different rounding methods used there will be different behavior (round(1.5) is 1 or 2). Might be a good idea to specify rounding method that is used in buttplug in developer docs.

@blackspherefollower
Copy link
Collaborator

@Yoooi0 the rounding method should be ceiling(float * step count). Early implementations of various protocols did different things, but the rounding was standardised when the codebase moved to rust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Change that will break surface APIs, needs major revision to update Features New things to add to the project
Projects
None yet
Development

No branches or pull requests

3 participants