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

[Patch] Add accelerometer and gyroscope APIs #3411

Closed
SDLBugzilla opened this issue Feb 11, 2021 · 13 comments
Closed

[Patch] Add accelerometer and gyroscope APIs #3411

SDLBugzilla opened this issue Feb 11, 2021 · 13 comments
Assignees
Labels
enhancement
Milestone

Comments

@SDLBugzilla
Copy link
Collaborator

@SDLBugzilla SDLBugzilla commented Feb 11, 2021

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: HG 2.0
Reported for operating system, platform: All, All

Comments on the original bug report:

On 2019-10-04 23:49:20 +0000, Jonathan Newman wrote:

Created attachment 3977
Add accelerometer and gyroscope APIs

Adds SDL_JoystickGetAccelerometer and SDL_JoystickGetGyroscope, with appropriate dummy implementations for all drivers.

I intend to implement these for HIDAPI_DriverPS4 soon.

On 2019-10-05 00:15:13 +0000, Jonathan Newman wrote:

Created attachment 3978
Add accelerometer and gyroscope APIs (with correct dynapi entries)

Guess who forgot to run gendynapi.pl? Added fixed patch.

On 2019-10-06 12:16:33 +0000, Alex Szpakowski wrote:

Could this be integrated with the SDL_Sensor API?

On 2019-10-06 12:43:07 +0000, Jonathan Newman wrote:

