bthidd: Add evdev protocol support

[wulf]

Summary

This implements native evdev support for bluetooth keyboards and mouses through bthidd daemon and uinput kernel interface

User-visible changes:

1. bthidcontrol and bthidd.conf are extended to query and store bt device name.
2. -u option is added to bthidd options to enable evdev support.
3. bthidd_evdev_support rc.conf variable is added to control bthidd's "-u" option. (possible values "YES", "NO", "AUTO"(default). "AUTO" depends on kernel EVDEV_SUPPORT option)
4. All??? consumer HID usage page keyboard events are supported.
5. kern.evdev.rcpt_mask sysctl is respected, so "sysctl kern.evdev.rcpt_mask=12" should be executed if EVDEV_SUPPORT is compiled into kernel.

Test Plan

Apply patch, rebuild, reinstall and restart uinput kernel module, bthidcontrol, bthidd and all /etc scripts.
Regenerate bthidd.conf entries with bthidcontrol (recommended).
Add bthidd_evdev_support="YES" to rc.conf (if kernel is not compiled with EVDEV_SUPPORT option enabled)
Restart bthidd
Test mouse/keyboard events with evemu-record tool from devel/evemu port or with xf86-input-evdev driver of Xorg-server

[gonzo]

Kernel part and overall style looks fine. I skimmed through userland code and didn't find any obvious blunders but I guess final say on funcitonal side of this change is up to emax@

[emax]

this looks great. my only comments are related to newly added "name" option / parameter. i can not seem to understand its purpose.

usr.sbin/bluetooth/bthidcontrol/sdp.c

52

i can not say i'm a huge fan of things like this. do you think it would be possible to create a public api for it?

128

i think this entire function should be moved into shared library. i would think requesting remote device name would be a very common operation.

250

again, not a huge fan of this #ifdef. i would prefer public api instead

292

what is the purpose of "name"? is it just to add "user-friendly"ness? or did i miss some logic based on device name?

[val_packett.cool]

Nice! Tested my Apple Magic Trackpad with this. Works as a basic mouse. But would be much better to have actual touchpad support (absolute motion, multi-touch)…

[wulf]

• Add getter for SDP session socket handle.
• Use this getter to avoid access to private members.
• hid_hci_remote_name_query now takes local bdattr as parameter not local HCI devname

[wulf]

usr.sbin/bluetooth/bthidcontrol/sdp.c

52

sdp_get_socket() has been added to libsdp to make required variable available to public

292

Device name is mandatory part of evdev protocol. Here it is copied into kernel with UI_DEV_SETUP ioctl in uinput_open_common() function.

[wulf]

In D13456#283044, @emax wrote:

my only comments are related to newly added "name" option / parameter. i can not seem to understand its purpose.

As I wrote before device name is mandatory part of evdev protocol. It passed unchanged through the kernel to client software so it can be an arbitrary string (not longer than 80 chars).
It is used in uinput_open_common() function which looks slightly obfuscated. The function attempts to set evdev device name with UI_DEV_SETUP ioctl and tries to use "name" field from bthidd.conf
than fallbacks to device hostname taken from /etc/bluetooth/hosts and than fallbacks to simple "Bluetooth Keyboard" or "Bluetooth Mouse".

[wulf]

In D13456#283295, @greg_unrelenting.technology wrote:

Nice! Tested my Apple Magic Trackpad with this. Works as a basic mouse. But would be much better to have actual touchpad support (absolute motion, multi-touch)…

Thanks for testing! I do not plan to support apple hardware as I do not own any. I you want to dig around it, you can start from r322440 https://svnweb.freebsd.org/base?view=revision&revision=322440 (Apple Magic Mouse support) as this commit contains at least 2 basic parts: 1. It sends magic report to enable absolute mode 2. It parses absolute mode reports.

[val_packett.cool]

In D13456#283348, @wulf wrote:

Thanks for testing! I do not plan to support apple hardware as I do not own any. I you want to dig around it, you can start from r322440 https://svnweb.freebsd.org/base?view=revision&revision=322440 (Apple Magic Mouse support) as this commit contains at least 2 basic parts: 1. It sends magic report to enable absolute mode 2. It parses absolute mode reports.

Okay, that was quite simple :) Something like this: https://github.com/myfreeweb/freebsd/commit/0c06a324311309228a8e2def334c603a390fac4f

I guess I can post it here after this gets merged.

[bcr]

A small man page fix.

usr.sbin/bluetooth/bthidd/bthidd.8

88

s/in to/into the/

[wulf]

1. Fix bthidd manpage (by bcr).
2. Add forgotten INPUT_PROP_POINTER property to evdev mouse descriptor.
3. Minor uinput_rep_mouse() simplification (no functional changes).
4. Comment evdev device name selection sequence.

[wulf]

usr.sbin/bluetooth/bthidcontrol/sdp.c

128

I see no existing library this function should belong to. May be it have some sense to split libhci off hccontrol utility? But it requires quite a lot of work.

usr.sbin/bluetooth/bthidd/bthidd.8

88

Fixed. Thanks.

[wulf]

In D13456#283582, @greg_unrelenting.technology wrote:

Okay, that was quite simple :) Something like this: https://github.com/myfreeweb/freebsd/commit/0c06a324311309228a8e2def334c603a390fac4f

I guess I can post it here after this gets merged.

