Page MenuHomeFreeBSD
Differential D55472

appleir: Add Apple IR receiver driver
Needs ReviewPublic
Actions

Authored by guest-seuros on Mon, Feb 23, 10:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 19, 3:44 PM
Unknown Object (File)
Thu, Mar 19, 7:28 AM
Unknown Object (File)
Thu, Mar 19, 6:44 AM
Unknown Object (File)
Wed, Mar 18, 5:31 AM
Unknown Object (File)
Wed, Mar 18, 3:43 AM
Unknown Object (File)
Wed, Mar 18, 12:27 AM
Unknown Object (File)
Tue, Mar 17, 3:03 AM
Unknown Object (File)
Tue, Mar 17, 3:02 AM
View All 26 Files
Subscribers
imp
ziaee

Details

Reviewers
obiwac
ngie
Summary

HID driver for Apple IR receivers (USB HID, vendor 0x05ac).
Supports Apple Remote and generic IR remotes using NEC protocol.

Supported hardware:

  • Apple IR Receiver (0x8240, 0x8241, 0x8242, 0x8243, 0x1440)

Apple Remote protocol (proprietary 5-byte HID reports):

  • Key down/repeat/battery-low detection
  • 17-key mapping with two-packet command support
  • Synthesized key-up via 125ms callout timer

Generic IR remotes (NEC protocol):

Output via evdev with standard KEY_* codes.
Raw HID access available at /dev/hidraw0 for custom remapping.

Based on protocol reverse-engineering by James McKenzie et al.
Reference: drivers/hid/hid-appleir.c (Linux)

Tested on Mac Mini 2011 (0x05ac:0x8242).

Test Plan

This driver works with both NEC protocol and apple remotes.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 71379
Build 68262: arc lint + arc unit

Event Timeline

guest-seuros created this revision.Mon, Feb 23, 10:02 PM
Herald added subscribers: ziaee, imp. · View Herald TranscriptMon, Feb 23, 10:02 PM
guest-seuros requested review of this revision.Mon, Feb 23, 10:02 PM
Harbormaster completed remote builds in B70937: Diff 172550.Mon, Feb 23, 10:02 PM
guest-seuros updated this revision to Diff 172551.Mon, Feb 23, 10:10 PM

Remove debug DPRINTFN calls and verbose NEC code comments

Harbormaster completed remote builds in B70938: Diff 172551.Mon, Feb 23, 10:10 PM
guest-seuros edited the test plan for this revision. (Show Details)Mon, Feb 23, 10:26 PM
guest-seuros added a reviewer: obiwac.
guest-seuros added a comment.Mon, Feb 23, 10:36 PM

This driver was supposed to be a port form Linux Apple IR driver.

However, during development I discovered that the remote available for testing was not a genuine Apple remote but a generic NEC-compatible device with a apple logo on it.

As a result, the implementation was extended to support both the Apple protocol and generic NEC protocol devices. FreeBSD now supports both. (i tested it the the genuine remote)

obiwac added a comment.Tue, Feb 24, 12:25 AM

haven't done a hugely in-depth review yet. Overall looks pretty good!

general question though: is this driver not applicable to other IR receivers? in which case maybe a different name would be better. I mean if other generic NEC remotes exist with a different format to the Apple one, presumably that would imply there are other IR receivers which pass on the same reports.

sys/dev/hid/appleir.c
7–9

you are duplicating this comment later on, so no need to have it here in the license header

140

