Re: NULL pointer dereference when writing fuzzed data to /dev/uhid

[Roderick Colenbrander]

Thanks, it seems the tests created a Buzz controller. It is
sony_led_init (called from sony_input_configured), which calls
hid_validate_values. It is hid_validate_values, which is unhappy due
to obviously corrupted reports.

I'm not too familiar with hid_validate_values, but it seems to access
a bunch of data structures on the HID device. The code probably makes
some assumptions. Fixing this issue requires some more sanity
checking, if it is worth it.

Thanks,
Roderick

On Fri, Jan 4, 2019 at 9:04 AM Anatoly Trosinenko
 wrote:
>
> > Would you be able to share the sony.bin file?
> Sent it in this message.
>
> > Did you inject a particular device?
> If you are asking me, then no, I blindly send fuzzed data with a
> simple (but quite large and not very meaningful) header. That time it
> just turned out to be Sony-like descriptor :)
>
> Best regards
> Anatoly
>
> пт, 4 янв. 2019 г. в 19:38, Roderick Colenbrander :
> >
> > > > For sony.bin:
> > > >
> > > > root@kvm-xfstests:~# cat /vtmp/sony.bin > /dev/uhid
> > > > [   16.891931] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.892432] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.892894] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.893362] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.893844] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.895389] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.898165] sony 0003:054C:1000.0001: ignoring exceeding usage max
> > > > [   16.901190] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.903797] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.906401] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.908957] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.911449] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.913936] sony 0003:054C:1000.0001: unknown main item tag 0x1
> > > > [   16.916551] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.918454] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.919743] sony 0003:054C:1000.0001: unknown main item tag 0x4
> > > > [   16.920834] sony 0003:054C:1000.0001: unknown main item tag 0xe
> > > > [   16.921904] sony 0003:054C:1000.0001: unknown main item tag 0xe
> > > > [   16.923006] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.924082] sony 0003:054C:1000.0001: unknown main item tag 0x2
> > > > [   16.925195] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.926289] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.927400] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > > > [   16.928546] BUG: unable to handle kernel NULL pointer dereference
> > > > at 0028
> > > > [   16.929951] #PF error: [normal kernel read fault]
> > > > [   16.930884] PGD 80007a52b067 P4D 80007a52b067 PUD 0
> > > > [   16.931836] Oops:  [#1] SMP PTI
> > > > [   16.932437] CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted
> > > > 4.20.0-xfstests-10979-g96d4f267e40 #1
> > > > [   16.933752] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > > > BIOS 1.11.1-1ubuntu1 04/01/2014
> > > > [   16.935372] Workqueue: events uhid_device_add_worker
> > > > [   16.936321] RIP: 0010:hid_validate_values+0x48/0x110
> > >
> > > In a sense, it's good to have a fault there because this was added to
> > > make sure we do not blindly accept any data. The fact that it doesn't
> > > fail gracefully is a sign that there is something else.
> > > Maybe Roderick could have a look?
> > >
> > > Cheers,
> > > Benjamin
> > >
> >
> > Sure I can have a look. Would you be able to share the sony.bin file?
> > Did you inject a particular device? We do a lot of remapping and
> > processing in hid-sony at startup. It is probably related to that.
> >
> > Thanks,
> > Roderick

Re: NULL pointer dereference when writing fuzzed data to /dev/uhid

[Roderick Colenbrander]

> > For sony.bin:
> >
> > root@kvm-xfstests:~# cat /vtmp/sony.bin > /dev/uhid
> > [   16.891931] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.892432] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.892894] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.893362] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.893844] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.895389] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.898165] sony 0003:054C:1000.0001: ignoring exceeding usage max
> > [   16.901190] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.903797] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.906401] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.908957] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.911449] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.913936] sony 0003:054C:1000.0001: unknown main item tag 0x1
> > [   16.916551] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.918454] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.919743] sony 0003:054C:1000.0001: unknown main item tag 0x4
> > [   16.920834] sony 0003:054C:1000.0001: unknown main item tag 0xe
> > [   16.921904] sony 0003:054C:1000.0001: unknown main item tag 0xe
> > [   16.923006] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.924082] sony 0003:054C:1000.0001: unknown main item tag 0x2
> > [   16.925195] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.926289] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.927400] sony 0003:054C:1000.0001: unknown main item tag 0x0
> > [   16.928546] BUG: unable to handle kernel NULL pointer dereference
> > at 0028
> > [   16.929951] #PF error: [normal kernel read fault]
> > [   16.930884] PGD 80007a52b067 P4D 80007a52b067 PUD 0
> > [   16.931836] Oops:  [#1] SMP PTI
> > [   16.932437] CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted
> > 4.20.0-xfstests-10979-g96d4f267e40 #1
> > [   16.933752] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > BIOS 1.11.1-1ubuntu1 04/01/2014
> > [   16.935372] Workqueue: events uhid_device_add_worker
> > [   16.936321] RIP: 0010:hid_validate_values+0x48/0x110
>
> In a sense, it's good to have a fault there because this was added to
> make sure we do not blindly accept any data. The fact that it doesn't
> fail gracefully is a sign that there is something else.
> Maybe Roderick could have a look?
>
> Cheers,
> Benjamin
>

Sure I can have a look. Would you be able to share the sony.bin file?
Did you inject a particular device? We do a lot of remapping and
processing in hid-sony at startup. It is probably related to that.

Thanks,
Roderick

Re: [PATCH] HID: Add support for 146b:0902 Bigben Interactive Kids' gamepad

[Roderick Colenbrander]

Hi Hanno,

A few suggestions for your patch based on a quick review (had limited time).

In terms of code style, I noticed a lot of C++ style comments which
are not part of the kernel code style instead use C comments. To check
for potential other issues as well, run your patch through
'scripts/checkpatch.pl'.

I noticed you fixed up reported descriptors a bit to get axes remapped
in a different way. This is reasonable as the default mappings are
often not good. However, I would suggest use the mapping functions
instead (e.g. see hid-sony.c and other drivers). It also allows you to
properly remap buttons as well. I suspect the current button mapping
is all over the place as well. Make sure axis and button mapping
complies to the Linux gamepad spec (see
Documentation/input/gamepad.rst).

Thanks,
Roderick

On Wed, May 23, 2018 at 8:57 AM, Hanno Zulla  wrote:
> Hello,
>
> this is a driver for the "BigBen Interactive Kid-friendly Wired
> Controller PS3OFMINIPAD SONY" [1] with USB id 146b:0902. It was
> originally sold as a PS3 accessory and serves as a very nice gamepad for
> Retropie.
>
> With the help of serialusb [2], it was possible to analyze the protocol
> used by the gamepad and fix the missing entries for the descriptor.
>
>
> This is my *first* driver, so please review accordingly.
>
>
> Three things where I'm not sure and hope to hear from you about:
>
> - is that the correct use of hid_validate_values() in bigben_probe()?
>
> - is the error handling in bigben_probe() correct?
>
> - how do I properly test possible suspend/resume scenarios?
>
> Thanks,
>
> Hanno
>
>
> [1]
> 
>
> [2] 

Re: [PATCH] [v2] Input: Add "Share" button to Microsoft Xbox One controller.

[Roderick Colenbrander]

(resend in plain text)
+benjamin in a more explicit way

Hi Chris,

I see the need for the Record button. I wonder what makes sense from
the Linux kernel perspective. For DualShock 4 and DualSense there is a
Share button too (it introduced it). For DualShock 4 that was mapped
to 'Select' I think per Linux gamepad spec. For DualSense in 5.12 we
did the same... Though if there is a true 'Record' button we want to
use moving forward. Maybe we still want to change the button
definition for DualSensein 5.12 while we can...

It would be good to get some consensus on these buttons.

Thanks,
Roderick


On Thu, Mar 4, 2021 at 6:25 PM Chris Ye  wrote:
>
> Hi Bastien,  just want to follow up again on this.  I've checked again
> with the Xbox team that the "Share button" is given for the product,
> the HID usage profile and mapping to RECORD is what Xbox team expects
> and they want the same mapping for USB.
>
> Thanks!
> Chris
>
>
> On Tue, Mar 2, 2021 at 3:57 PM Chris Ye  wrote:
> >
> > Hi Bastien,
> > The "Share button" is a name Microsoft calls it, it actually has
> > HID descriptor defined in the bluetooth interface, which the HID usage
> > is:
> > consumer 0xB2:
> > 0x05, 0x0C,//   Usage Page (Consumer)
> > 0x0A, 0xB2, 0x00,  //   Usage (Record)
> > Microsoft wants the same key code to be generated consistently for USB
> > and bluetooth.
> > Thanks!
> > Chris
> >
> >
> > On Tue, Mar 2, 2021 at 1:50 AM Bastien Nocera  wrote:
> > >
> > > On Thu, 2021-02-25 at 05:32 +, Chris Ye wrote:
> > > > Add "Share" button input capability and input event mapping for
> > > > Microsoft Xbox One controller.
> > > > Fixed Microsoft Xbox One controller share button not working under USB
> > > > connection.
> > > >
> > > > Signed-off-by: Chris Ye 
> > > > ---
> > > >  drivers/input/joystick/xpad.c | 9 -
> > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/input/joystick/xpad.c
> > > > b/drivers/input/joystick/xpad.c
> > > > index 9f0d07dcbf06..0c3374091aff 100644
> > > > --- a/drivers/input/joystick/xpad.c
> > > > +++ b/drivers/input/joystick/xpad.c
> > > > @@ -79,6 +79,7 @@
> > > >  #define MAP_DPAD_TO_BUTTONS(1 << 0)
> > > >  #define MAP_TRIGGERS_TO_BUTTONS(1 << 1)
> > > >  #define MAP_STICKS_TO_NULL (1 << 2)
> > > > +#define MAP_SHARE_BUTTON   (1 << 3)
> > > >  #define DANCEPAD_MAP_CONFIG(MAP_DPAD_TO_BUTTONS
> > > > |  \
> > > > MAP_TRIGGERS_TO_BUTTONS |
> > > > MAP_STICKS_TO_NULL)
> > > >
> > > > @@ -130,6 +131,7 @@ static const struct xpad_device {
> > > > { 0x045e, 0x02e3, "Microsoft X-Box One Elite pad", 0,
> > > > XTYPE_XBOXONE },
> > > > { 0x045e, 0x02ea, "Microsoft X-Box One S pad", 0, XTYPE_XBOXONE
> > > > },
> > > > { 0x045e, 0x0719, "Xbox 360 Wireless Receiver",
> > > > MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360W },
> > > > +   { 0x045e, 0x0b12, "Microsoft X-Box One X pad",
> > > > MAP_SHARE_BUTTON, XTYPE_XBOXONE },
> > > > { 0x046d, 0xc21d, "Logitech Gamepad F310", 0, XTYPE_XBOX360 },
> > > > { 0x046d, 0xc21e, "Logitech Gamepad F510", 0, XTYPE_XBOX360 },
> > > > { 0x046d, 0xc21f, "Logitech Gamepad F710", 0, XTYPE_XBOX360 },
> > > > @@ -862,6 +864,8 @@ static void xpadone_process_packet(struct usb_xpad
> > > > *xpad, u16 cmd, unsigned char
> > > > /* menu/view buttons */
> > > > input_report_key(dev, BTN_START,  data[4] & 0x04);
> > > > input_report_key(dev, BTN_SELECT, data[4] & 0x08);
> > > > +   if (xpad->mapping & MAP_SHARE_BUTTON)
> > > > +   input_report_key(dev, KEY_RECORD, data[22] & 0x01);
> > > >
> > > > /* buttons A,B,X,Y */
> > > > input_report_key(dev, BTN_A,data[4] & 0x10);
> > > > @@ -1669,9 +1673,12 @@ static int xpad_init_input(struct usb_xpad
> > > > *xpad)
> > > >
> > > > /* set up model-specific ones */
> > > > if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype ==
> > > > XTYPE_XBOX360W ||
> > > > -   xpad->xtype == XTYPE_XBOXONE) {
> > > > +   xpad->xtype == XTYPE_XBOXONE) {
> > > > for (i = 0; xpad360_btn[i] >= 0; i++)
> > > > input_set_capability(input_dev, EV_KEY,
> > > > xpad360_btn[i]);
> > > > +   if (xpad->mapping & MAP_SHARE_BUTTON) {
> > > > +   input_set_capability(input_dev, EV_KEY,
> > > > KEY_RECORD);
> > >
> > > Is there not a better keycode to use than "Record"? Should a "share"
> > > keycode be added?
> > >
> > > I couldn't find a share button in the most recent USB HID Usage Tables:
> > > https://www.usb.org/document-library/hid-usage-tables-121
> > >
> > > > +   }
> > > > } else {
> > > > for (i = 0; xpad_btn[i] >= 0; i++)
> > > > input_set_capability(input_dev, EV_KEY,
> > > > xpad_btn[i]);
> > >
> > >

[PATCH] HID: uhid: Fixes a bug with userspace bluetooth stacks, which causes hangs during certain operations

[Roderick Colenbrander]

Many devices use userspace bluetooth stacks like BlueZ or Bluedroid in 
combination
with uhid. If any of these stacks is used with a HID device for which the driver
performs a HID request as part .probe (or technically another HID operation),
this results in a deadlock situation. The deadlock results in a 5 second timeout
for I/O operations in HID drivers, so isn't fatal, but none of the I/O 
operations
have a chance of succeeding.

The root cause for the problem is that uhid only allows for one request to be
processed at a time per uhid instance and locks out other operations. This means
that if a user space is creating a new HID device through 'UHID_CREATE', which
ultimately triggers '.probe' through the HID layer. Then any HID request e.g. a
read for calibration data would trigger a HID operation on uhid again, but it
won't go out to userspace, because it is still stuck in UHID_CREATE.
In addition bluetooth stacks are typically single threaded, so they wouldn't be
able to handle any requests while waiting on uhid.

Lucikly the UHID spec is somewhat flexible and allows for fixing the issue,
without breaking user space. The idea which the patch implements as discussed
with David Herrmann is to decouple adding of a hid device (which triggers 
.probe)
from UHID_CREATE. The work will kick off roughly once UHID_CREATE completed (or
else will wait a tiny bit of time in .probe for a lock). A HID driver has to 
call
HID to call 'hid_hw_start()' as part of .probe once it is ready for I/O, which
triggers UHID_START to user space. Any HID operations should function now within
.probe and won't deadlock because userspace is stuck on UHID_CREATE.

We verified this patch on Bluedroid with Android 6.0 and on desktop Linux with
BlueZ stacks. Prior to the patch they had the deadlock issue.

Signed-off-by: Roderick Colenbrander 
Cc: sta...@vger.kernel.org
---
 drivers/hid/uhid.c | 33 -
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 16b6f11..99ec3ff 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -51,10 +51,26 @@ struct uhid_device {
u32 report_id;
u32 report_type;
struct uhid_event report_buf;
+   struct work_struct worker;
 };
 
 static struct miscdevice uhid_misc;
 
+static void uhid_device_add_worker(struct work_struct *work)
+{
+   struct uhid_device *uhid = container_of(work, struct uhid_device, 
worker);
+   int ret;
+
+   ret = hid_add_device(uhid->hid);
+   if (ret) {
+   hid_err(uhid->hid, "Cannot register HID device: error %d\n", 
ret);
+
+   hid_destroy_device(uhid->hid);
+   uhid->hid = NULL;
+   uhid->running = false;
+   }
+}
+
 static void uhid_queue(struct uhid_device *uhid, struct uhid_event *ev)
 {
__u8 newhead;
@@ -498,18 +514,14 @@ static int uhid_dev_create2(struct uhid_device *uhid,
uhid->hid = hid;
uhid->running = true;
 
-   ret = hid_add_device(hid);
-   if (ret) {
-   hid_err(hid, "Cannot register HID device\n");
-   goto err_hid;
-   }
+   /* Adding of a HID device is done through a worker, to allow HID drivers
+* which use feature requests during .probe to work, without they would
+* be blocked on devlock, which is held by uhid_char_write.
+*/
+   schedule_work(&uhid->worker);
 
return 0;
 
-err_hid:
-   hid_destroy_device(hid);
-   uhid->hid = NULL;
-   uhid->running = false;
 err_free:
kfree(uhid->rd_data);
uhid->rd_data = NULL;
@@ -550,6 +562,8 @@ static int uhid_dev_destroy(struct uhid_device *uhid)
uhid->running = false;
wake_up_interruptible(&uhid->report_wait);
 
+   cancel_work_sync(&uhid->worker);
+
hid_destroy_device(uhid->hid);
kfree(uhid->rd_data);
 
@@ -612,6 +626,7 @@ static int uhid_char_open(struct inode *inode, struct file 
*file)
init_waitqueue_head(&uhid->waitq);
init_waitqueue_head(&uhid->report_wait);
uhid->running = false;
+   INIT_WORK(&uhid->worker, uhid_device_add_worker);
 
file->private_data = uhid;
nonseekable_open(inode, file);
-- 
2.5.5

Re: [PATCH] HID: uhid: Fixes a bug with userspace bluetooth stacks, which causes hangs during certain operations

[Roderick Colenbrander]

Please ignore this revision of the patch, there was a small merge
conflict on my end.

On Thu, Apr 21, 2016 at 2:15 PM, kbuild test robot  wrote:
> Hi,
>
> [auto build test WARNING on hid/for-next]
> [also build test WARNING on v4.6-rc4 next-20160421]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improving the system]
>
> url:
> https://github.com/0day-ci/linux/commits/roderick-gaikai-com/HID-uhid-Fixes-a-bug-with-userspace-bluetooth-stacks-which-causes-hangs-during-certain-operations/20160422-050505
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next
> config: x86_64-randconfig-x003-201616 (attached as .config)
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All warnings (new ones prefixed by >>):
>
>In file included from arch/x86/include/asm/atomic.h:4:0,
> from include/linux/atomic.h:4,
> from drivers/hid/uhid.c:13:
>drivers/hid/uhid.c: In function 'uhid_event_from_user':
>drivers/hid/uhid.c:403:6: error: implicit declaration of function 
> 'is_compat_task' [-Werror=implicit-function-declaration]
>  if (is_compat_task()) {
>  ^
>include/linux/compiler.h:151:30: note: in definition of macro '__trace_if'
>  if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
>  ^
>>> drivers/hid/uhid.c:403:2: note: in expansion of macro 'if'
>  if (is_compat_task()) {
>  ^
>cc1: some warnings being treated as errors
>
> vim +/if +403 drivers/hid/uhid.c
>
>387  __u8 phys[64];
>388  __u8 uniq[64];
>389
>390  compat_uptr_t rd_data;
>391  __u16 rd_size;
>392
>393  __u16 bus;
>394  __u32 vendor;
>395  __u32 product;
>396  __u32 version;
>397  __u32 country;
>398  } __attribute__((__packed__));
>399
>400  static int uhid_event_from_user(const char __user *buffer, size_t len,
>401  struct uhid_event *event)
>402  {
>  > 403  if (is_compat_task()) {
>404  u32 type;
>405
>406  if (get_user(type, buffer))
>407  return -EFAULT;
>408
>409  if (type == UHID_CREATE) {
>410  /*
>411   * This is our messed up request with compat 
> pointer.
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation



-- 
Roderick Colenbrander
Manager of Software Engineering
Gaikai, a Sony Computer Entertainment Company
roder...@gaikai.com

Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface

[Roderick Colenbrander]

On Fri, May 10, 2019 at 1:57 AM Bastien Nocera  wrote:
>
> On Sun, 2019-04-14 at 09:26 -0700, Roderick Colenbrander wrote:
> >
> 
> > We at the time were one of the first to expose acceleration and gyro
> > data through /dev/input for DualShock 4 as supported by hid-sony. We
> > report acceleration in 'g' and angular velocity in 'degree / s'. We
> > set the resolution to respectively '1 g' and '1 degree / s'. The range
> > we set to the range of the device e.g. for DS4 -4g to +4g for
> > acceleration. I need to check though what coordinate system we use,
> > but I think it is right handed (gyro counter clockwise relative to
> > acceleration axes).
>
> How do you export the gyro information through the input device?

For each DS4, there are multiple evdev devices for a DS4 for
respectively "gamepad", touchpad and motion sensors. The motion
sensors one, has INPUT_PROP_ACCELEROMETER set. ABS_X/_Y_Z provide
acceleration and ABS_RX/_RY/_RZ provide gyro. When we added this we
also updated the input documentation (event-codes.rst) to allow gyro
to use the rotational axes.

> FWIW, we needed to do extra work in iio-sensor-proxy so that the
> accelerometer in the Sixaxis/DS4 joypads (and uDraw tablet) didn't
> appear as though they were accelerometer for the system:
> https://github.com/hadess/iio-sensor-proxy/commit/401d59e54b3123860180d4401e09df8a1e1bc6c3
>
> > The two other drivers using INPUT_PROC_ACCELEROMETER are hid-wacom and
> > hid-udraw-ps3 Wacom. Both seem to report resolution in 'g'  as well.
>
> I wrote hid-udraw-ps3, and it's reporting accelerometer data through
> input because the rest of the driver is input, and it didn't make much
> sense to use another subsystem for just that small portion of the
> events the device sends out.
>

Re: [PATCH 2/2] Input: add motion-tracking ABS_* bits and docs

[Roderick Colenbrander]

On Thu, Sep 29, 2016 at 12:25 AM, Benjamin Tissoires
 wrote:
>
> On Sep 28 2016 or thereabouts, Roderick Colenbrander wrote:
> > On Wed, Sep 28, 2016 at 10:39 AM, Dmitry Torokhov
> >  wrote:
> > >
> > > On Tue, Sep 27, 2016 at 4:38 PM, Roderick Colenbrander
> > >  wrote:
> > > > From: Roderick Colenbrander 
> > > >
> > > > This patch introduces new axes for acceleration and angular velocity.
> > > > David Herrmann's work served as a base, but we extended the 
> > > > specification
> > > > with various changes inspired by real devices and challenges we see
> > > > when doing motion tracking.
> > > >
> > > > - Changed unit of acceleration to G instead of m/s^2. We felt that m/s^2
> > > >   is not the appropriate unit to return, because accelerometers are most
> > > >   often calibrated based on gravity. They return values in multiples of
> > > >   G and since we don't know the device location on earth, we should not
> > > >   blindly multiply by '9.8' for accuracy reasons. Such conversion is 
> > > > left
> > > >   to userspace.
> > > > - Resolution field is used for acceleration and gyro to report 
> > > > precision.
> > > >   The previous spec, specified to map 1 unit to e.g. 0.001 deg/s or 
> > > > 0.001 m/s^2.
> > > >   This is of course simpler for applications, but unit definition is a 
> > > > bit
> > > >   arbitrary. Previous axes definitions used the resolution field, which
> > > >   felt more consistent.
> > > > - Added section on timestamps, which are important for accurate motion
> > > >   tracking purposes. The use of MSC_TIMESTAMP was recommended in this
> > > >   situation to get access to the hardware timestamp if available.
> > > > - Changed motion axes to be defined as a right-handed coordinate system.
> > > >   Due to this change the gyro vectors are now defined as 
> > > > counter-clockwise.
> > > >   The overall changes makes the definitions consistent with computer 
> > > > graphics.
> > > >
> > > > [PATCH 4/4] Input: add motion-tracking ABS_* bits and docs
> > > > David Herrmann 
> > > > Tue Dec 17 07:48:54 PST 2013
> > > >
> > > > Motion sensors are getting quite common in mobile devices. To avoid
> > > > returning accelerometer data via ABS_X/Y/Z and irritating the Xorg
> > > > mouse-driver, this adds separate ABS_* bits for that.
> > >
> > > We have IIO for motions sensors that are not strictly human input
> > > devices; I believe there is also IIO->input bridge where generic IIO
> > > sensors could be mapped to input device if they are supposed to be
> > > used as such in given product.
> > >
> >
> > If we decide to move forward in the direction proposed by this patch,
> > the spec could be updated
> > to limit the scope a bit or to make it wider.
> >
> >
> > > >
> > > > This is needed if gaming devices want to report their normal data plus
> > > > accelerometer/gyro data. Usually, ABS_X/Y are already used by analog
> > > > sticks, so need separate definitions, anyway.
> > >
> > > I am not sure if this direction is sustainable. We can't keep adding
> > > more and more ABS axes every time we add another control to something
> > > that is basically a composite device. What if you add another stick?
> > > Magnetometer? Some other sensor?
> > >
> > > I think the only reasonable way it to come up with a notion of
> > > "composite" input device consisting of several event nodes and have
> > > userspace "assemble" it all together.
> > >
> >
> > In our case we are interested in the motion functionality for some devices
> > with drivers already in the kernel, which we want to extend over time with
> > improved capabilities.
> >
> > I understand your concerns about the scalability of ABS axes in general.
> > If someone were to come up with some crazy flight simulator joystick with 
> > many
> > weird axes, do you then add an ABS_X2, ABS_X3 etcetera? Similar what if
> > a controller for whatever reasons shipped with multiple gyroscopes,
> > accelerometers,
> > magnetic sensor, heartrate sensors etcetera?
> >
> > A composite device would on the other hand be more of a pain for the 
> > different
> > userland APIs ranging

Re: [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface

[Roderick Colenbrander]

Hi Jonathan and Nikolaus,

I would like to jump into this discussion a bit as well. Might be
slightly off topic or not... I'm not too familiar with the details of
IIO, but as Sony we do report accelerometer / gyroscope data through
/dev/input for our DualShock devices through hid-sony. This is among
reasons done for Android, which Nikolaus showed interest in as well.
We are working on exposing this data through user space through
standard Android APIs (see,
https://android-review.googlesource.com/c/platform/hardware/libhardware/+/571762/).
The work will likely be merged later this year and it is already used
in some devices. Main reason it wasn't merged yet it that in our case
we have multiple evdev devices (gamepad, touchpad, motion sensors) and
need to tie those together for applications. Android doesn't support a
good mechanism for that yet.

Since the IIO input work relates to our Android work as well, I will
jump in more from the sidelines to make sure things are done in a
consistent manner. The main concern are resolution and value ranges.

Thanks,
Roderick

On Sun, Apr 14, 2019 at 4:41 AM Jonathan Cameron  wrote:
>
> On Mon, 8 Apr 2019 15:15:56 +0200
> H. Nikolaus Schaller  wrote:
>
> > Hi Jonathan,
> >
> > > Am 07.04.2019 um 14:30 schrieb Jonathan Cameron :
> > >
> > > On Sun, 31 Mar 2019 12:09:46 +0200
> > > "H. Nikolaus Schaller"  wrote:
> > >
> > > Hi Nikolaus,
> > >
> > > I'm probably going to repeat a few things I sent for v1 as the audience 
> > > has
> > > expanded somewhat!
> > >
> > > Good to see this moving forwards though as there has been at least some 
> > > demand
> > > for it going way back to the early days of IIO.
> > >
> > >> Some user spaces (e.g. some Android devices) use /dev/input/event* for 
> > >> handling
> > >> the 3D position of the device with respect to the center of gravity 
> > >> (earth).
> > >> This can be used for gaming input, auto-rotation of screens etc.
> > >>
> > >> This interface should be the standard for such use cases because it is 
> > >> an abstraction
> > >> of how orientation data is acquired from sensor chips. Sensor chips may 
> > >> be connected
> > >> through different interfaces and in different positions. They may also 
> > >> have different
> > >> parameters. And, if a chip is replaced by a different one, the values 
> > >> reported by
> > >> the device position interface should remain the same, provided the 
> > >> device tree reflects
> > >> the changed chip.
> > >>
> > >> This did initially lead to input accelerometer drivers like 
> > >> drivers/input/misc/bma150.c
> > >> or drivers/misc/lis3lv02d/
> > >>
> > >> But nowadays, new accelerometer chips mostly get iio drivers and rarely 
> > >> input drivers.
> > >>
> > >> Therefore we need something like a protocol stack which bridges raw data 
> > >> and input devices.
> > >> It can be seen as a similar layering like TCP/IP vs. bare Ethernet. Or 
> > >> keyboard
> > >> input events vs. raw gpio or raw USB access.
> > >>
> > >> This patch bridges the gap between raw iio data and the input device 
> > >> abstraction
> > >> so that accelerometer measurements can additionally be presented as 
> > >> X/Y/Z accelerometer
> > >> channels (INPUT_PROP_ACCELEROMETER) through /dev/input/event*.
> > >>
> > >> There are no special requirements or changes needed for an iio driver.
> > >>
> > >> There is no need to define a mapping (e.g. in device tree).
> > > This worries me, as it inherently means we end up with this interface 
> > > being
> > > registered in cases where it makes no sense.  A lot of generic distros get
> > > used across widely differing use cases.
> >
> > I still do not fully understand what is worrying you here.
>
> >
> > Do you worry about functionality, flexibility or resources or something 
> > else?
>
> Two main things:
> 1) Lack of generality of the approach.
>This is a single use trick for input devices. Why does it make sense for
>input devices?  There are lots of other in kernel users and potential
>ones in the future.  The ability to register additional IIO consumers like
>this is useful, lets make it useful to everyone.
>
> 2) To much generality of the specific usecase.  I don't want to put an Input
>interface on accelerometers where it makes no sense.  The rule of it has
>2-3 axis so it must make sense isn't good enough to my mind.  How
>does userspace know which accelerometer to use (more and more devices have
>multiple?)  You could do something like looking at the location info from
>DT / ACPI in your driver and pick the 'best' but that's policy. Should be
>in userspace.  Sure you can just use the right input driver, but the moment
>we do that, we need aware userspace, if that's the case why not make it
>aware from the start.
>
> Believe me I've been round this one a good few times and thought about it
> a lot.  I'll take a lot of convincing that this isn't a problem that
> should be pushed into userspace.
>
> >
> > I think havi

[PATCH 0/2] Input: ABS2 and motion tracking

[Roderick Colenbrander]

From: Roderick Colenbrander 

For some input driver work we have been doing, we were limited by not
having a wide enough ABS range in evdev. The current ABS range is mostly
full and due to limitations on the ioctl design, it can't be extended.

About 3 years ago, David Herrmann did work in this area to address this
problem for similar reasons as us. However his patches at the time didn't
make it in.

Since we have a need for this kind of functionality, we took over his
patches and are submitting them again with various improvements based
on feedback to the original patches and some new functionality for motion
tracking we are interested in ourselves. The patchset is simpler than before,
because at the time a uinput overhaul was needed, before uinput could even
support ABS_MAX2, which has happened since then.

The patches have been tested with some David's libevdev branch which
supports the new APIs. In addition we verified the new APIs in some
of internal software.

Thanks,
Roderick Colenbrander
Gaikai Inc, a Sony Interactive Entertainment Company

Roderick Colenbrander (2):
  Input: introduce ABS_MAX2/CNT2 and friends
  Input: add motion-tracking ABS_* bits and docs

 Documentation/input/gamepad.txt  |   9 +-
 Documentation/input/motion-tracking.txt  | 176 +++
 drivers/hid/hid-debug.c  |   2 +-
 drivers/hid/hid-input.c  |   2 +-
 drivers/input/evdev.c|  95 -
 drivers/input/input.c|  14 +--
 drivers/input/keyboard/goldfish_events.c |   6 +-
 drivers/input/keyboard/hil_kbd.c |   2 +-
 drivers/input/misc/uinput.c  |   8 +-
 include/linux/hid.h  |   2 +-
 include/linux/input.h|   6 +-
 include/linux/mod_devicetable.h  |   2 +-
 include/uapi/linux/input-event-codes.h   |  21 +++-
 include/uapi/linux/input.h   |  37 ++-
 14 files changed, 355 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/input/motion-tracking.txt

-- 
2.7.4

[PATCH 1/2] Input: introduce ABS_MAX2/CNT2 and friends

[Roderick Colenbrander]

From: Roderick Colenbrander 

David Herrmann's original patch was ported over to a modern Linux kernel.
In the process, we went over all the feedback to the original patch series
and added various improvements:

- evdev_handle_get_abs2 returns valid_cnt instead of 0 when succesfull
- Updated documentation of EVIOCGABS2/EVIOCSABS2 ioctls based.
- Clarified language around ABS_MAX2 definition, to state the *ABS2 ioctls
  should be used for the full range.

[PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends

David Herrmann 
Tue Dec 17 07:48:52 PST 2013
Previous message: [PATCH 1/4] Input: uinput: add full absinfo support
Next message: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
As we painfully noticed during the 3.12 merge-window our
EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
hacks to work around it but if we ever decide to increase ABS_MAX, the
EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
misinterpretations in the kernel that we cannot catch.

Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
ioctls to get/set abs-params. They no longer encode the ABS code in the
ioctl number and thus allow up to 4 billion ABS codes.

The new API also allows to query multiple ABS values with one call. To
allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
writes to ABS_MT_SLOT. Furthermore, for better compatibility with
newer user-space, we ignore writes to unknown codes. Hence, if we ever
increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
fine even on old kernels.

Note that we also need to increase EV_VERSION so user-space can reliably
know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
reliably without EVIOCGVERSION.

Signed-off-by: David Herrmann 
Signed-off-by: Roderick Colenbrander 
---
 drivers/hid/hid-debug.c  |  2 +-
 drivers/hid/hid-input.c  |  2 +-
 drivers/input/evdev.c| 95 +++-
 drivers/input/input.c| 14 ++---
 drivers/input/keyboard/goldfish_events.c |  6 +-
 drivers/input/keyboard/hil_kbd.c |  2 +-
 drivers/input/misc/uinput.c  |  8 +--
 include/linux/hid.h  |  2 +-
 include/linux/input.h|  6 +-
 include/linux/mod_devicetable.h  |  2 +-
 include/uapi/linux/input-event-codes.h   | 14 -
 include/uapi/linux/input.h   | 37 -
 12 files changed, 165 insertions(+), 25 deletions(-)

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index acfb522..7205d4a 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -963,7 +963,7 @@ static const char *relatives[REL_MAX + 1] = {
[REL_WHEEL] = "Wheel",  [REL_MISC] = "Misc",
 };
 
-static const char *absolutes[ABS_CNT] = {
+static const char *absolutes[ABS_CNT2] = {
[ABS_X] = "X",  [ABS_Y] = "Y",
[ABS_Z] = "Z",  [ABS_RX] = "Rx",
[ABS_RY] = "Ry",[ABS_RZ] = "Rz",
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index bcfaf32..26b161d 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1411,7 +1411,7 @@ static bool hidinput_has_been_populated(struct hid_input 
*hidinput)
for (i = 0; i < BITS_TO_LONGS(REL_CNT); i++)
r |= hidinput->input->relbit[i];
 
-   for (i = 0; i < BITS_TO_LONGS(ABS_CNT); i++)
+   for (i = 0; i < BITS_TO_LONGS(ABS_CNT2); i++)
r |= hidinput->input->absbit[i];
 
for (i = 0; i < BITS_TO_LONGS(MSC_CNT); i++)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..569e425 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -815,7 +815,7 @@ static int handle_eviocgbit(struct input_dev *dev,
case  0: bits = dev->evbit;  len = EV_MAX;  break;
case EV_KEY: bits = dev->keybit; len = KEY_MAX; break;
case EV_REL: bits = dev->relbit; len = REL_MAX; break;
-   case EV_ABS: bits = dev->absbit; len = ABS_MAX; break;
+   case EV_ABS: bits = dev->absbit; len = ABS_MAX2; break;
case EV_MSC: bits = dev->mscbit; len = MSC_MAX; break;
case EV_LED: bits = dev->ledbit; len = LED_MAX; break;
case EV_SND: bits = dev->sndbit; len = SND_MAX; break;
@@ -827,6 +827,93 @@ static int handle_eviocgbit(struct input_dev *dev,
return bits_to_user(bits, len, size, p, compat_mode);
 }
 
+static int evdev_handle_get_abs2(struct input_dev *dev, void __user *p)
+{
+   u32 code, cnt, valid_cnt, i;
+   struct input_absinfo2 __user *pinfo = p;
+   struct input_absinfo abs;
+
+   if

[PATCH 2/2] Input: add motion-tracking ABS_* bits and docs

[Roderick Colenbrander]

From: Roderick Colenbrander 

This patch introduces new axes for acceleration and angular velocity.
David Herrmann's work served as a base, but we extended the specification
with various changes inspired by real devices and challenges we see
when doing motion tracking.

- Changed unit of acceleration to G instead of m/s^2. We felt that m/s^2
  is not the appropriate unit to return, because accelerometers are most
  often calibrated based on gravity. They return values in multiples of
  G and since we don't know the device location on earth, we should not
  blindly multiply by '9.8' for accuracy reasons. Such conversion is left
  to userspace.
- Resolution field is used for acceleration and gyro to report precision.
  The previous spec, specified to map 1 unit to e.g. 0.001 deg/s or 0.001 m/s^2.
  This is of course simpler for applications, but unit definition is a bit
  arbitrary. Previous axes definitions used the resolution field, which
  felt more consistent.
- Added section on timestamps, which are important for accurate motion
  tracking purposes. The use of MSC_TIMESTAMP was recommended in this
  situation to get access to the hardware timestamp if available.
- Changed motion axes to be defined as a right-handed coordinate system.
  Due to this change the gyro vectors are now defined as counter-clockwise.
  The overall changes makes the definitions consistent with computer graphics.

[PATCH 4/4] Input: add motion-tracking ABS_* bits and docs
David Herrmann 
Tue Dec 17 07:48:54 PST 2013

Motion sensors are getting quite common in mobile devices. To avoid
returning accelerometer data via ABS_X/Y/Z and irritating the Xorg
mouse-driver, this adds separate ABS_* bits for that.

This is needed if gaming devices want to report their normal data plus
accelerometer/gyro data. Usually, ABS_X/Y are already used by analog
sticks, so need separate definitions, anyway.

Signed-off-by: David Herrmann 
Signed-off-by: Roderick Colenbrander 
---
 Documentation/input/gamepad.txt |   9 +-
 Documentation/input/motion-tracking.txt | 176 
 include/uapi/linux/input-event-codes.h  |   7 ++
 3 files changed, 190 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/input/motion-tracking.txt

diff --git a/Documentation/input/gamepad.txt b/Documentation/input/gamepad.txt
index 3f6d8a5..ed13782 100644
--- a/Documentation/input/gamepad.txt
+++ b/Documentation/input/gamepad.txt
@@ -57,6 +57,9 @@ Most gamepads have the following features:
   - Rumble
 Many devices provide force-feedback features. But are mostly just
 simple rumble motors.
+  - Motion-tracking
+Gamepads may include motion-tracking sensors like accelerometers and
+gyroscopes.
 
 3. Detection
 
@@ -138,8 +141,6 @@ Triggers:
   Upper trigger buttons are reported as BTN_TR or ABS_HAT1X (right) and BTN_TL
   or ABS_HAT1Y (left). Lower trigger buttons are reported as BTN_TR2 or
   ABS_HAT2X (right/ZR) and BTN_TL2 or ABS_HAT2Y (left/ZL).
-  If only one trigger-button combination is present (upper+lower), they are
-  reported as "right" triggers (BTN_TR/ABS_HAT1X).
 (ABS trigger values start at 0, pressure is reported as positive values)
 
 Menu-Pad:
@@ -155,5 +156,9 @@ Menu-Pad:
 Rumble:
   Rumble is advertised as FF_RUMBLE.
 
+Motion-tracking:
+  Motion-tracking is defined in ./Documentation/input/motion-tracking.txt and
+  gamepads shall comply to the rules defined there.
+
 
   Written 2013 by David Herrmann 
diff --git a/Documentation/input/motion-tracking.txt 
b/Documentation/input/motion-tracking.txt
new file mode 100644
index 000..d34a290
--- /dev/null
+++ b/Documentation/input/motion-tracking.txt
@@ -0,0 +1,176 @@
+   Motion Tracking API
+
+
+1. Intro
+
+Motion tracking devices produce device motion events generated from an
+accelerometer, gyroscope or compass. These data can be returned to user-space
+via input events. This document defines how these data are reported.
+
+2. Devices
+~~
+In this document, a "device" is one of:
+ - accelerometer
+ - gyroscope
+ - compass
+
+These devices returned their information via different APIs in the past. To
+unify them and define a common API, a set of input evdev codes was created. Old
+drivers might continue using their API, but developers are encouraged to use
+the input evdev API for new drivers.
+
+2.1 Axes
+
+Movement data is usually returned as absolute data for the 3 axes of a device.
+In this context, the three axes are defined in a right-handed coordinate
+system as:
+ - X: Axis goes from the left to the right side of the device
+ - Y: Axis goes from the bottom to the top of the device
+ - Z: Axis goes from the back to the front of the device
+
+The front of a device is the side faced to the u

Re: [PATCH 2/2] Input: add motion-tracking ABS_* bits and docs

[Roderick Colenbrander]

On Wed, Sep 28, 2016 at 10:39 AM, Dmitry Torokhov
 wrote:
>
> On Tue, Sep 27, 2016 at 4:38 PM, Roderick Colenbrander
>  wrote:
> > From: Roderick Colenbrander 
> >
> > This patch introduces new axes for acceleration and angular velocity.
> > David Herrmann's work served as a base, but we extended the specification
> > with various changes inspired by real devices and challenges we see
> > when doing motion tracking.
> >
> > - Changed unit of acceleration to G instead of m/s^2. We felt that m/s^2
> >   is not the appropriate unit to return, because accelerometers are most
> >   often calibrated based on gravity. They return values in multiples of
> >   G and since we don't know the device location on earth, we should not
> >   blindly multiply by '9.8' for accuracy reasons. Such conversion is left
> >   to userspace.
> > - Resolution field is used for acceleration and gyro to report precision.
> >   The previous spec, specified to map 1 unit to e.g. 0.001 deg/s or 0.001 
> > m/s^2.
> >   This is of course simpler for applications, but unit definition is a bit
> >   arbitrary. Previous axes definitions used the resolution field, which
> >   felt more consistent.
> > - Added section on timestamps, which are important for accurate motion
> >   tracking purposes. The use of MSC_TIMESTAMP was recommended in this
> >   situation to get access to the hardware timestamp if available.
> > - Changed motion axes to be defined as a right-handed coordinate system.
> >   Due to this change the gyro vectors are now defined as counter-clockwise.
> >   The overall changes makes the definitions consistent with computer 
> > graphics.
> >
> > [PATCH 4/4] Input: add motion-tracking ABS_* bits and docs
> > David Herrmann 
> > Tue Dec 17 07:48:54 PST 2013
> >
> > Motion sensors are getting quite common in mobile devices. To avoid
> > returning accelerometer data via ABS_X/Y/Z and irritating the Xorg
> > mouse-driver, this adds separate ABS_* bits for that.
>
> We have IIO for motions sensors that are not strictly human input
> devices; I believe there is also IIO->input bridge where generic IIO
> sensors could be mapped to input device if they are supposed to be
> used as such in given product.
>

If we decide to move forward in the direction proposed by this patch,
the spec could be updated
to limit the scope a bit or to make it wider.


> >
> > This is needed if gaming devices want to report their normal data plus
> > accelerometer/gyro data. Usually, ABS_X/Y are already used by analog
> > sticks, so need separate definitions, anyway.
>
> I am not sure if this direction is sustainable. We can't keep adding
> more and more ABS axes every time we add another control to something
> that is basically a composite device. What if you add another stick?
> Magnetometer? Some other sensor?
>
> I think the only reasonable way it to come up with a notion of
> "composite" input device consisting of several event nodes and have
> userspace "assemble" it all together.
>

In our case we are interested in the motion functionality for some devices
with drivers already in the kernel, which we want to extend over time with
improved capabilities.

I understand your concerns about the scalability of ABS axes in general.
If someone were to come up with some crazy flight simulator joystick with many
weird axes, do you then add an ABS_X2, ABS_X3 etcetera? Similar what if
a controller for whatever reasons shipped with multiple gyroscopes,
accelerometers,
magnetic sensor, heartrate sensors etcetera?

A composite device would on the other hand be more of a pain for the different
userland APIs ranging from libinput, SDL2, Android and other embedded
platforms. It
would be quite an extensive change. How would they even do the
stitching? You could
handle this through sysfs (not my favorite way) or maybe have a notion
of a 'master'
device being the current event node and some way to enumerate 'sensor'
nodes or something.

It ultimate won't be my call, but I find it hard to say whether such a
potential big overhaul
is warranted at this point.

Thanks,
Roderick


> >
> > Signed-off-by: David Herrmann 
> > Signed-off-by: Roderick Colenbrander 
> > ---
> >  Documentation/input/gamepad.txt |   9 +-
> >  Documentation/input/motion-tracking.txt | 176 
> > 
> >  include/uapi/linux/input-event-codes.h  |   7 ++
> >  3 files changed, 190 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/input/motion-tracking.txt
> >
> > diff --git a/Documentation/input/gamepad.txt 
> > b/Doc

Re: [PATCH 2/2] Input: add motion-tracking ABS_* bits and docs

[Roderick Colenbrander]

On Thu, Sep 29, 2016 at 1:55 AM,   wrote:
> On 28.09.2016 18:39, Dmitry Torokhov wrote:
>>
>> On Tue, Sep 27, 2016 at 4:38 PM, Roderick Colenbrander
>>  wrote:
>>>
>>> From: Roderick Colenbrander 
>>>
>>> This patch introduces new axes for acceleration and angular velocity.
>>> David Herrmann's work served as a base, but we extended the specification
>>> with various changes inspired by real devices and challenges we see
>>> when doing motion tracking.
>>>
>>> - Changed unit of acceleration to G instead of m/s^2. We felt that m/s^2
>>>   is not the appropriate unit to return, because accelerometers are most
>>>   often calibrated based on gravity.
>
> I'd so like to believe they are referenced to a standard g rather than
> whatever is true at the plant.  Almost no consumer parts are able to
> internally
> adjust their calibrations (or at least it's undocumented if they are).

Main motivation for g is that it is often easy to calibrate for.
Typically there are registers with device specific constants for
calibrated 1g values. At least in our case we are able to leverage
these.

>>>
>>> They return values in multiples of
>>>   G and since we don't know the device location on earth, we should not
>>>   blindly multiply by '9.8' for accuracy reasons. Such conversion is left
>>>   to userspace.
>
> Hmm. If userspace is involved in trimming the readings, then the units don't
> matter.
> Ah well, not important really. However, there is a question of generalised
> interfaces.  There are plenty of other devices (distance sensors etc) which
> measure
> in m. For IIO at least we have to support those as well so SI units are
> preferred.  I remember having the same discussion long long ago!

I think it really depends on the use case if trimming is needed. For
various situations just 'g' is fine (it is better than raw values).
Just the use case to go to m/s^2 is not possible at all in kernel
(without passing parameters to the kernel module, which is undesired).
Doing real calibration in user space would be a big challenge I think
and not sure if it is easy to standardize. Some devices calibration is
just about a multiplication and an offset, but other devices are
non-linear or temperature dependent and needing frequent
recalibration. It would best I think to basic calibration in kernel.

>>>
>>> - Resolution field is used for acceleration and gyro to report precision.
>>>   The previous spec, specified to map 1 unit to e.g. 0.001 deg/s or 0.001
>>> m/s^2.
>>>   This is of course simpler for applications, but unit definition is a
>>> bit
>>>   arbitrary. Previous axes definitions used the resolution field, which
>>>   felt more consistent.
>>> - Added section on timestamps, which are important for accurate motion
>>>   tracking purposes. The use of MSC_TIMESTAMP was recommended in this
>>>   situation to get access to the hardware timestamp if available.
>
> The moment you are into doing motion tracking I'm really thinking shoving it
> through input makes little sense...   This is one of the main things
> IIO is set up to do...

The devices we intend to support through this API are actual HID
devices. They have the timestamp and motion sensor values within the
same HID report as the other HID values come in (buttons, sticks, ..).
That's basically the background on why we thought to add this to the
spec, we noticed significant enough variation to the Linux timestamps.

>>> - Changed motion axes to be defined as a right-handed coordinate system.
>>>   Due to this change the gyro vectors are now defined as
>>> counter-clockwise.
>>>   The overall changes makes the definitions consistent with computer
>>> graphics.
>>>
>>> [PATCH 4/4] Input: add motion-tracking ABS_* bits and docs
>>> David Herrmann 
>>> Tue Dec 17 07:48:54 PST 2013
>>>
>>> Motion sensors are getting quite common in mobile devices. To avoid
>>> returning accelerometer data via ABS_X/Y/Z and irritating the Xorg
>>> mouse-driver, this adds separate ABS_* bits for that.
>>
>>
>> We have IIO for motions sensors that are not strictly human input
>> devices; I believe there is also IIO->input bridge where generic IIO
>> sensors could be mapped to input device if they are supposed to be
>> used as such in given product.
>
> Yeah, *looks guilty* I've been failing to actually submit the input
> bridge for a quite some time due to the open question of how the heck
> we describe the connectivity (device tree etc) after all the