This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• Sensor API patch with sample Android implementation (sensor_patch, application/octet-stream, 2013-02-11 07:46:32 +0000, 49815 bytes)
• Added missing SDLCALL (sensor_patch, text/plain, 2013-02-11 07:59:51 +0000, 49823 bytes)
• Update to the patch with changes proposed by Philipp Wiesemann (sensor_patch, text/plain, 2013-02-17 18:57:10 +0000, 49069 bytes)
• Update the attachment with backwards compatibility in mind (sensor_patch, text/plain, 2013-02-18 18:59:25 +0000, 51797 bytes)
• Updated Android Sensor Patch (sensor_patch, text/plain, 2013-02-23 09:07:13 +0000, 53301 bytes)
• Updated Android Sensor patch (sensor_patch, text/plain, 2013-03-22 12:58:27 +0000, 52377 bytes)

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

Comments on the original bug report:

On 2013-02-11 07:46:32 +0000, Joseba García Echebarria wrote:

Created attachment 1039
Sensor API patch with sample Android implementation

The attached patch adds a sensor API to the latest SDL HG code.
That way SDL-based programs can fetch data from the physical world surrounding the user's "computer".

It includes a sample Android implementation as well as a dummy one.
Some work has been done so that the dummy driver compiles on systems other than Android, but it's unfinished and untested.

Regards,
Joseba

PS: On a personal note, I won't be able to work on this until thursday

On 2013-02-11 07:59:51 +0000, Joseba García Echebarria wrote:

Created attachment 1040
Added missing SDLCALL

On 2013-02-16 02:53:29 +0000, Philipp Wiesemann wrote:

The file SDL_sensor.h from the patch contains multiple "wrong" comments about joysticks including that SDL_INIT_JOYSTICK would be needed for initialization (it should be SDL_INIT_SENSOR).

Also this patch removes the accelerometer as default (and currently only) "joystick" on Android. So this patch is not only an extension but it also changes functionality. Programs relying on the accelerometer being the default joystick need to be rewritten to use sensors. I do not use the accelerometer myself but I felt that this information was missing above (though it is on the forum/mailing).

On 2013-02-16 03:05:43 +0000, Philipp Wiesemann wrote:

The implementation of SDL_SensorEventState() from SDL_sensor.h seems missing.

On 2013-02-16 06:36:36 +0000, Philipp Wiesemann wrote:

The SDL_androidsensor.c from the patch has a SDL_SYS_SensorClose() where ASensorEventQueue_enableSensor() is used. I do not know the ASensor* API but maybe ASensorEventQueue_disableSensor() would be more correct.

The SDL_sensor.c from the patch uses SDL_SYS_GetInstanceIdOfDeviceIndex() but there is no new implementation for it. I would assume (not yet having compiled this) that the function from the joystick source would be accidentally used. If the mapping for joysticks is different from the mapping for sensor on one platform this will not work.

Minor style comment: The documentation of the new functions often tells that a function may return 0 or 1 but the function actually has the return type SDL_bool. The values will match most of the time but maybe mentioning the constants (SDL_FALSE, SDL_TRUE) would be more nice.

On 2013-02-17 18:57:10 +0000, Joseba García Echebarria wrote:

Created attachment 1047
Update to the patch with changes proposed by Philipp Wiesemann

On 2013-02-17 19:03:55 +0000, Joseba García Echebarria wrote:

I've uploaded a new version of the patch with changes for the issues you propose, thanks for having a look.

There's one thing I'm not sure of: I've removed the SensorNeedsPolling function from my code since I haven't hooked the sensor code to the event loop in any way. This makes the user have to manually call SDL_SensorUpdate() before reading the sensor data.
This if fine for me but maybe it's not for other people...

And thanks for pointing out that the accelerometer functionality is removed from the joystick API once this patch is applied: I forgot to mention that here, yes.

I believe that's all.

Regards,
Joseba