(In reply to Alex Szpakowski from comment # 2)

Could this be integrated with the SDL_Sensor API?

SDL_Sensor seems to be focused on built-in always-available sensors intended for phones, whereas my mental model of the accel+gyro in gamepads is as an additional high-precision analogue input. For that reason I didn't really consider using it. There's also the matter of Sensors having a separate Update function, whearas generally motion data is included in the normal updates from game controllers.

If you have a strong preference then I can rework things to use SDL_Sensor, but I would prefer that sensors remain strongly associated with their corresponding Joysticks... as opposed to the user needing to separately open a Joystick and a Sensor, and maintain a mapping between them. It could result in a fairly cumbersome bridge between the _joystick and _sensor systems.

I could instead just revise the APIs to be more consistent with Sensor, though. e.g. represent gyroscope data in radians per second rather than degrees per second, and accept a single float* param instead of separate x/y/z.

Let me know how you'd like me to proceed.

On 2019-10-15 01:59:09 +0000, Alex Szpakowski wrote:

I think the idea with the Sensor APIs was to eventually extend them to work with Joysticks, similarly to how the haptic API can work with joysticks ( https://twitter.com/icculus/status/1032105900981530624 ).

I agree that it would be ideal to obtain a joystick's sensor data without having to iterate through a separate list of sensors, I think the haptic API provides a similar concept for itself as well, so maybe you could look at how that works as a base.

On 2020-03-11 18:02:15 +0000, Mathieu Eyraud wrote:

Created attachment 4247
PS4 hidapi implementation

I don't know if you are still working on this but I implemented motion control for dualshock4 hidapi driver.

  • API:

Two functions to check for gyro and accelerometer:
int SDL_JoystickHasAccelerometer(SDL_Joystick * joystick)
int SDL_JoystickHasGyroscope(SDL_Joystick * joystick)

Two functions to get the data:
int SDL_JoystickGetGyroscope(SDL_Joystick * joystick, float *gyro_xyz);
int SDL_JoystickGetAccelerometer(SDL_Joystick * joystick, float *accel_xyz);
They take a pointer to an array of 3 floats unlike the original patch proposed.

  • Event

Added a new event type SDL_JOYSENSORUPDATE that uses the same struct as SDL_SENSORUPDATE.

  • Joystick system

Added fields to store gyro and acceleration data to _SDL_Joystick.
The driver functions added by the original patch were not needed, instead I added SDL_PrivateJoystickSensors which work like others SDL_PrivateJoystick* functions.

  • HIDAPI PS4

I looked at the dualshock4 Linux driver to get the meaning of data from calibration report.
https://elixir.bootlin.com/linux/v5.5.1/source/drivers/hid/hid-sony.c#L1631

Getting the data from gyro and accelerometer was straightforward as they already are in the PS4StatePacket_t struct.

Some third-party joysticks compatible with the PS4 don't have a motion sensors. I don't know how to check the presence of sensors.

Data from report is an array of Uint8, it cannot be casted to PS4StatePacket_t as it could cause misaligned read of the sensor data (Sint16). I use memcpy instead.

  • Misc.

Added SDL_SENSORUPDATE and SDL_JOYSENSORUPDATE to SDL_LogEvent. Logging of these events are disabled by default as they spam a lot.
Added SDL_JOYSENSORUPDATE to testjoystick.c.
Rumble over Bluetooth only cause problem on Windows, so only disable it on Windows.

On 2020-04-30 19:33:26 +0000, Jonathan Newman wrote:

(In reply to meyraud705 from comment # 5)

Created attachment 4247 [details]
PS4 hidapi implementation

I don't know if you are still working on this but I implemented motion
control for dualshock4 hidapi driver.

Hey meyraud705, thank you for picking this up! I couldn't find time to get back to it.

Your API is definitely better than my attempt- Fingers crossed this can be merged :)

On 2020-05-04 17:13:34 +0000, Mathieu Eyraud wrote:

Created attachment 4329
PS4 hidapi implementation update1

Updated patch so it applies cleanly on last revision.

On 2020-07-12 15:26:49 +0000, Jonathan Newman wrote:

(In reply to meyraud705 from comment # 5)

Rumble over Bluetooth only cause problem on Windows, so only disable it on
Windows.
If I remember this correctly: In generic Bluetooth mode (not the official dongle), motion/touchpad/etc data isn't sent until rumble has been enabled. But enabling rumble also causes the pad to stop working with DirectInput applications until the pad is turned off and on.

In my opinion, SDL should just put the pad into this full functionality mode without worrying about DirectInput. It's trivial to reverse by just restarting the controller, and I doubt there are many people who use the DS4 with DI anyway (it's mostly used with third-party wrappers like Steam Input/JoyShockMapper/DS4Windows, all of which enable rumble and break DI).

On 2020-11-12 12:46:03 +0000, Mathieu Eyraud wrote:

Created attachment 4514
PS4 implementation update 2

I made some improvements:

  • SDL_JoystickGetSensorRate()

When you integrate the angular velocity provided by the gyro you need the time between each sample.

Using the timestamp of the event does not work (timestamp represents time when event is pushed in the queue).
If you poll event 60 time per second and the controller report event at 240, then you get 4 events with the same timestamp.

Dualshock4 and Switch controller send data at a fixed rate so expose this to the user with SDL_JoystickGetSensorRate().

  • SDL_HINT_JOYSTICK_ALLOW_BACKGROUND_SENSOR_EVENTS

Joystick does not send events when app is in the background unless SDL_HINT_JOYSTICK_ALLOW_BACKGROUND_EVENTS is set to 1.

This behavior is undesirable for sensor as you lose track of the orientation and most algorithm will need time to recover after app come back to foreground.

SDL_HINT_JOYSTICK_ALLOW_BACKGROUND_SENSOR_EVENTS is added and is set to 1 by default. It controls whether or not sensor events are sent in the background and works independently from SDL_HINT_JOYSTICK_ALLOW_BACKGROUND_EVENTS.

  • Switch and Steam controller

Added implementation for Switch controller. It does not read calibration data, so accuracy will be low. I do not own a switch controller so this is not tested.
Added stub implementation for Steam controller and make the file compile in c89 mode.

  • Trackpad

The trackpad on the Dualshock4 sends SDL_FINGER_DOWN/SDL_FINGERUP/SDL_FINGERMOTION event.
Added ref count to touch subsystem to be sure that it is not shutdown while joystick is still using it.

  • Development note

This sensor implementation for joystick does not use the sensor API. Sensor data are stored in joystick struct and events are sent by joystick function. It only reuse the SDL_SensorEvent struct, rest of it is duplicate code.
From an user point of view, sensor datas are obtained from two event types and two set of functions, which add unneeded complexity.

The way the touch API works make it easy use it from anywhere and to extend the capability of a device. This make the implementation for the Dualshock 4 trackpad short and simple: create and delete a SDL_Touch, 4 lines of code to read data from controller and 2 functions to send data to SDL_Touch object.
From an user point of view, if touch is already working in their app then no additional work is required.

If you plan to write a new subsystem or rewrite an existing one, I think it would be better to make it works like the touch API.

On 2020-11-24 05:36:22 +0000, Sam Lantinga wrote:

Hey guys, I just saw this bug after I went ahead and added touchpad and accelerometer support. I looked through what you have here, and it looks pretty good. Is there anything that you'd like to see incorporated into the new API?

On 2020-11-24 09:30:00 +0000, Mathieu Eyraud wrote:

As I said in my previous comment, when you integrate angular velocity you need the time between each sample. Timestamp of the event is not accurate enough. In my patch I added SDL_JoystickGetSensorRate(), but having an accurate timestamp will work too.

I haven't used touchpad yet, but adding gesture support could be interesting.

@SDLBugzilla SDLBugzilla added enhancement waiting labels Feb 11, 2021
@Yamakaky
Copy link

@Yamakaky Yamakaky commented Jul 28, 2021

(can I comment here?)

@meyraud705 additional information if you don't know it: on Switch pro controller and joycons, the report rate is 66Hz, but every report contains 3 IMU reports, for a final 200Hz report rate for gyro and accelerometer. In it's current form, GetSensorData returns all three measures concatenated from newest to oldest, as 9 floats, but you need to know about this behaviour to exploit all values instead of just one like with PS4 controller. Native support in SDL should handle this case.

@slouken
Copy link
Collaborator

@slouken slouken commented Jul 28, 2021

@Yamakaky They're currently averaged for the single report. Does it make more sense to split them into 3 separate sensor events?

@slouken slouken self-assigned this Jul 28, 2021
@slouken slouken removed the waiting label Jul 28, 2021
@meyraud705
Copy link
Contributor

@meyraud705 meyraud705 commented Jul 29, 2021

Sending 3 sensor events lets the user integrate the velocity on every sample. It should give more accurate orientation than integrating the average because rotation are not linear.

I would also like to repeat that having an accurate timestamp or the sensor report rate is needed to integrate accurately:

When you integrate the angular velocity provided by the gyro you need the time between each sample.

Using the timestamp of the event does not work (timestamp represents time when event is pushed in the queue).
If you poll event 60 time per second and the controller report event at 240, then you get 4 events with the same timestamp.

Dualshock4 and Switch controller send data at a fixed rate so expose this to the user with SDL_JoystickGetSensorRate().

@slouken slouken added this to the 2.0.16 milestone Jul 29, 2021
@slouken
Copy link
Collaborator

@slouken slouken commented Jul 29, 2021

How does a186a50 look?

@meyraud705
Copy link
Contributor

@meyraud705 meyraud705 commented Jul 29, 2021

@slouken I spotted some mistakes:

  • In SendSensorUpdate you are using HIDAPI_DriverSwitch_ScaleGyro for both gyro and accelerometer.

  • Order of rotation matter:

    SendSensorUpdate(joystick, ctx, SDL_SENSOR_GYRO, &packet->imuState[2].sGyroX);
    SendSensorUpdate(joystick, ctx, SDL_SENSOR_GYRO, &packet->imuState[1].sGyroX);
    SendSensorUpdate(joystick, ctx, SDL_SENSOR_GYRO, &packet->imuState[0].sGyroX);

I don't know the correct order, I would have guessed the opposite. But if you know this is the correct order then it's OK.

  • In testgamecontroller.c, you are querying the rate of accelerometer for both gyro and accelerometer.

I do not have Joycon to test, but everything else looks good.
Tested SDL_GameControllerGetSensorDataRate with DS4 and it works as expected.

Now, we only need support for the Steam controller ;)

@Yamakaky
Copy link

@Yamakaky Yamakaky commented Jul 29, 2021

I don't think the axes are accurate since they vary per device : https://github.com/dekuNukem/Nintendo_Switch_Reverse_Engineering/blob/master/imu_sensor_notes.md. Iirc the pro controller use the same axis as the left joycon.
As mentioned, having all three reports would be nice to have the highest precision possible.

@meyraud705
Copy link
Contributor

@meyraud705 meyraud705 commented Jul 29, 2021

@Yamakaky
Copy link

@Yamakaky Yamakaky commented Jul 29, 2021

OK, nice!

@slouken
Copy link
Collaborator

@slouken slouken commented Jul 29, 2021

@slouken I spotted some mistakes:

  • In SendSensorUpdate you are using HIDAPI_DriverSwitch_ScaleGyro for both gyro and accelerometer.

Nice catch, I fixed that in b3a0174

  • Order of rotation matter:
    SendSensorUpdate(joystick, ctx, SDL_SENSOR_GYRO, &packet->imuState[2].sGyroX);
    SendSensorUpdate(joystick, ctx, SDL_SENSOR_GYRO, &packet->imuState[1].sGyroX);
    SendSensorUpdate(joystick, ctx, SDL_SENSOR_GYRO, &packet->imuState[0].sGyroX);

I don't know the correct order, I would have guessed the opposite. But if you know this is the correct order then it's OK.

Yeah, the old code used the axes in Y,Z,X order for both gyro and accelerometer and I checked that orientation when I originally wrote it. @Yamakaky mentioned that the data in the report is newest to oldest, and I'm not really sure how to test that, but the new code should deliver the axes as before, in oldest to newest order.

However, please feel free to double check my work. :)

  • In testgamecontroller.c, you are querying the rate of accelerometer for both gyro and accelerometer.

