Page MenuHomeFreeBSD
Differential D3702

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

Authored by erdgeist_erdgeist.org on Sep 21 2015, 1:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 28, 9:43 PM
Unknown Object (File)
Feb 23 2024, 4:24 AM
Unknown Object (File)
Feb 23 2024, 4:24 AM
Unknown Object (File)
Feb 23 2024, 4:24 AM
Unknown Object (File)
Feb 23 2024, 4:24 AM
Unknown Object (File)
Feb 23 2024, 4:24 AM
Unknown Object (File)
Jan 13 2024, 10:07 PM
Unknown Object (File)
Dec 20 2023, 4:05 AM
View All Files
Subscribers
imp
pi
wblock
wulf

Details

Reviewers
emax
bapt
Commits
rS322440: bthidd(8): Add support for Apple's magic mouse
rS322439: bthidd(8): Add support for vendor_id, product_id and version.
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.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

erdgeist_erdgeist.org updated this revision to Diff 8855.Sep 21 2015, 1:30 AM
erdgeist_erdgeist.org retitled this revision from to Add support for vendor_id, product_id and version in bthidd. Adds support for Apple's magic mouse.
erdgeist_erdgeist.org updated this object.
erdgeist_erdgeist.org edited the test plan for this revision. (Show Details)
erdgeist_erdgeist.org added a reviewer: emax.
erdgeist_erdgeist.org set the repository for this revision to rS FreeBSD src repository - subversion.
Herald added a subscriber: imp. · View Herald TranscriptSep 21 2015, 1:30 AM
erdgeist_erdgeist.org updated this revision to Diff 8856.Sep 21 2015, 2:08 AM

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

erdgeist_erdgeist.org updated this revision to Diff 8898.Sep 22 2015, 8:48 PM

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

emax edited edge metadata.Sep 22 2015, 11:37 PM

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

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

427

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

430

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

443

same as above. why use comma?

498

comma again

erdgeist_erdgeist.org added a comment.Sep 23 2015, 3:08 AM

Uploaded new version and replied to comments.

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

Yep. Fixed.

427

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.

430

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

443

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.

498

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

erdgeist_erdgeist.org updated this revision to Diff 8902.Sep 23 2015, 3:11 AM
erdgeist_erdgeist.org edited edge metadata.
erdgeist_erdgeist.org removed rS FreeBSD src repository - subversion as the repository for this revision.

Fixing diff to incorporate review by emax

emax accepted this revision.Sep 24 2015, 8:22 PM
emax edited edge metadata.
This revision is now accepted and ready to land.Sep 24 2015, 8:22 PM
erdgeist_erdgeist.org updated this revision to Diff 8934.Sep 25 2015, 6:46 PM
erdgeist_erdgeist.org edited edge metadata.
erdgeist_erdgeist.org set the repository for this revision to rS FreeBSD src repository - subversion.

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

This revision now requires review to proceed.Sep 25 2015, 6:46 PM
emax accepted this revision.Sep 27 2015, 1:20 PM
emax edited edge metadata.
This revision is now accepted and ready to land.Sep 27 2015, 1:20 PM
wblock added a subscriber: wblock.Apr 5 2016, 2:46 AM
wblock added a comment.Apr 21 2016, 2:02 AM

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

pi added a subscriber: pi.Jul 14 2017, 6:41 PM
wulf added a subscriber: wulf.Jul 30 2017, 11:20 AM

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 added a comment.Jul 30 2017, 5:10 PM
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 added a comment.Jul 30 2017, 10:03 PM
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.

  1. 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

  1. 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

  1. 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 added a comment.Jul 30 2017, 10:10 PM

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.

bapt accepted this revision.Aug 9 2017, 9:32 AM
Closed by commit rS322439: bthidd(8): Add support for vendor_id, product_id and version. (authored by wulf). · Explain WhyAug 12 2017, 9:21 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
usr/
src/
usr.sbin/
bluetooth/
bthidcontrol/
81 lines
bthidd/
3 lines
2 lines
6 lines
131 lines
16 lines
31 lines
4 lines
2 lines

Diff 8902

View Options

/usr/src/usr.sbin/bluetooth/bthidcontrol/sdp.c

Loading...
View Options

/usr/src/usr.sbin/bluetooth/bthidd/bthid_config.h

Loading...
View Options

/usr/src/usr.sbin/bluetooth/bthidd/bthidd.h

Loading...
View Options

/usr/src/usr.sbin/bluetooth/bthidd/bthidd.conf.sample

Loading...
View Options

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

Loading...
View Options

/usr/src/usr.sbin/bluetooth/bthidd/lexer.l

Loading...
View Options

/usr/src/usr.sbin/bluetooth/bthidd/parser.y

Loading...
View Options

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

Loading...
View Options

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

Loading...