(In reply to comment # 4)

The SDL_androidsensor.c from the patch has a SDL_SYS_SensorClose() where
ASensorEventQueue_enableSensor() is used. I do not know the ASensor* API but
maybe ASensorEventQueue_disableSensor() would be more correct.

The SDL_sensor.c from the patch uses SDL_SYS_GetInstanceIdOfDeviceIndex() but
there is no new implementation for it. I would assume (not yet having compiled
this) that the function from the joystick source would be accidentally used. If
the mapping for joysticks is different from the mapping for sensor on one
platform this will not work.

Minor style comment: The documentation of the new functions often tells that a
function may return 0 or 1 but the function actually has the return type
SDL_bool. The values will match most of the time but maybe mentioning the
constants (SDL_FALSE, SDL_TRUE) would be more nice.

On 2013-02-17 23:27:29 +0000, Sam Lantinga wrote:

Removing the accelerometer joystick connection needs to be optional. There are games already shipping that rely on this functionality

On 2013-02-18 01:35:46 +0000, Joseba García Echebarria wrote:

I suppose a SDL_HINT should be adequate, right?

I'll try to implement it this evening.

Regards,
Joseba

(In reply to comment # 7)

Removing the accelerometer joystick connection needs to be optional. There are
games already shipping that rely on this functionality

On 2013-02-18 18:59:25 +0000, Joseba García Echebarria wrote:

Created attachment 1049
Update the attachment with backwards compatibility in mind

The attached patch implements my proposed sensor API but keeps the traditional behaviour of passing accelerometer data as a joystick for backwards compatibility.
The user can now, however, choose to disable this behaviour via a new SDL_HINT:
SDL_SetHint(SDL_HINT_ACCELASJOY, "0"); // "0" disables, "1" enables

This hint should have the same effect in iOS, too, although I haven't tested that code.

The proposed patch also takes care of the case where the device has no accelerometer whatsoever: no joystick will be emulated whereas until now SDL unconditionally reported there was one accelerometer present.

Just to add some visual feedback, I took two screenshots of my test app.

• Here's what it reports by default with the attached patch applied and no hint set:
  https://www.dropbox.com/s/c2xtaad8dz00xs4/2013-02-19%2003.30.51.png
  The number after JOY.DOWN is the reading when the phone has a defined yaw angle, but it's not the actual number given by SDL.
• And here's what it reports when I set do SDL_SetHint(SDL_HINT_ACCELASJOY, "0");
  https://www.dropbox.com/s/jrqmdr8vqy2vdec/2013-02-19%2003.31.27.png

Regards,
Joseba

On 2013-02-19 06:27:20 +0000, Sam Lantinga wrote:

This patch looks very well done. I'd probably want to tweak the function naming for consistency with the joystick and gamepad API.

What API level does this require on Android?

Thanks!

On 2013-02-19 07:21:23 +0000, Joseba García Echebarria wrote:

The code uses functions available in android-9 (2.3) and over, which covers around 90% of android phones out there, according to Google's public data. The number should be even higher when SDL2 stable is released. I believe it's a reasonable limitation.

And yeah, I'm not very good at naming things :)

Joseba

(In reply to comment # 10)

This patch looks very well done. I'd probably want to tweak the function
naming for consistency with the joystick and gamepad API.

What API level does this require on Android?

Thanks!

On 2013-02-19 08:33:10 +0000, Andreas Schiffler wrote:

Quick code review comments

sensor.h
127 extern DECLSPEC Uint8 SDLCALL SDL_SensorType(SDL_Sensor * sensor);
should return SDL_SensorFlags
140 * The state is a value ranging from -32768 to 32767.
Limit it to this range? The return value is a float.
152 * The axis indices start at index 0.
154 extern DECLSPEC float SDLCALL SDL_SensorGetResolution(SDL_Sensor * sensor);
is missing the axis variable as input.

Suggestion: Automatically normalize the return value to [-1.0,1.0] or [0.0,1.0]; the resolution would become a scale factor ("inverse resolution").

The SDL_PrivateAccelAsJoy())code needs to take into account that SDL can be build without joystick support (but with accel support).

Android implementation

SDL_SYS_SensorNameForDeviceIndex(int device_index)
should return NULL on error.

263 SDL_bool SDL_SYS_SensorAttached(SDL_Sensor *sensor)
needs implementation? add a TODO comment

270 SDL_SYS_SensorClose(SDL_Sensor * sensor)
all functions taking a pointer like this one should do a NULL check

On 2013-02-19 14:26:18 +0000, Philipp Wiesemann wrote:

The patch adds an unused implementation of SDL_SYS_SensorNeedsPolling() with SDL_syssensor.c.

The function Android_JNI_HasAccelerometer() added into SDL_android.cpp returns an undefined value if GetStaticMethodID() would returns a falsy value. This can be fixed by setting "has" to SDL_FALSE.

Minor style comment: The documentation of SDL_SYS_SensorAttached() in SDL_syssensor.h mentions 0 and 1 although the return value is SDL_bool. Maybe SDL_FALSE and SDL_TRUE could be used.

(In reply to comment # 12)

270 SDL_SYS_SensorClose(SDL_Sensor * sensor)
all functions taking a pointer like this one should do a NULL check

Maybe a precondition could be documented stating that all SDL_SYS_* functions never get NULL pointers. This way dall NULL checks would be in the main implementation (an SDL_SYS_* is only called through it) and do not need to be repeated for all platforms (e.g. SDL_SensorClose() checks and only calls SDL_SYS_SensorClose() if not NULL).
It seems parts of SDL's joystick source already do this so maybe it could be used here too.

On 2013-02-19 19:43:35 +0000, Joseba García Echebarria wrote:

Hi,

I'll reply point by point.

(In reply to comment # 12)

Quick code review comments

sensor.h
127 extern DECLSPEC Uint8 SDLCALL SDL_SensorType(SDL_Sensor * sensor);
should return SDL_SensorFlags

Sure, that's true. Noted.

140 * The state is a value ranging from -32768 to 32767.
Limit it to this range? The return value is a float.

The error in the comments is the product of copy-pasting from the joystick API...
The thing is that the value returned by SDL_SensorGetAxis returns a physical magnitude that makes sense by itself. In Android, the format of the data returned by the system is described in [1].
That's why I'm returning a float: for many uses the rounded magnitude doesn't provide enough precission.

152 * The axis indices start at index 0.
154 extern DECLSPEC float SDLCALL SDL_SensorGetResolution(SDL_Sensor *
sensor);
is missing the axis variable as input.

Copy-paste again, sorry (noted). The comment there now reads:
/**

• Get the current resolution of an axis on a sensor.
• The returned value is the resolution for that sensor
• (the minimum change in the measured magnitude discernible
• by the sensor).
  */
  Again, IMHO this should return a float.

Suggestion: Automatically normalize the return value to [-1.0,1.0] or
[0.0,1.0]; the resolution would become a scale factor ("inverse resolution").

I believe that's wrong. First of all, the magnitudes returned by the sensors make sense by themselves (accelerations in m²/s, pressures in hPa...) and differente sensor models will have different measurement ranges.
It's not like in a joystick where "1" might mean "full right". Here it'd mean "the maximum value this sensor can measure". But that's not of much help and the user would end up converting that data back to the kind of data the patch provides.

The SDL_PrivateAccelAsJoy())code needs to take into account that SDL can be
build without joystick support (but with accel support).

Noted.

Android implementation

SDL_SYS_SensorNameForDeviceIndex(int device_index)
should return NULL on error.

Noted.
I've also added an error through SDL_SetError before returning NULL so that debugging should be easier.

263 SDL_bool SDL_SYS_SensorAttached(SDL_Sensor *sensor)
needs implementation? add a TODO comment

Noted.

270 SDL_SYS_SensorClose(SDL_Sensor * sensor)
all functions taking a pointer like this one should do a NULL check

Noted.

I've already changed most things you comment. I'll upload the updated patch tomorrow.

Thanks for taking the time to have a look.

Cheers,
Joseba

[1] http://developer.android.com/reference/android/hardware/SensorEvent.html

On 2013-02-19 19:49:52 +0000, Joseba García Echebarria wrote:

Hi,

Thanks for your comments.

(In reply to comment # 13)

The patch adds an unused implementation of SDL_SYS_SensorNeedsPolling() with
SDL_syssensor.c.

The function Android_JNI_HasAccelerometer() added into SDL_android.cpp returns
an undefined value if GetStaticMethodID() would returns a falsy value. This can
be fixed by setting "has" to SDL_FALSE.

Minor style comment: The documentation of SDL_SYS_SensorAttached() in
SDL_syssensor.h mentions 0 and 1 although the return value is SDL_bool. Maybe
SDL_FALSE and SDL_TRUE could be used.

Noted.

(In reply to comment # 12)

270 SDL_SYS_SensorClose(SDL_Sensor * sensor)
all functions taking a pointer like this one should do a NULL check

Maybe a precondition could be documented stating that all SDL_SYS_* functions
never get NULL pointers. This way dall NULL checks would be in the main
implementation (an SDL_SYS_* is only called through it) and do not need to be
repeated for all platforms (e.g. SDL_SensorClose() checks and only calls
SDL_SYS_SensorClose() if not NULL).
It seems parts of SDL's joystick source already do this so maybe it could be
used here too.

I've had a look at the joystick API and that doesn't do NULL check either, yes. Maybe documenting it makes sense as it saves others from writing tedious code when porting the sensor API to new platforms.

As said, I'll try to have a look at the code to check for those 1/0 vs SDL_TRUE/SDL_FALSE and other comment mistakes and upload the new patch tomorrow.

Regards,
Joseba

On 2013-02-19 20:06:28 +0000, Rodrigo Cardoso wrote:

(In reply to comment # 14)

Suggestion: Automatically normalize the return value to [-1.0,1.0] or
[0.0,1.0]; the resolution would become a scale factor ("inverse resolution").

I believe that's wrong. First of all, the magnitudes returned by the sensors
make sense by themselves (accelerations in m²/s, pressures in hPa...) and
differente sensor models will have different measurement ranges.
It's not like in a joystick where "1" might mean "full right". Here it'd mean
"the maximum value this sensor can measure". But that's not of much help and
the user would end up converting that data back to the kind of data the patch
provides.

I also agree that the values should not be normalized, unless it have a defined range like a joystick, which is not the case.

Also, if the return value from another platform diverges from what SDL uses, then it should be converted, but not normalized IMHO. Of course, a convention should be done in this case.

On 2013-02-23 09:07:13 +0000, Joseba García Echebarria wrote:

Created attachment 1051
Updated Android Sensor Patch

This patch is basically the same as the previous one but does a few more things:

• State in SDL_sensor.c that all the SDL_SYS_Sensor* must be given non-NULL pointers.
• Make sure we don't call those functions with NULL data from SDL_sensor.c
• SDL_PrivateAccelAsJoy will now return SDL_FALSE in case SDL_SENSOR_DISABLED is defined, so that we can disable joystick-sensor emulation with a compile flags.
• Fixed a few function headers, correcting list of params, return values and others.
• Changed the return type of SDL_SensorType from Uint8 to SDL_SensorFlags.

I believe that's all.

On 2013-02-24 11:26:44 +0000, Philipp Wiesemann wrote:

(In reply to comment # 17)

• Make sure we don't call those functions with NULL data from SDL_sensor.c

The current version of the patch has now NULL checks in some functions of SDL_sensor.c although these functions also ask SDL_PrivatesensorValid() after the check (which checks for NULL again).
Therefore I think the first check is not needed (as in the older version of the patch) because otherwise the arguments would be checked twice.

On 2013-03-22 12:58:27 +0000, Joseba García Echebarria wrote:

Created attachment 1076
Updated Android Sensor patch

I've updated the patch with a few minor modifications to the comments and removing a few NULL checks that were later performed by SDL_PrivateSensorValid. The patch has been created from the latest available HG revision.

The only notable change is that I've fixed a bug in SDL_SYS_SensorClose where I was calling
ASensorEventQueue_disableSensor(eventqueue, sensor->hwdata->asensor);
right after
SDL_free(sensor->hwdata);
Which would've exploded at some point.

Joseba

On 2013-03-22 17:05:16 +0000, Philipp Wiesemann wrote:

(In reply to comment # 19)

Created attachment 1076 [details]
Updated Android Sensor patch

I've updated the patch with a few minor modifications to the comments and
removing a few NULL checks that were later performed by
SDL_PrivateSensorValid. The patch has been created from the latest available
HG revision.

The only notable change is that I've fixed a bug in SDL_SYS_SensorClose
where I was calling
ASensorEventQueue_disableSensor(eventqueue, sensor->hwdata->asensor);
right after
SDL_free(sensor->hwdata);
Which would've exploded at some point.

Joseba

The patch seems to be a diff against an older revision and does not care about the changes from bug 1700. Therefore it may not be imported easily.

Three more comments about SDL_androidsensor.c:

I think the range checks with device_index and the return value of ASensorManager_getSensorList() have an off-by-one error.

The error message in SDL_SYS_SensorNameForDeviceIndex() has to many spaces.

There are some // line comments which may not work on outdated C compilers.

On 2013-07-16 04:49:10 +0000, wrote:

Hey,

First, I should apologize in case I have resulted in any inconvenience that comes from my changes to the patch from bug 1700. The patch has been reverted since then, so that should not be an issue.

Secondly, after encountering a bug regarding a similar thing, I have the following suggestion. Basically, it seems like with the last patch, the sensors are still opened while the application is halted, which is the case if SDL_ANDROID_BLOCK_ON_PAUSE is defined (currently the default). I believe that in order to improve a device's battery life, these should temporarily be closed (but only if SDL_ANDROID_BLOCK_ON_PAUSE is defined). Due to the multi-threaded nature of the Android port, this results in the following suggestions:

• If SDL_ANDROID_BLOCK_ON_PAUSE is defined, maintain a list of all opened sensors. (Looks like there is already such a thing right now, independently of SDL_ANDROID_BLOCK_ON_PAUSE.)
• When Android_PumpEvents is called by the C/C++ thread (in SDL_androidevents.c), temporarily close all sensor devices (again if SDL_ANDROID_BLOCK_ON_PAUSE is defined) before a pause begins. These should be remembered thanks to a list as given before.
• Right after such a pause, re-open all sensor devices.
• (Optionally) Add to SDL_sensor.h a comment telling it's recommended to open a sensor only if one is required, and close it when it is not. That should apply on Android, at least. This tip can be useful in case SDL_ANDROID_BLOCK_ON_PAUSE is not defined.
• Finally, we should not forget about the current behaviors of accessing a default accelerometer using the Joystick API. At the moment the default accelerometer device is opened even if it is never used. I guess that similar improvements can be applied to it, only that the Joystick API is involved this time. One difference is that joystick events should still arrive, if desired.

Thanks for the suggested sensor patch!

On 2013-07-16 04:56:29 +0000, wrote:

Oh yeah, assuming what I have described is applied at some point, it probably worths to mention that the Sensor API should only be accessed from the thread that (possibly indirectly) calls SDL_PumpEvents, i.e. the thread that creates the window.

On 2018-03-26 11:05:50 +0000, Stéphane Magnenat wrote:

What is the status of this patch? Sensors are an important part of the hardware of mobile devices nowadays (and also appears in laptops) and it would be very helpful for SDL to abstract them.

On 2018-08-20 22:14:51 +0000, Sam Lantinga wrote:

I'm working on this now, thanks!

On 2018-08-21 11:21:10 +0000, Joseba García Echebarria wrote:

(In reply to Sam Lantinga from comment # 24)

I'm working on this now, thanks!

Cool!

SDL has changed a bit since I wrote te initial patch, but let me know if I can be of any help. I have Android 8.1 & iOS 11 devices available for testing.

Kind regards,
Joseba

On 2018-08-23 17:58:35 +0000, Sam Lantinga wrote:

This is implemented!

Because of the large number of sensors, I've only created definitions for ones that are implemented on both Android and iOS (accelerometer and gyroscope). These have well defined semantics, and I've defined them in SDL_sensor.h. You can access additional sensors on Android using platform specific semantics without additional library changes.

On 2018-10-02 11:18:36 +0000, Sylvain wrote:

Just a question about sensors on Android, implemented in https://hg.libsdl.org/SDL/rev/24142c5073a4

On the java side, we have always called:

mSurface.enableSensor(Sensor.TYPE_ACCELEROMETER, true);

And there is this comment FIXME: "Why aren't we enabling sensor input at start?"

Wouldn't it make sensor to do this only if we have initialized the subsystem by doing SDL_Init(SDL_INIT_SENSOR).

e.g. to add in "SDL_ANDROID_SensorInit()" (SDL_androidsensor.c) a JNI call to allow enable/disable sensors on java side during create()/pause()/resume()/destroy()