Return-Path: Date: Thu, 8 May 2014 22:36:09 +0300 From: Johan Hedberg To: Petri Gynther Cc: linux-bluetooth Subject: Re: [PATCH] input: Add userspace HID support Message-ID: <20140508193609.GA13996@t440s.P-661HNU-F1> References: <20140506004501.318D810193E@puck.mtv.corp.google.com> <20140508073646.GA14385@t440s.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 In-Reply-To: Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Petri, On Thu, May 08, 2014, Petri Gynther wrote: > On Thu, May 8, 2014 at 12:36 AM, Johan Hedberg wrote: > > Hi Petri, > > > > On Mon, May 05, 2014, Petri Gynther wrote: > >> Enable HID protocol handling in userspace when UserspaceHID=true in input.conf. > >> > >> Benefits of userspace HID: > >> 1. Persistent HID/input pipeline > >> For a Bluetooth HID device, the corresponding kernel HID/input devices are > >> created only once when the Bluetooth HID device is used the first time. > >> The HID/input pipeline is not destroyed and recreated every time when > >> the Bluetooth HID device disconnects and reconnects. > >> > >> 2. HID vs HoG parity > >> Enables HID and HoG devices to operate the same way in BlueZ stack, using > >> uHID kernel module (/dev/uhid) to pass HID report data between bluetoothd > >> and kernel HID subsystem. > >> > >> 3. Debugging > >> It is easier to debug HID protocol in userspace than in HIDP kernel module. > >> --- > >> Makefile.plugins | 3 +- > >> profiles/input/device.c | 674 +++++++++++++++++++++++++++++++++++++++++++-- > >> profiles/input/device.h | 1 + > >> profiles/input/hidp_defs.h | 79 ++++++ > >> profiles/input/input.conf | 4 + > >> profiles/input/manager.c | 10 + > >> 6 files changed, 754 insertions(+), 17 deletions(-) > >> create mode 100644 profiles/input/hidp_defs.h > > > > Considering that we're using uhid also for the Android HID solution > > (see android/hidhost.c) is there an opportunity here to avoid code > > duplication by sharing some code? I'm not saying that this must be done > > before we apply your patch, but if code sharing is possible it'd be good > > to try to refactor common parts into src/shared/hid.c or similar. > > > > Johan > > I think Marcel also mentioned this earlier. I can certainly look into > refactoring, but I'd prefer to do that after this patch is applied, so > we have a working userspace HID solution as a baseline. Fair enough. I was just about to apply your patch, but it has actually some compiler warnings/errors that need fixing: CC profiles/input/bluetoothd-device.o profiles/input/device.c: In function ‘hidp_send_message’: profiles/input/device.c:175:10: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] if (len < size) { ^ profiles/input/device.c: In function ‘uhid_send_feature_answer’: profiles/input/device.c:230:10: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] if (len < sizeof(ev)) { ^ profiles/input/device.c: In function ‘uhid_send_input_report’: profiles/input/device.c:272:10: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] if (len < sizeof(ev)) { ^ profiles/input/device.c: In function ‘uhid_watch_cb’: profiles/input/device.c:683:10: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] if (len < sizeof(ev.type)) { ^ cc1: all warnings being treated as errors make[1]: *** [profiles/input/bluetoothd-device.o] Error 1 make: *** [all] Error 2 Please always use ./bootstrap-configure && make or at least --enable-maintainer-mode to catch as many issues as possible. For the record, my gcc version is "gcc (GCC) 4.8.2 20131212 (Red Hat 4.8.2-7)" from Fedora 20. This particular error is a bit silly since you do have the correct type for the read/write return parameter, but you'll simply need to do an explicit type cast into size_t in the comparison. Johan