please add space between ; and /*

158–167

I don't know if I love this way of doing things. Perhaps this function should simply return (data >> 1) & KEY_MASK and then you track if a second report is expected separately?

213

are you sure about this? do you have an example battery low report which contains valid/sensible subsequent remote_id and key_code values?

guest-seuros updated this revision to Diff 173043.Tue, Mar 3, 12:24 AM

Address review feedback and add hardware test results

  • Fix header: remove duplicate description, move #include <sys/cdefs.h> after comments, ASCII-only arrows
  • Battery low: log once (sc_batt_warned flag), do not process as keydown
  • Remove appleir_get_key() negative-index trick; track two-packet state inline
  • Add sc_twoco callout to expire stale two-packet state after 250ms
  • Fix repeat handling: validate repeat packet matches sc_current_key
  • Strip verbose debug comments from NEC section

Tested on Mac Mini 2011 (0x05ac:0x8242): KEY_ENTER two-packet command,
key repeat, and synthesized key-up all verified via /dev/input/event0.

Harbormaster completed remote builds in B71145: Diff 173043.Tue, Mar 3, 12:24 AM
ziaee added inline comments.Tue, Mar 3, 12:46 AM
share/man/man4/appleir.4
57

The hardware section appears verbatim in the hardware release notes, so it needs context.

guest-seuros added a reviewer: ngie.Wed, Mar 4, 12:15 AM
ngie added a comment.Thu, Mar 5, 1:24 AM

Out of curiosity, how much of the Linux driver did you use as a reference for this driver? I'm concerned about licensing of the new code.

sys/dev/hid/appleir.c
118–120
imp added a comment.Thu, Mar 5, 1:33 AM
In D55472#1273784, @ngie wrote:

Out of curiosity, how much of the Linux driver did you use as a reference for this driver? I'm concerned about licensing of the new code.

How much was copied is the controlling question. Please include a path in the linux tree or git url.

guest-seuros added a comment.Thu, Mar 5, 2:28 AM

The reverse engineering of the Apple IR protocol was done by James McKenzie in the Linux driver 2 decades ago.
I referenced the Linux files in the original message and in the code (see line 14).

My work here is the FreeBSD driver side is implement probe, attach / detach, build the driver skeleton

During testing I first used the wrong remote.
With a generic NEC-compatible remote the driver seemed to work.

When I compared the behavior with the Linux implementation, I saw that the key mapping was different.

After comparing both drivers, I found that Apple remotes need a special handling in the mapping.
So I ported the Apple remote quirk from the Linux driver to fix the mapping.

If the Apple reverse-engineering part is considered problematic, it can be removed.
The IR receiver still works with standard NEC remotes, and basic IR functionality remains. (Linux Driver don't support that)

The Apple protocol only adds extra features such as battery reporting and Apple-specific button which is apple hardware specific.

sys/dev/hid/appleir.c
7–9

Thanks.

guest-seuros updated this revision to Diff 173546.Wed, Mar 11, 3:27 AM

Address review: clarify licensing, rewrite HARDWARE section with USB IDs

  • License header: explicitly state protocol constants are factual hardware data (not copied code), clarify Linux reference is GPL-2.0 with no code copied
  • man page HARDWARE: replace machine model list with USB product ID table; the previous list appeared verbatim in release notes without context
  • battery-low: already handled correctly (log once + ignore); the report byte[4] is 0x00 when battery-low so no valid key_code to process
Harbormaster completed remote builds in B71329: Diff 173546.Wed, Mar 11, 3:27 AM
guest-seuros updated this revision to Diff 173549.Wed, Mar 11, 3:49 AM

Replace static product ID table with dynamic report descriptor detection: match any Apple USB HID device with a 5-byte input report

Harbormaster completed remote builds in B71331: Diff 173549.Wed, Mar 11, 3:49 AM
guest-seuros updated this revision to Diff 173550.Wed, Mar 11, 3:56 AM

Revert dynamic detection: Thunderbolt Display Audio (0x05ac:0x1107) has a 5-byte HID input report and caused a false positive on Mac Mini 2011 test hardware. Restore static product ID table.

Harbormaster completed remote builds in B71332: Diff 173550.Wed, Mar 11, 3:56 AM
ziaee added a comment.Wed, Mar 11, 2:31 PM

Hardware section is looking good! Consider sorting them in reverse order, so that as it gets longer over time, the current stuff that people are more likely to be able to source is at the top.

share/man/man4/appleir.4
36–41

No need to duplicate this information, it bloats the manual and will make it harder to maintain later :)

73

Macros inside list width arguments is unspecified behavior, sometimes rendering as -1 columns. Manpages have a ton of consumers.

84–85

The Lk macro accepts an argument to do this.

guest-seuros updated this revision to Diff 173640.EditedFri, Mar 13, 12:20 AM

Address review: remove duplicate device list from DESCRIPTION, fix .Bl width arg (no macros), use .Lk with description argument, add space between ; and /*, document byte[4]=0x00 on battery-low, sort HARDWARE newest first

Apple has deprecated IR receivers in favor of wireless. no new hardware will require this driver.

Harbormaster completed remote builds in B71379: Diff 173640.Fri, Mar 13, 12:20 AM
ngie added inline comments.Fri, Mar 13, 4:44 PM
sys/dev/hid/appleir.c
14
16

Thank you for clarifying this point!

67–68

Is this defined or discussed anywhere else, and if so can it be mentioned here?

130–133

style(9): is the whitespace correct here (Phabricator sometimes goofs up when rendering hard tabs)?

153

If this is used as a sentinel, it might be a good idea to either put it at the top of the conditional block (if not reentrant friendly) or down at the bottom (if reentrant friendly)

238–239

Could you please add helpfully named constants for 0x26, et al, with corresponding comments?

257–258

This can fail. How should the driver respond to the failure?

268

This can fail. How should the driver respond to the failure?

291–292

This can fail. How should the driver respond to the failure?

368

Can this fail?

379–380

evdev_free(..) handles the NULL case gracefully for consistency with free(3), et al.

392–396

These calls can all fail at various points in the driver lifecycle. I would check the result and return an appropriate error if some of these calls fail (otherwise the driver/device could be left in a non-determinate state at detach).

sys/modules/hid/appleir/Makefile
1–2

We don't add $FreeBSD$ to files anymore.

ngie added a comment.Fri, Mar 13, 4:48 PM

Does building/installing this driver on all platforms (TARGET/TARGET_ARCH) make sense?

sys/dev/hid/appleir.c
193

Can you please add helpfully named constants with corresponding documentation (optional, but encouraged) for 0x25, et al?
I realize that you fundamentally did this, but in the event the comment gets scrambled, refactored, etc, having humanized names might help prevent context loss (this general comment applies for other areas where I asked about "magic constants").

sys/modules/hid/appleir/Makefile
7

Where is opt_kbd.h included?

ngie added a comment.Fri, Mar 13, 4:51 PM

Out of curiosity--since I no longer have any Apple IR remotes (my last one went away with my AppleTV back in the late 2010s), have you tried taking off the case and looking at the internal chips for identifying marks on any of the ASICs? I'm curious because it might help you figure out which NEC ASIC may or may not be compatible with this driver.

Revision Contents
Changeset List

PathSize
share/
man/
man4/
1 line
86 lines
sys/
conf/
1 line
dev/
hid/
419 lines
modules/
hid/
1 line
appleir/
10 lines

Diff 173640

View Options

share/man/man4/Makefile

Loading...
View Options

share/man/man4/appleir.4

Loading...
View Options

sys/conf/files

Loading...
View Options

sys/dev/hid/appleir.c

Loading...
View Options

sys/modules/hid/Makefile

Loading...
View Options

sys/modules/hid/appleir/Makefile

Loading...