Add support for vendor_id, product_id and version in bthidd. Adds support for Apple's magic mouse

[erdgeist_erdgeist.org]

Summary

The patch adds support for vendor_id, product_id and version to bthidcontrol and bthidd. This information is stored in bthidd.conf for later reference.

The patch enables bthidd to support middle mouse button and scroll wheel emulation on Apple's magic mouse. When the mouse connected, bthidd sends an undocumented hid report to the mouse to enable the mouse's special features.

The patch adds code to bthidd's hid interrupt handler, so that the undocumented reports sent by magic mice are properly decoded and fed into the sysmouse framework.

[erdgeist_erdgeist.org]

Fix: In sdp.c, int16_t vendor_id, product_id, version were not properly initialised.

[erdgeist_erdgeist.org]

Rename sdp attribute constants to match the SDP_ATTR_ naming convention already in place.

[emax]

i think it looks fine. i, personally, would try to avoid using "odd" c syntax, i.e. comma and prefix in/decrement where there is no good reason for it. i would also sprinkle some comments explaining where "magic" numbers came from. of course author of the original code should get the credit too. otherwise it looks good. thank you for your work.

/usr/src/usr.sbin/bluetooth/bthidd/hid.c

85 ↗

(On Diff #8898)

arguments to calloc() reversed, i.e. its number then size. does not really matter though. just being pedantic

422 ↗

(On Diff #8898)

what's up with prefix in/decrements? :) also why on one line?

425 ↗

(On Diff #8898)

magic numbers are magical :) perhaps a comment explaining where those came from?

438 ↗

(On Diff #8898)

same as above. why use comma?

493 ↗

(On Diff #8898)

comma again

[erdgeist_erdgeist.org]

Uploaded new version and replied to comments.

/usr/src/usr.sbin/bluetooth/bthidd/hid.c

85 ↗

(On Diff #8898)

Yep. Fixed.

422 ↗

(On Diff #8898)

Prefix reads more natural: "Increase data", "Increase len."

But if you insist ;) Fixed now.

Single line and comma operator, because it modifies the same logical unit { uint8_t * data, size_t len } describing the buffer. Order does not matter, so language conventions say: prefer comma operator.

425 ↗

(On Diff #8898)

Again. I was afraid that apple specific code takes too much space, already in sources. Fixed.

438 ↗

(On Diff #8898)

Same logical unit: The mouse_x += operation only is valid, if mevents is set.

Order does not matter, operating on the logical struct { int mouse_x, bool mevents_valid }, so comma operator is preferrable.

493 ↗

(On Diff #8898)

What do you have against comma operator? It is used as language conventions imply.

[erdgeist_erdgeist.org]

Fixing diff to incorporate review by emax

[erdgeist_erdgeist.org]

Last cleanup broke detection when increasing len variable instead of decreasing it.

[wblock]

This works for me on 11-current r298354. How about committing it?

[wulf]

IMO, Apple mouse support should be done in other way:

1. Write fake HID descriptor and place it either into bthidd.conf (in textual form) or into bthidctl (in binary form)
2. The descriptor will replace hardwire provided one on bthidctl run, so bthidd will be provided with proper one at start
3. Extend bthidd HID parser to support absolute usages and multitouch to avoid extra apple specific parser.
4. This will reduce size of "if (MAGIC_MOUSE(hid_device) && s->ctx) {}", make it HID-agnostic and provide good start point to write multitouch bt-touchpad/touchscreen support

[erdgeist_erdgeist.org]

In D3702#244252, @wulf wrote:

IMO, Apple mouse support should be done in other way:

1. Write fake HID descriptor and place it either into bthidd.conf (in textual form) or into bthidctl (in binary form)
2. The descriptor will replace hardwire provided one on bthidctl run, so bthidd will be provided with proper one at start
3. Extend bthidd HID parser to support absolute usages and multitouch to avoid extra apple specific parser.
4. This will reduce size of "if (MAGIC_MOUSE(hid_device) && s->ctx) {}", make it HID-agnostic and provide good start point to write multitouch bt-touchpad/touchscreen support

I am not sure, I am completely following here.

1. Who is supposed to provide the magic words to the device in your scenario? Still bthidd?
2. How does this daemon figure out that the device in question needs a special init message? Currently
3. Since apple touchpads speak HID only to a certain extent (i.e. in dumb mode), a special translation unit needs to be in place to capture and translate the un-announced HID packets when in smart mode, before bthidd dismisses them. Do you think there should be a pluggable system?
4. Normal hid touch devices use other reports, as specified in HID specs, so some sort of apple specific translation is necessary.

Did you follow the original thread at https://lists.freebsd.org/pipermail/freebsd-bluetooth/2015-August/001997.html and https://lists.freebsd.org/pipermail/freebsd-bluetooth/2015-September/002000.html ?

I'd be willing to implement code that is more generic to support other touch pads, though I don't have devices to test and I need more info on the architecture that you have in mind.

[wulf]

In D3702#244301, @erdgeist_erdgeist.org wrote:

In D3702#244252, @wulf wrote:

IMO, Apple mouse support should be done in other way:

1. Write fake HID descriptor and place it either into bthidd.conf (in textual form) or into bthidctl (in binary form)
2. The descriptor will replace hardwire provided one on bthidctl run, so bthidd will be provided with proper one at start
3. Extend bthidd HID parser to support absolute usages and multitouch to avoid extra apple specific parser.
4. This will reduce size of "if (MAGIC_MOUSE(hid_device) && s->ctx) {}", make it HID-agnostic and provide good start point to write multitouch bt-touchpad/touchscreen support

I am not sure, I am completely following here.

1. Who is supposed to provide the magic words to the device in your scenario? Still bthidd?

Yes! Bthidd can parse bthidd.conf to obtain vendor and device ids by device MAC address and then send magic words to device if they match predefined values. That is like USB quirk subsystem does.

2. How does this daemon figure out that the device in question needs a special init message? Currently

By vendor and device(product) ids stored in bthidd.conf for the device

3. Since apple touchpads speak HID only to a certain extent (i.e. in dumb mode), a special translation unit needs to be in place to capture and translate the un-announced HID packets when in smart mode, before bthidd dismisses them.

That should be done by providing of faked HID-descriptor. BT-report parser should take care of report length in this case as it is variable. OpenBSD fakes HID descriptor for some Microsoft USB gadgets.

Do you think there should be a pluggable system?

Not sure. IMO current state of FreeBSD bluetooth stack is too bad to spend ones time to make it pluggable. But I do not vote against improvements

4. Normal hid touch devices use other reports, as specified in HID specs, so some sort of apple specific translation is necessary.

Exactly! That is what your current patch proposes! Generalizing of a report parser will make this part simpler and shorter.

Did you follow the original thread at https://lists.freebsd.org/pipermail/freebsd-bluetooth/2015-August/001997.html and https://lists.freebsd.org/pipermail/freebsd-bluetooth/2015-September/002000.html ?

I see you was advised to provide fake HID descriptor too: https://lists.freebsd.org/pipermail/freebsd-bluetooth/2015-September/002005.html

I'd be willing to implement code that is more generic to support other touch pads, though I don't have devices to test and I need more info on the architecture that you have in mind.

I do not have such devices as well. That is why I vote for commiting this change as is if you have no ideas how to improve it.

[wulf]

Really, I have some ongoing work (evdev protocol support for bluetooth devices) that depends on this review. So if no one objects, I will commit it after some testing.