Looks promising. Did you check what is transferring through sysmouse interface after trackpad has been switched to absolute mode?

[emax]

usr.sbin/bluetooth/bthidcontrol/sdp.c

52

thank you. it looks cleaner, but still, i would prefer to not export descriptor associated with SDP session. SDP session is expected to be managed as a whole. exporting part of it can lead to condition when locally cached descriptor can be different from one used in SDP session. i can understand desire to know which local BD_ADDR is used for SDP session, so, may be, add a call for that?

please read my other comment/question about device name. perhaps we can avoid adding this code completely and use some unique string that is not even exposed to user

128

i think, libbluetooth would be fine for it. we could probably add hci.c there to include commonly used HCI commands (such as reading device name).

292

ok thanks. could you please tell if there is a requirement for this string to be unique? bluetooth devices names, in my personal experience, tend to be fairly generic. they are also easily changeable at any point. for example, device name can be changed after it was put into configuration file. i guess my question is: is there any benefit from requesting device name and exposing it in configuration? is it possible to use some soft of templated name, i.e. "Mouse/Keyboard/Device BD_ADDR" or something like that?

[emax]

lib/libsdp/session.c

187 ↗

(On Diff #36934)

can you also please check ss for being not NULL?

[wulf]

1. SDP-session descriptor exporter replaced with SDP-session local BD_ADDR exporter
2. remote device name query helper has been moved to libbluetooth
3. libsdp & libbluetooth manpages have been updated to reflect changes.
4. Evdev device name format changed to "{REMOTE_NAME}, bdaddr {BD_ADDR}"

[wulf]

lib/libsdp/session.c

187 ↗

(On Diff #36934)

NULL check added

usr.sbin/bluetooth/bthidcontrol/sdp.c

52

descriptor exporter is replaced with local BD_ADDR exporter, sdp_get_lcaddr()

128

Function has been moved to lib/libbluetooth/hci.c and renamed to bt_devremote_name() to be more consistent with other libbluetooth function names

292

At low level (kernel/libinput/xorg-server), evdev device names could be an arbitrary strings e.g. non-unique, templated and so on. But at high level e.g. shell-scripts it`s preferable to have them unique as trully unique identifiers like IDs, cdev paths or serial numbers are often lost somewhere in tools output or could be unstable between reboots. So I have added remote device name & BD_ADDR concatenation to this patch version. Something like USB device drivers do.

Also, it`s possible to use templated names, moreover it is a fallback option of current implementation in a cases of name record absence in bthidd.conf or failing of HCI_Remote_Name query. But personally I prefer to see "user-friendly" name rather than templated despite it takes several extra seconds to make HCI query.

[wblock]

lib/libbluetooth/bluetooth.3

606 ↗

(On Diff #38580)

I think this might be clearer by putting the value in parentheses:

If not specified (NULL),

so the whole thing is:

If not specified (NULL), the first available device is used.

607 ↗

(On Diff #38580)

the first available device is used.

610 ↗

(On Diff #38580)

Articles needed:

parameter specifies the Bluetooth BD_ADDR of the remote device to query.

613 ↗

(On Diff #38580)

Spelling:

parameter specifies response timeout in seconds.

Please install textproc/igor and run igor -R lib/libbluetooth/bluetooth.3 | less -RS to find errors like these.

614 ↗

(On Diff #38580)

It is preferable to avoid Latin constructs when a small aside will work. Also needs an article and current tense:

If not specified (0), the default value is taken from

615 ↗

(On Diff #38580)

This should not be in quotes. Markup might not be needed at all, but it does need an article:

the net.bluetooth.hci.command_timeout

621 ↗

(On Diff #38580)

Missing "and":

and
.Fa ps_mode

622 ↗

(On Diff #38580)

Use the serial comma:

parameters specify Clock_Offset, Page_Scan_Repetition_Mode, and Page_Scan_Mode

624 ↗

(On Diff #38580)

On success, the function returns a pointer to a dynamically allocated

627 ↗

(On Diff #38580)

Don't need "has" here.

628 ↗

(On Diff #38580)

Don't think "of the function" is needed here. Also needs an article, and can remove some redundancy from the lines after it:

It is up to the caller to release the returned string using
.Xr free 3 .

(Remove "call".)

639 ↗

(On Diff #38580)

Use the serial comma:

.Fa ps_rep_mode ,

lib/libsdp/sdp.3

195 ↗

(On Diff #38580)

This needs articles, but the meaning is unclear. Maybe:

function returns the SDP session actual source BD_ADDR.

205 ↗

(On Diff #38580)

Needs articles:

function is useful if the current SDP session has been opened with the

207 ↗

(On Diff #38580)

Needs an article:

value passed as a source address.

419 ↗

(On Diff #38580)

Not part of this diff, I know. Still needs a serial comma, as per https://www.freebsd.org/doc/en_US.ISO8859-1/books/fdp-primer/writing-style-guidelines.html:

.Fn sdp_unregister_service ,

usr.sbin/bluetooth/bthidd/bthidd.8

88

The dash does not do much. Better would be a reference:

Requires evdev and uinput drivers to be loaded with
.Xr kldload 8
or compiled into the kernel.

[wulf]

Apply manpage fixes by Warren Block

[wulf]

ping...

[emax]

i think it looks good. i still question the whole idea of querying and putting remote device name in the configuration file, but, ultimately, this is not a hill I want to die on :)