Nice catch, I fixed that in 65ff00e

I do not have Joycon to test, but everything else looks good.
Tested SDL_GameControllerGetSensorDataRate with DS4 and it works as expected.

Are we actually getting 250 reports per second from the DS4? I know the USB polling rate should be set to 4 ms, but I haven't confirmed that.
Also, does this change when connected over Bluetooth?

@Yamakaky
Copy link

@Yamakaky Yamakaky commented Jul 30, 2021

@Yamakaky mentioned that the data in the report is newest to oldest, and I'm not really sure how to test that,

In my own library at github.com/Yamakaky/joy, I just printed the samples in both orders, and newest first was smoother^^ I can retest it just to be sure.

I also think it is 250Hz,i can test that too.

@meyraud705
Copy link
Contributor

@meyraud705 meyraud705 commented Jul 30, 2021

@Yamakaky mentioned that the data in the report is newest to oldest, and I'm not really sure how to test that, but the new code should deliver the axes as before, in oldest to newest order.

To test this you could log all values from gyro and do an accelerating movement. Because you are accelerating, the largest value from a packet is the newest. I don't have Joycon, so if someone else could test that.

diff --git a/src/joystick/hidapi/SDL_hidapi_switch.c b/src/joystick/hidapi/SDL_hidapi_switch.c
index aef17c8de..71f956c4c 100644
--- a/src/joystick/hidapi/SDL_hidapi_switch.c
+++ b/src/joystick/hidapi/SDL_hidapi_switch.c
@@ -44,6 +44,9 @@
 /* Define this to get log output for rumble logic */
 /*#define DEBUG_RUMBLE*/
 
+/* Define this to get log output for sensor values */
+#define DEBUG_SWITCH_SENSORS
+
 /* The initialization sequence doesn't appear to work correctly on Windows unless
    the reads and writes are on the same thread.
 
@@ -1446,6 +1449,12 @@ static void HandleFullControllerState(SDL_Joystick *joystick, SDL_DriverSwitch_C
         SendSensorUpdate(joystick, ctx, SDL_SENSOR_GYRO, &packet->imuState[2].sGyroX);
         SendSensorUpdate(joystick, ctx, SDL_SENSOR_GYRO, &packet->imuState[1].sGyroX);
         SendSensorUpdate(joystick, ctx, SDL_SENSOR_GYRO, &packet->imuState[0].sGyroX);
+#ifdef DEBUG_SWITCH_SENSORS
+        SDL_Log("%f %f %f | %f %f %f | %f %f %f\n",
+                HIDAPI_DriverSwitch_ScaleGyro(packet->imuState[2].sGyroX), HIDAPI_DriverSwitch_ScaleGyro(packet->imuState[2].sGyroY), HIDAPI_DriverSwitch_ScaleGyro(packet->imuState[2].sGyroZ),
+                HIDAPI_DriverSwitch_ScaleGyro(packet->imuState[1].sGyroX), HIDAPI_DriverSwitch_ScaleGyro(packet->imuState[1].sGyroY), HIDAPI_DriverSwitch_ScaleGyro(packet->imuState[1].sGyroZ),
+                HIDAPI_DriverSwitch_ScaleGyro(packet->imuState[0].sGyroX), HIDAPI_DriverSwitch_ScaleGyro(packet->imuState[0].sGyroY), HIDAPI_DriverSwitch_ScaleGyro(packet->imuState[0].sGyroZ));
+#endif
 
         SendSensorUpdate(joystick, ctx, SDL_SENSOR_ACCEL, &packet->imuState[2].sAccelX);
         SendSensorUpdate(joystick, ctx, SDL_SENSOR_ACCEL, &packet->imuState[1].sAccelX);

Are we actually getting 250 reports per second from the DS4? I know the USB polling rate should be set to 4 ms, but I haven't confirmed that.
Also, does this change when connected over Bluetooth?

Yes, 250 reports per second with USB and Bluetooth, tested on Windows and Linux.

@slouken
Copy link
Collaborator

@slouken slouken commented Jul 30, 2021

testgamecontroller prints out sensor values, and the current implementation looks correct.

This is a smooth curve as I move the controller:
INFO: Controller 0 sensor accelerometer: -0.22, 9.58, 1.80
INFO: Controller 0 sensor accelerometer: -0.22, 9.57, 1.79
INFO: Controller 0 sensor accelerometer: -0.23, 9.59, 1.78
INFO: Controller 0 sensor accelerometer: -0.25, 9.62, 1.76
INFO: Controller 0 sensor accelerometer: -0.26, 9.65, 1.76
INFO: Controller 0 sensor accelerometer: -0.26, 9.67, 1.77
INFO: Controller 0 sensor accelerometer: -0.27, 9.70, 1.78
INFO: Controller 0 sensor accelerometer: -0.26, 9.72, 1.78
INFO: Controller 0 sensor accelerometer: -0.27, 9.73, 1.79
INFO: Controller 0 sensor accelerometer: -0.28, 9.74, 1.80
INFO: Controller 0 sensor accelerometer: -0.28, 9.75, 1.81
INFO: Controller 0 sensor accelerometer: -0.28, 9.75, 1.82
INFO: Controller 0 sensor accelerometer: -0.27, 9.75, 1.83
INFO: Controller 0 sensor accelerometer: -0.26, 9.75, 1.83
INFO: Controller 0 sensor accelerometer: -0.26, 9.74, 1.84
INFO: Controller 0 sensor accelerometer: -0.24, 9.68, 1.84
INFO: Controller 0 sensor accelerometer: -0.25, 9.67, 1.84
INFO: Controller 0 sensor accelerometer: -0.24, 9.65, 1.84
INFO: Controller 0 sensor accelerometer: -0.23, 9.62, 1.85
INFO: Controller 0 sensor accelerometer: -0.24, 9.61, 1.86
INFO: Controller 0 sensor accelerometer: -0.24, 9.58, 1.88

@slouken
Copy link
Collaborator

@slouken slouken commented Jul 30, 2021

Thanks everyone, I think we're good!

@slouken slouken closed this as completed Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement
Projects
None yet
Development

No branches or pull requests

4 participants