diff --git a/HARDWARE_TESTING.md b/HARDWARE_TESTING.md index 6250eb0d..4f5067fd 100644 --- a/HARDWARE_TESTING.md +++ b/HARDWARE_TESTING.md @@ -2131,9 +2131,11 @@ app touches first takes that path for the other two. ## 22. Guide button LED brightness The intensity the driver sends at `startDevice()` is now the user's setting rather than a -hardcoded `0x14`, mapped from four presets in `PreferenceConfiguration.getGuideButtonLedValue()`: +hardcoded `0x14`, mapped from the presets in `PreferenceConfiguration.getGuideButtonLedValue()`: off `0x00`, dim `0x0A`, normal `0x14`, bright `0x2F`. `normal` is what the driver sent before, so -an untouched install behaves exactly as it did. +an untouched install behaves exactly as it did. A fifth setting, "Battery level", starts at +`normal` and then moves the LED with each battery report — that is section 31's to verify; this +section covers the four fixed presets. Two of those values are assertions about hardware that nothing off a device can settle. @@ -2176,8 +2178,9 @@ next created — the next stream — not on re-plugging a pad. That is the exist constructor path is wired up, not just the adapter's. - [ ] **Both ABIs.** The change is in the shared driver, but the Shield is `arm64-v8a` and the Homatics `armeabi-v7a`, and only a run on each proves the JNI signature change - (`createDriver` is now `(II)J`) binds. A mismatch fails at library load, so the symptom would - be the adapter not starting at all rather than the LED misbehaving. + (`createDriver` is now `(II)J`, and since section 31 `setLedNative` is `(JII)Z`) binds. A + mismatch fails at library load, so the symptom would be the adapter not starting at all + rather than the LED misbehaving. --- @@ -2771,6 +2774,65 @@ it would say whether "never drop" was ever more than an accident of the PTS base --- +## 31. Guide LED following the battery + +"Battery level" is a fifth value of the Xbox button brightness setting. The guide LED is +monochrome, so following the battery means brightness steps, taken from the presets so each one is +a level the user has already seen: full → bright `0x2F`, medium → normal `0x14`, low → dim `0x0A`, +and critical → a **slow blink** (`LED_BLINK_SLOW`, `0x04`) at normal intensity, because a warning +has to look like one and a dimmer steady LED across a room does not. A pad with no battery fitted +(type 0, running off its cable) goes bright. Before the first report, and for any level the spec +does not define, the pad shows the normal start value. The ladder is `GuideButtonLed.forBattery()`, +on the Java side so it is tested; the native side only applies what it is handed and refuses a +pattern outside `enum LedMode`. + +The LED command is now built in one place, `Controller::applyLed()`: `startDevice()` applies +whatever is stored, and `Controller::setLed()` — the new `setLedNative` JNI entry — stores the +battery ladder's command and sends it if the device has started, or leaves it for `startDevice()` +if not, since a command sent in Arrival goes unanswered. + +Also fixed on the way: on the cabled path the read thread runs before the Java object exists, and a +status that landed in that window used to arm the driver's change filter with nobody listening, so +Java heard nothing until the battery actually moved — hours, on a full pad. The filter now stays +unarmed until a report is delivered; MS-GIPUSB 2.2.9 has the pad repeat status every second for its +first ten seconds, so the next one is at most a second away. + +Nothing here is on a per-frame path: one GIP frame per *level* change, on the driver's read thread, +deduplicated against charge-only changes. Like section 22, the setting is read once in +`UsbDriverService.onCreate()`, so the procedure is "change setting, leave the stream, start it +again". + +- [ ] **The LED steps down over a discharge.** With "Battery level" set, run a pad down: bright at + full, visibly dimmer at medium, dimmer again at low. Each step is one log line: + ```bash + adb shell setprop persist.log.tag '""' + adb logcat -d | grep -a "Guide button LED set to" + adb shell setprop persist.log.tag S + ``` + One line per level change, and none for a charge-state-only change (plug and unplug the + cable at a steady level): `GipController` deduplicates against the last command sent. +- [ ] **Critical actually blinks.** Table 42 marks every pattern except On as "not implemented by + host" — which describes Microsoft's host, not the pad — so whether a pad honours + `LED_BLINK_SLOW` is unverified, and xone's exposing the patterns through sysfs is inference + rather than proof. If the pad sits steady at `0x14` instead, or logs `Refused`/no ack, try + `BLINK_MED`, `BLINK_FAST` and the two `FADE`s in turn and record which of them reads as a + warning; only if none does, fall back to `PATTERN_ON` at `0x04` and note that critical then + reads like "dim". The spec also has hosts wait for two consecutive critical reports before + warning the user; the LED reacts to the first. +- [ ] **A battery-less cabled pad goes bright.** Type 0 in the status byte, whatever the level + field says (the spec has such a pad report critical in the level bits). Needs a pad with the + pack removed, on the wired GIP path (`wiredPadAudio` on). +- [ ] **A pad pairing mid-stream picks the ladder up.** Its first status after `startDevice()` + drives it; nothing about the setting is per-pad. +- [ ] **The cabled pad, wired GIP path.** Same ladder through `WiredController`, plus the + first-status fix above: the LED should reflect the pad within a couple of seconds of it being + claimed, not only after the battery next moves. +- [ ] **Both ABIs load.** `setLedNative` is a new `(JII)Z` entry in the controller's JNI table; a + mismatch fails at library load, so the symptom is the adapter or cabled pad not starting at + all rather than the LED misbehaving. + +--- + ## Hardware still needed | Needed for | Hardware | @@ -2798,6 +2860,7 @@ it would say whether "never drop" was ever more than an accident of the PTS base | §17.1 sub-50 fractional | A 23.976 or 29.97 Hz output mode specifically — 59.94 takes the other branch and cannot settle it | | §18.2 two-client check | A second Moonlight client against the same host | | §30 debug boxes | Shield: a debug build in min-latency for the presented counter, then cap-FPS for the gating check. Homatics: any build in min-latency. The mechanism itself is verified on the Shield's release build | +| §31 ladder | A rechargeable adapter pad run down to critical, a play-and-charge cable for the charge-only case, and a cabled pad with its battery pack removed for the type-0 case | | §19 | Both target devices; the survey differs per SoC | | §26 badge centring | Either box, on a television at normal viewing distance | | §20 | The Homatics specifically — the Shield cannot verify a change scoped to `armeabi-v7a` | diff --git a/app/src/main/java/com/limelight/binding/input/driver/GipController.java b/app/src/main/java/com/limelight/binding/input/driver/GipController.java index 74c8da49..2a92079f 100644 --- a/app/src/main/java/com/limelight/binding/input/driver/GipController.java +++ b/app/src/main/java/com/limelight/binding/input/driver/GipController.java @@ -23,10 +23,24 @@ public class GipController extends AbstractController{ // Native controller instance owned by the driver, valid until the driver removes it private final long handle; - /** @param handle native controller instance supplied by the dongle's driver */ - public GipController(int deviceId, UsbDriverListener listener, int vendorId, int productId, long handle) { + // The user's guide LED choice, and the last command sent for it. Seeded with what + // startDevice() sends so a first battery report that maps to the same thing is not sent + // twice. Touched only on the driver's read thread - updateBattery() is the sole writer - so + // it needs no synchronisation. + private final GuideButtonLed guideButtonLed; + private GuideButtonLed.State ledSent; + + /** + * @param handle native controller instance supplied by the dongle's driver + * @param guideButtonLed the guide LED choice; its starting intensity has already gone to the + * native side with the transport, this is for what follows + */ + public GipController(int deviceId, UsbDriverListener listener, int vendorId, int productId, + long handle, GuideButtonLed guideButtonLed) { super(deviceId, listener, vendorId, productId); this.handle = handle; + this.guideButtonLed = guideButtonLed; + this.ledSent = guideButtonLed.startState(); // An Xbox pad however it is attached, so it declares what every other Xbox pad does. It // extends AbstractController rather than AbstractXboxController because the transport below @@ -89,9 +103,9 @@ public void updateInput(int buttons,short triggerLeft, short triggerRight, } /** - * Called from the native driver when the controller's battery state changes, with the raw GIP - * values. The mapping lives here rather than in the driver because the constants it maps onto - * are Moonlight's. + * Called from the native driver, on its read thread, when the controller's battery state + * changes, with the raw GIP values. The mapping lives here rather than in the driver because + * the constants it maps onto are Moonlight's. * *

The status byte packs four fields (MS-GIPUSB Table 30). Type says what kind of battery is * fitted, not whether it is charging — that is a separate field, which xow's struct @@ -99,11 +113,16 @@ public void updateInput(int buttons,short triggerLeft, short triggerRight, * battery at all: the pad is running off the cable. xow's enum calls that value * {@code BATT_TYPE_CHARGING}, which is a misnomer the spec settles. * + *

Also where the guide LED follows the battery, when the setting says so — see + * {@link #followBatteryLed}. + * * @param type GIP battery type: 0 absent or bus powered, 1 standard, 2 rechargeable * @param level GIP battery level, 0 to 3, meaningless when {@code type} is 0 * @param charge GIP charge state: 0 not charging, 1 charging, 2 charge error */ public void updateBattery(byte type, byte level, byte charge) { + followBatteryLed(type, level); + // No battery fitted: the pad is running off the cable, and its level means nothing if (type == 0) { reportBattery(MoonBridge.LI_BATTERY_STATE_NOT_CHARGING, @@ -135,6 +154,33 @@ public void updateBattery(byte type, byte level, byte charge) { reportBattery(state, percentage); } + /** + * Steps the guide LED with the battery, when the user's choice is to follow it. + * + *

Deduplicated against the last command sent because the driver reports on any change in + * the status byte - charge state and power level included - and only the level and the + * battery type move the LED. A send the pad refused is not recorded, so the next report + * retries it. + * + *

Runs inside the battery callback, on the driver's read thread: the same thread + * {@code startDevice()} sends the first LED command from, so this adds no new cross-thread + * sender to the native side. + */ + private void followBatteryLed(byte type, byte level) { + if (!guideButtonLed.followsBattery()) { + return; + } + + var next = GuideButtonLed.forBattery(type, level); + if (next.equals(ledSent)) { + return; + } + + if (setLedNative(handle, next.pattern(), next.intensity())) { + ledSent = next; + } + } + /** * @return true if this pad told us, in its metadata, that it can render 48 kHz stereo. A pad * that did not has no audio endpoint reachable this way, and enabling would move the @@ -239,5 +285,14 @@ public void forgetAudioDevice() { native void queueAudioNative(long handle, short[] samples, int count); native void sendRumble(long handle, short lowFreqMotor, short highFreqMotor); native void sendrumbleTriggers(long handle, short leftTrigger, short rightTrigger); + /** + * Sends a guide LED command, or stores it for {@code startDevice()} if the pad is not started + * yet. + * + * @param pattern one of {@code GuideButtonLed.PATTERN_*}; anything else is refused + * @param brightness 0 to {@code GuideButtonLed.INTENSITY_MAX}; clamped natively + * @return whether the pad took it, or it was stored to be sent at start + */ + native boolean setLedNative(long handle, int pattern, int brightness); } diff --git a/app/src/main/java/com/limelight/binding/input/driver/GuideButtonLed.java b/app/src/main/java/com/limelight/binding/input/driver/GuideButtonLed.java new file mode 100644 index 00000000..92b27701 --- /dev/null +++ b/app/src/main/java/com/limelight/binding/input/driver/GuideButtonLed.java @@ -0,0 +1,96 @@ +package com.limelight.binding.input.driver; + +/** + * The user's guide button LED choice for the Xbox pads this app drives, and the ladder that turns + * a battery level into an LED command when the choice is to follow the battery. + * + *

Every LED number on the Java side lives here. The values are the protocol's own, not + * percentages of anything we chose: MS-GIPUSB 3.1.5.5.7 Table 41 defines the LED command's + * intensity byte as 0 to 47 ({@code 0x2F}), and Table 42 its pattern byte. Keeping the whole + * translation on this side of the JNI boundary keeps it testable; the native side only applies + * what it is handed, and rejects a pattern it does not know. + * + *

Split out of {@link GipController}, which loads a native library in its static initialiser + * and so cannot be touched from a JVM test, following {@link StickCalibration}. Android-free. + * + *

Latency: none. Read once per pad at start and once per battery report, on the driver's own + * thread; never on the frame or input path. + * + * @param intensity what the pad is told at start, Table 41 units + * @param followsBattery whether battery reports move it after that + */ +public record GuideButtonLed(int intensity, boolean followsBattery) { + + // Table 41 intensities. The presets, and the ceiling. xow believed the maximum was 0x20, so + // whether a pad honours anything above that is an open hardware question - see + // HARDWARE_TESTING.md 22. + public static final int INTENSITY_OFF = 0x00; + public static final int INTENSITY_DIM = 0x0A; + public static final int INTENSITY_NORMAL = 0x14; + public static final int INTENSITY_BRIGHT = 0x2F; + public static final int INTENSITY_MAX = 0x2F; + + // Table 42 patterns. These mirror enum LedMode in app/src/main/jni/xow_driver/controller/gip.h + // and must stay equal to it: the native side accepts exactly this set and refuses anything + // else, which is what catches the two drifting apart. + public static final int PATTERN_OFF = 0x00; + public static final int PATTERN_ON = 0x01; + public static final int PATTERN_BLINK_FAST = 0x02; + public static final int PATTERN_BLINK_MED = 0x03; + public static final int PATTERN_BLINK_SLOW = 0x04; + public static final int PATTERN_FADE_SLOW = 0x08; + public static final int PATTERN_FADE_FAST = 0x09; + + /** What the driver sent before the setting existed, and what anything unrecognised gets. */ + public static final GuideButtonLed NORMAL = new GuideButtonLed(INTENSITY_NORMAL, false); + + /** + * One LED command's worth: what the pad is told to show. + * + * @param pattern one of {@code PATTERN_*} + * @param intensity 0 to {@link #INTENSITY_MAX} + */ + public record State(int pattern, int intensity) { + } + + /** + * @return what the driver sends when the pad starts. Pattern and intensity are separate + * fields (Table 42), so an intensity of zero on a lit pattern is not what the spec + * means by off: zero selects the off pattern rather than a maximally dimmed on one. + */ + public State startState() { + return new State(intensity == 0 ? PATTERN_OFF : PATTERN_ON, intensity); + } + + /** + * The ladder: the LED for a battery report, when the setting is to follow it. + * + *

The steps are the presets, so each one is a brightness the user has already seen on the + * other settings: full is Bright, medium is Normal, low is Dim. Critical flashes rather than + * dimming further, because a dim LED across a room is indistinguishable from the Dim preset + * and a warning has to look like one; it flashes at Normal so the flash is visible. A pad with + * no battery fitted is running off its cable, and gets full brightness for the same reason a + * full battery does. + * + *

Only critical uses a pattern other than On. Table 42 marks every other pattern "not + * implemented by host", which describes Microsoft's host rather than the pad, so whether a + * pad honours the slow blink is a hardware question - HARDWARE_TESTING.md 31. + * + * @param type GIP battery type: 0 absent or bus powered, 1 standard, 2 rechargeable + * @param level GIP battery level, 0 critical to 3 full; meaningless when {@code type} is 0 + * @return the command to send; a level the spec does not define gets what the pad was + * started with, so an unexpected value never changes anything + */ + public static State forBattery(byte type, byte level) { + if (type == 0) { + return new State(PATTERN_ON, INTENSITY_BRIGHT); + } + return switch (level) { + case 3 -> new State(PATTERN_ON, INTENSITY_BRIGHT); + case 2 -> new State(PATTERN_ON, INTENSITY_NORMAL); + case 1 -> new State(PATTERN_ON, INTENSITY_DIM); + case 0 -> new State(PATTERN_BLINK_SLOW, INTENSITY_NORMAL); + default -> NORMAL.startState(); + }; + } +} diff --git a/app/src/main/java/com/limelight/binding/input/driver/XboxWiredGipController.java b/app/src/main/java/com/limelight/binding/input/driver/XboxWiredGipController.java index 0b5ae433..f0898fb2 100644 --- a/app/src/main/java/com/limelight/binding/input/driver/XboxWiredGipController.java +++ b/app/src/main/java/com/limelight/binding/input/driver/XboxWiredGipController.java @@ -63,8 +63,10 @@ private static UsbInterface findGipInterface(UsbDevice device) { private XboxWiredGipController(UsbDevice device, UsbDeviceConnection connection, UsbInterface gipInterface, int deviceId, - UsbDriverListener listener, long wiredHandle, long gipHandle) { - super(deviceId, listener, device.getVendorId(), device.getProductId(), gipHandle); + UsbDriverListener listener, long wiredHandle, long gipHandle, + GuideButtonLed guideButtonLed) { + super(deviceId, listener, device.getVendorId(), device.getProductId(), gipHandle, + guideButtonLed); this.wiredHandle = wiredHandle; this.connection = connection; @@ -74,16 +76,17 @@ private XboxWiredGipController(UsbDevice device, UsbDeviceConnection connection, /** * Claims the pad and brings the GIP stack up on it. * - * @param ledBrightness guide button LED intensity, as the raw protocol value. Passed at - * transport creation rather than set afterwards because the native side - * sends the LED command from {@code startDevice()}, which can run before - * this object exists — see {@link XboxWirelessDongle}. + * @param guideButtonLed guide button LED choice. Its starting intensity is passed at transport + * creation rather than set afterwards because the native side sends the + * LED command from {@code startDevice()}, which can run before this + * object exists — see {@link XboxWirelessDongle}. The battery moves it + * from here on, if the choice says so. * @return the controller, or null if the device could not be claimed — in which case nothing * has been left running and the caller still owns {@code connection} */ public static XboxWiredGipController create(UsbDevice device, UsbDeviceConnection connection, int deviceId, UsbDriverListener listener, - int ledBrightness) { + GuideButtonLed guideButtonLed) { /* * Claimed here rather than natively, and forced. Android's own driver holds this interface @@ -103,7 +106,7 @@ public static XboxWiredGipController create(UsbDevice device, UsbDeviceConnectio return null; } - long wired = createWiredDriver(connection.getFileDescriptor(), ledBrightness); + long wired = createWiredDriver(connection.getFileDescriptor(), guideButtonLed.intensity()); if (wired == 0) { LimeLog.warning("Wired GIP: could not create the native transport"); connection.releaseInterface(gipInterface); @@ -128,7 +131,7 @@ public static XboxWiredGipController create(UsbDevice device, UsbDeviceConnectio } return new XboxWiredGipController(device, connection, gipInterface, deviceId, listener, - wired, gip); + wired, gip, guideButtonLed); } /** diff --git a/app/src/main/java/com/limelight/binding/input/driver/XboxWirelessDongle.java b/app/src/main/java/com/limelight/binding/input/driver/XboxWirelessDongle.java index a7e42059..a7fa8395 100644 --- a/app/src/main/java/com/limelight/binding/input/driver/XboxWirelessDongle.java +++ b/app/src/main/java/com/limelight/binding/input/driver/XboxWirelessDongle.java @@ -33,14 +33,16 @@ public class XboxWirelessDongle { private long driverHandle; /* - * Guide button LED intensity handed to every pad this adapter brings up. + * Guide button LED choice handed to every pad this adapter brings up. * - * Held here, and passed at driver creation rather than set on each pad afterwards, because the - * native side sends the LED command from startDevice() as soon as a pad's metadata arrives - - * which can be before the Java GipController for that pad exists. A setter called after - * construction would race that first packet and lose on a fast pairing. + * The starting intensity is passed at driver creation rather than set on each pad afterwards, + * because the native side sends the LED command from startDevice() as soon as a pad's metadata + * arrives - which can be before the Java GipController for that pad exists, so a setter called + * after construction would race that first packet and lose on a fast pairing. Whether the + * battery moves it after that is the pad's own business: each GipController gets the whole + * choice and drives its LED from its battery reports. */ - private final int ledBrightness; + private final GuideButtonLed guideButtonLed; // Live controllers by the native driver's slot index private Map controllers = new HashMap<>(); @@ -57,17 +59,17 @@ public class XboxWirelessDongle { } /** - * @param connection an already-open connection, whose file descriptor is handed to the native driver - * @param ledBrightness guide button LED intensity for this adapter's pads, as the raw protocol - * value — see {@code PreferenceConfiguration.getGuideButtonLedValue} + * @param connection an already-open connection, whose file descriptor is handed to the native driver + * @param guideButtonLed guide button LED choice for this adapter's pads — see + * {@code PreferenceConfiguration.getGuideButtonLedValue} */ public XboxWirelessDongle(UsbDevice device, UsbDeviceConnection connection, - UsbDriverListener listener, int ledBrightness) { + UsbDriverListener listener, GuideButtonLed guideButtonLed) { this.device = device; this.connection = connection; this.listener = listener; this.driverHandle = -1; - this.ledBrightness = ledBrightness; + this.guideButtonLed = guideButtonLed; } /** @@ -84,7 +86,7 @@ public synchronized boolean start() { if(this.driverHandle != -1) { return false; //we already started; } - this.driverHandle = createDriver(connection.getFileDescriptor(), ledBrightness); + this.driverHandle = createDriver(connection.getFileDescriptor(), guideButtonLed.intensity()); boolean ok = startDriver(this.driverHandle, ""); if(!ok) { LimeLog.info("xbox wireless dongle driver failed to start"); @@ -168,7 +170,7 @@ public static boolean canClaimDevice(UsbDevice device) { public void addNewController(int id, long handle, short vid, short pid, long address){ // Namespaced by vendor ID so pad numbers can't collide with other drivers' device IDs var controller = new GipController(numberFor(address) + 0x045e0000, - listener, vid, pid, handle); + listener, vid, pid, handle, guideButtonLed); controllers.put(id, controller); this.listener.deviceAdded(controller); } diff --git a/app/src/main/java/com/limelight/preferences/PreferenceConfiguration.java b/app/src/main/java/com/limelight/preferences/PreferenceConfiguration.java index 4036a0f6..20a17a58 100644 --- a/app/src/main/java/com/limelight/preferences/PreferenceConfiguration.java +++ b/app/src/main/java/com/limelight/preferences/PreferenceConfiguration.java @@ -6,6 +6,7 @@ import java.util.Locale; +import com.limelight.binding.input.driver.GuideButtonLed; import com.limelight.nvstream.StreamConfiguration; import com.limelight.nvstream.jni.MoonBridge; @@ -162,10 +163,13 @@ public enum AnalogStickForScrolling { /** Whether to drive cabled Xbox pads ourselves so their headphone jack can take audio. */ public boolean wiredPadAudio; /** - * Guide button LED intensity for Xbox pads this app drives, as the raw protocol value the - * pad expects. Only reaches a pad the GIP driver owns — see {@code UsbDriverService}. + * Guide button LED choice for Xbox pads this app drives: the intensity to start at, in the + * pad's own units, and whether the battery moves it after that. Only reaches a pad the GIP + * driver owns — see {@code UsbDriverService}. The type lives in the driver package because its + * numbers are the GIP protocol's; this package importing it is the only dependency between + * the two, and the record has no static initialiser, so nothing loads that should not. */ - public int guideButtonLed; + public GuideButtonLed guideButtonLed; public boolean mouseEmulation; public AnalogStickForScrolling analogStickForScrolling; public boolean mouseNavButtons; @@ -366,34 +370,37 @@ static int getEncryptionFlagsValue(String value) { } /** - * Maps the stored preference value to the guide button LED intensity the pad expects. + * Maps the stored preference value to the guide button LED choice the driver applies. * - *

The returned number is the protocol's own field, not a percentage of anything we chose: + *

The intensity is the protocol's own field, not a percentage of anything we chose: * MS-GIPUSB 3.1.5.5.7 Table 41 defines the LED command's third payload byte as an intensity * of 0 to 47 ({@code 0x2F}). Mapping presets to that value here, rather than in the driver, * keeps the whole translation on the testable side of the JNI boundary — the native side - * only applies what it is handed. + * only applies what it is handed, pattern included, and {@link GuideButtonLed} holds the + * numbers. + * + *

"battery" starts at Normal, because the pad has not said what it has yet, and lets each + * battery report move it from there. * *

Split out and static so it can be unit tested, following * {@link #getEncryptionFlagsValue(String)}. * * @param value the stored string, or null if the preference has never been written - * @return the intensity to send, defaulting to the value the driver used before this setting + * @return the choice to apply, defaulting to the value the driver used before this setting * existed */ - static int getGuideButtonLedValue(String value) { + static GuideButtonLed getGuideButtonLedValue(String value) { if (value == null) { value = DEFAULT_GUIDE_BUTTON_LED; } return switch (value) { - case "off" -> 0x00; - case "dim" -> 0x0A; - // The spec's ceiling. xow believed the maximum was 0x20, so whether a pad honours - // anything above that is an open hardware question - see HARDWARE_TESTING.md 22. - case "bright" -> 0x2F; + case "off" -> new GuideButtonLed(GuideButtonLed.INTENSITY_OFF, false); + case "dim" -> new GuideButtonLed(GuideButtonLed.INTENSITY_DIM, false); + case "bright" -> new GuideButtonLed(GuideButtonLed.INTENSITY_BRIGHT, false); + case "battery" -> new GuideButtonLed(GuideButtonLed.INTENSITY_NORMAL, true); // "normal", plus anything unrecognised - default -> 0x14; + default -> GuideButtonLed.NORMAL; }; } diff --git a/app/src/main/jni/xow_driver/controller/controller.cpp b/app/src/main/jni/xow_driver/controller/controller.cpp index 57ffddf0..67c33239 100644 --- a/app/src/main/jni/xow_driver/controller/controller.cpp +++ b/app/src/main/jni/xow_driver/controller/controller.cpp @@ -110,12 +110,18 @@ #define RUMBLE_SCALE(magnitude) \ static_cast((static_cast(magnitude) * RUMBLE_MAX_POWER) / UINT16_MAX) -// Initialiser order follows declaration order in the header, which -Wreorder checks +// Initialiser order follows declaration order in the header, which -Wreorder checks. +// +// Pattern and intensity are separate fields of the LED command (MS-GIPUSB Table 42), so an +// intensity of zero on a lit pattern is not what the spec means by off. A zero brightness +// therefore selects the off pattern rather than a maximally dimmed on one. Controller::Controller( SendPacket sendPacket, uint8_t ledBrightness ) : GipDevice(std::move(sendPacket)), - stopRumbleThread(false), ledBrightness(ledBrightness), jvm(nullptr), jthis(nullptr) {} + stopRumbleThread(false), + ledPattern(ledBrightness == 0 ? LED_OFF : LED_ON), ledBrightness(ledBrightness), + jvm(nullptr), jthis(nullptr) {} Controller::~Controller() { @@ -401,7 +407,24 @@ void Controller::statusReceived(uint8_t id, const StatusData *status) Log::info("Controller is powering off or resetting"); } - notifyJavaBattery(type, level, charge); + /* + * If Java was not there to hear it, leave the filter unarmed so the next status gets through. + * + * A cabled pad's read thread is running before the Java object that wraps it exists - + * WiredController::start() precedes it - so the first status can land with no jthis to + * deliver to. With the filter armed regardless, Java heard nothing until the battery actually + * moved, which on a full pad is hours. Status is periodic (MS-GIPUSB 2.2.9: every second for + * the first 10 s after START, every 20 s after that), so the next one is at most a second away + * and finds the Java side registered. The adapter path never needs this: a pad's Java object + * exists before its first packet is read. + */ + if (!notifyJavaBattery(type, level, charge)) + { + batteryType = 0xff; + batteryLevel = 0xff; + batteryCharge = 0xff; + powerLevel = 0xff; + } } /* @@ -513,21 +536,23 @@ void Controller::inputReceived(uint8_t id, const InputData *input) stickLeftX, stickLeftY, stickRightX, stickRightY); } -void Controller::notifyJavaBattery(uint8_t type, uint8_t level, uint8_t charge) +bool Controller::notifyJavaBattery(uint8_t type, uint8_t level, uint8_t charge) { if (jthis == nullptr || updateBatteryMethod == nullptr) { - return; + return false; } JNIEnv *env = getAttachedEnv(jvm); if (env == nullptr) { - return; + return false; } // Raw GIP values: the mapping onto Moonlight's battery constants lives on the Java side, // where those constants are defined. env->CallVoidMethod(jthis, updateBatteryMethod, static_cast(type), static_cast(level), static_cast(charge)); + + return true; } void Controller::initInput() @@ -799,31 +824,18 @@ void Controller::startDevice() return; } - LedModeData ledMode = {}; - /* - * Guide button brightness, from the user's setting. - * - * The range is 0x00 to 0x2F, not the 0x00 to 0x20 xow claimed here: MS-GIPUSB 3.1.5.5.7 - * Table 41 gives the intensity field as "0 - 47%", and xone caps it at 50. Whether a pad - * actually honours anything above 0x20 is untested - see HARDWARE_TESTING.md 22. - * - * Pattern and intensity are separate fields (Table 42), so an intensity of zero on a lit - * pattern is not what the spec means by off. A zero brightness therefore selects the off - * pattern rather than a maximally dimmed on one. + * The guide button LED, from the user's setting - or, if the battery has already reported by + * now, from wherever the battery ladder left it: this applies whatever is stored rather than + * recomputing the start value, so a report that beat the start is not overwritten by it. */ - ledMode.mode = ledBrightness == 0 ? LED_OFF : LED_ON; - ledMode.brightness = ledBrightness; - - if (!setLedMode(ledMode)) + if (!applyLed()) { Log::error("Failed to set initial LED mode"); return; } - Log::info("Guide button LED set to mode %d intensity 0x%02x", ledMode.mode, ledBrightness); - if (!requestSerialNumber()) { Log::error("Failed to request serial number"); @@ -1943,6 +1955,69 @@ void Controller::sendRumble() { rumbleCondition.notify_one(); } +bool Controller::applyLed() +{ + LedModeData ledMode = {}; + + /* + * The range is 0x00 to 0x2F, not the 0x00 to 0x20 xow claimed here: MS-GIPUSB 3.1.5.5.7 + * Table 41 gives the intensity field as "0 - 47%", and xone caps it at 50. Whether a pad + * actually honours anything above 0x20 is untested - see HARDWARE_TESTING.md 22. Both values + * arrive already checked, by the constructor's caller and by the JNI layer. + */ + ledMode.mode = ledPattern.load(); + ledMode.brightness = ledBrightness.load(); + + if (!setLedMode(ledMode)) + { + return false; + } + + // HARDWARE_TESTING.md 22 and 31 grep for this line; keep its wording + Log::info("Guide button LED set to mode %d intensity 0x%02x", ledMode.mode, ledMode.brightness); + + return true; +} + +bool Controller::setLed(uint8_t pattern, uint8_t brightness) +{ + /* + * The Java side carries its own copy of these constants in GuideButtonLed, and refusing + * anything outside the enum is what catches the two drifting apart: an unknown pattern is + * logged and refused here rather than sent to the pad. + */ + switch (pattern) + { + case LED_OFF: + case LED_ON: + case LED_BLINK_FAST: + case LED_BLINK_MED: + case LED_BLINK_SLOW: + case LED_FADE_SLOW: + case LED_FADE_FAST: + break; + + default: + Log::error("Refused unknown LED pattern 0x%02x", pattern); + + return false; + } + + ledPattern.store(pattern); + ledBrightness.store(brightness); + + // Stored but not sent: the device is still in Arrival, where an LED command goes unanswered, + // and startDevice() applies whatever is stored when it gets there. The store above and this + // load are both seq_cst against startDevice()'s exchange on deviceStarted and its loads in + // applyLed(), so one of the two sides always sends the state stored here. + if (!deviceStarted.load()) + { + return true; + } + + return applyLed(); +} + void Controller::inputRumble(short lowFreqMotor, short highFreqMotor) { this->rumbleLeft = lowFreqMotor; this->rumbleRight = highFreqMotor; diff --git a/app/src/main/jni/xow_driver/controller/controller.h b/app/src/main/jni/xow_driver/controller/controller.h index ac77cf0c..8b2abdf6 100644 --- a/app/src/main/jni/xow_driver/controller/controller.h +++ b/app/src/main/jni/xow_driver/controller/controller.h @@ -85,16 +85,33 @@ class Controller : public GipDevice { public: /* - * @param ledBrightness guide button LED intensity to apply once the device starts, as the - * protocol's own 0 - 0x2F field. Taken at construction because - * startDevice() sends the LED command as soon as metadata arrives, which - * can be before the Java object for this pad exists - a setter would race - * that packet. The default reproduces the value the driver sent - * unconditionally before this was configurable. + * @param ledBrightness guide button LED intensity to start with, as the protocol's own + * 0 - 0x2F field. Taken at construction because startDevice() sends the + * LED command as soon as metadata arrives, which can be before the Java + * object for this pad exists. Later changes go through setLed(), which + * defers until the device has started rather than racing that packet. + * The default reproduces the value the driver sent unconditionally before + * this was configurable. */ Controller(SendPacket sendPacket, uint8_t ledBrightness = 0x14); ~Controller(); + /* + * Changes the guide button LED. Stores the command, and sends it only once the device has + * started - a command sent in Arrival gets no answer, see the auth comment in startDevice() - + * so a call that lands early is applied by startDevice() rather than lost. + * + * Called from the Java battery callback, on this pad's read thread. That is the thread + * startDevice() itself sends from in the normal case, so no new cross-thread sender is + * introduced; the startThread fallback is the one path that can overlap it, and the atomics + * below plus deviceStarted settle that overlap with at worst a harmless double send. + * + * @param pattern a LedMode value; anything else is refused, not sent + * @param brightness 0 - 0x2F, already clamped by the JNI layer + * @return true if the pad took it, or it was stored for startDevice() to send + */ + bool setLed(uint8_t pattern, uint8_t brightness); + /* * Takes a global reference to thiz and resolves the callback methods once. Called from the * Java thread constructing XboxWirelessController, so env is that thread's - jmethodIDs are @@ -538,11 +555,21 @@ class Controller : public GipDevice bool audioVolumeKnown = false; /* - * Guide button LED intensity applied at startDevice(), from the user's setting. Const because - * it is fixed for the life of this pad: nothing re-sends the LED command, so changing it after - * the fact would leave the member and the hardware disagreeing. + * The guide button LED command last asked for: applied by startDevice() and re-sent by + * setLed(). Atomic rather than const because the Java side rewrites them from the read thread + * as the battery moves while startDevice() may be running on startThread's metadata-timeout + * path; with seq_cst stores and loads on both sides, whichever of the two sends last sends the + * newer state, and a state stored after the other's load is sent by the storer itself. + * Declared in this order, which the constructor's initialiser list follows. */ - const uint8_t ledBrightness; + std::atomic ledPattern; + std::atomic ledBrightness; + + /* + * Builds and sends the LED command from the two members above. The one place the frame is + * built, so startDevice() and setLed() cannot disagree about what a brightness of zero means. + */ + bool applyLed(); /* * The level in force, 0 - 100. Starts as a guess and is replaced by the device's own reported @@ -566,7 +593,11 @@ class Controller : public GipDevice */ std::atomic audioSoftwareScale{256}; - void notifyJavaBattery(uint8_t type, uint8_t level, uint8_t charge); + /* + * @return whether the Java side was there to take it. False before registerJavaContext(), + * which statusReceived() uses to leave its change filter unarmed - see there. + */ + bool notifyJavaBattery(uint8_t type, uint8_t level, uint8_t charge); // Last reported status fields, so only changes are forwarded. 0xff is "nothing seen yet", // which no GIP value collides with - each of these is two bits wide. diff --git a/app/src/main/jni/xow_driver/xow_driver_jni.cpp b/app/src/main/jni/xow_driver/xow_driver_jni.cpp index d6778902..021aa00f 100644 --- a/app/src/main/jni/xow_driver/xow_driver_jni.cpp +++ b/app/src/main/jni/xow_driver/xow_driver_jni.cpp @@ -37,7 +37,9 @@ namespace * The guide button LED intensity the protocol accepts, 0 to 47 (MS-GIPUSB Table 41). * * Clamped rather than trusted on the way in, as setAudioVolume() clamps its percentage: the - * Java mapping cannot currently produce anything outside this, but a jint is a jint. + * two Java producers - PreferenceConfiguration.getGuideButtonLedValue() for the presets and + * GuideButtonLed.forBattery() for the battery ladder - cannot currently produce anything + * outside this, but a jint is a jint. */ constexpr jint LED_BRIGHTNESS_MAX = 0x2F; @@ -105,6 +107,25 @@ namespace controller->inputRumbleTrigger(left_trigger, right_trigger); } + /* + * The battery ladder's LED command. The pattern is checked against the protocol's enum in + * Controller::setLed rather than here - the enum is GipDevice's, and protected - so this only + * keeps it inside a byte on the way there. See Controller::setLed for the threading. + */ + jboolean setLedNative(JNIEnv *env, jobject thiz, jlong handle, jint pattern, jint brightness) + { + if (pattern < 0 || pattern > 0xFF) + { + Log::error("Refused LED pattern 0x%x: not a byte", pattern); + + return JNI_FALSE; + } + + auto *controller = (Controller *) handle; + return controller->setLed((uint8_t) pattern, clampLedBrightness(brightness)) + ? JNI_TRUE : JNI_FALSE; + } + jboolean setAudioEnabledNative(JNIEnv *env, jobject thiz, jlong handle, jboolean enable) { auto *controller = (Controller *) handle; @@ -248,6 +269,7 @@ namespace {"queueAudioNative", "(J[SI)V", (void *) queueAudioNative}, {"sendRumble", "(JSS)V", (void *) sendRumble}, {"sendrumbleTriggers", "(JSS)V", (void *) sendrumbleTriggers}, + {"setLedNative", "(JII)Z", (void *) setLedNative}, }; const JNINativeMethod WIRED_METHODS[] = { diff --git a/app/src/main/res/values/arrays.xml b/app/src/main/res/values/arrays.xml index f6b82f62..9606a5a8 100644 --- a/app/src/main/res/values/arrays.xml +++ b/app/src/main/res/values/arrays.xml @@ -72,12 +72,14 @@ @string/guide_button_led_dim @string/guide_button_led_normal @string/guide_button_led_bright + @string/guide_button_led_battery off dim normal bright + battery @string/mouse_mode_relative diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index a02460b9..e11761e9 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -247,6 +247,7 @@ Dim Normal Bright + Battery level Headphone audio on cabled Xbox pads Use the full Xbox protocol driver for cabled controllers, so their headphone jack can play game audio. Experimental Mouse emulation via gamepad diff --git a/app/src/test/java/com/limelight/binding/input/driver/GuideButtonLedTest.java b/app/src/test/java/com/limelight/binding/input/driver/GuideButtonLedTest.java new file mode 100644 index 00000000..dd028a88 --- /dev/null +++ b/app/src/test/java/com/limelight/binding/input/driver/GuideButtonLedTest.java @@ -0,0 +1,153 @@ +package com.limelight.binding.input.driver; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +/** + * Tests for the guide LED numbers and the battery ladder. + * + *

Everything here goes to the pad byte for byte, so the values are pinned rather than + * described: a step that silently changed would otherwise only be visible on hardware, and the + * hardware is the one place these are hard to check. + */ +class GuideButtonLedTest { + + @Nested + @DisplayName("forBattery()") + class Ladder { + + /** + * The ladder in full. Type 1 and 2 are the two kinds of battery the spec knows; type 0 is + * no battery at all, whose level the spec says to ignore. Level 7 does not exist and must + * change nothing, which means the start state rather than the brightest or dimmest step. + */ + @ParameterizedTest(name = "type {0} level {1} -> pattern {2} intensity {3}") + @CsvSource({ + "1, 3, 0x01, 0x2F", + "1, 2, 0x01, 0x14", + "1, 1, 0x01, 0x0A", + "1, 0, 0x04, 0x14", + "2, 0, 0x04, 0x14", + "0, 0, 0x01, 0x2F", + "0, 3, 0x01, 0x2F", + "1, 7, 0x01, 0x14", + }) + @DisplayName("each battery report maps to its LED command") + void ladderIsPinned(byte type, byte level, int pattern, int intensity) { + assertEquals(new GuideButtonLed.State(pattern, intensity), + GuideButtonLed.forBattery(type, level)); + } + + /** + * The flash is the warning. Any other level flashing would make the warning meaningless, + * and a flashing full pad would send the user hunting for a charger. + */ + @Test + @DisplayName("only the critical level uses a pattern other than on") + void onlyCriticalBlinks() { + for (byte level = 1; level <= 3; level++) { + assertEquals(GuideButtonLed.PATTERN_ON, + GuideButtonLed.forBattery((byte) 1, level).pattern(), + "level " + level + " should be steady"); + } + assertEquals(GuideButtonLed.PATTERN_ON, + GuideButtonLed.forBattery((byte) 0, (byte) 0).pattern(), + "a pad on its cable should be steady"); + assertNotEquals(GuideButtonLed.PATTERN_ON, + GuideButtonLed.forBattery((byte) 1, (byte) 0).pattern()); + } + + /** + * A pad about to die must still show something: an LED that went dark at critical would + * read as the pad having already gone. + */ + @Test + @DisplayName("critical is not off") + void criticalIsNotOff() { + GuideButtonLed.State critical = GuideButtonLed.forBattery((byte) 1, (byte) 0); + + assertNotEquals(GuideButtonLed.PATTERN_OFF, critical.pattern()); + assertTrue(critical.intensity() > 0); + } + + /** The steady steps dim as the battery drains; a brighter step for less charge is a bug. */ + @Test + @DisplayName("intensity never rises as the level drops") + void intensityNeverRisesAsTheLevelDrops() { + int previous = Integer.MAX_VALUE; + for (byte level = 3; level >= 1; level--) { + int intensity = GuideButtonLed.forBattery((byte) 1, level).intensity(); + assertTrue(intensity <= previous, + "level " + level + " is brighter than the level above it"); + previous = intensity; + } + } + + /** Nothing may exceed what the protocol's intensity field can carry, whatever the input. */ + @Test + @DisplayName("every state is within Table 41's range") + void everyStateIsWithinTable41() { + for (byte type = 0; type <= 3; type++) { + for (byte level = 0; level <= 3; level++) { + int intensity = GuideButtonLed.forBattery(type, level).intensity(); + assertTrue(intensity >= 0 && intensity <= GuideButtonLed.INTENSITY_MAX, + "type " + type + " level " + level + " gives " + intensity); + } + } + } + } + + @Nested + @DisplayName("startState()") + class StartState { + + /** + * Pattern and intensity are separate fields (Table 42), so off has to be the off pattern: + * an intensity of zero on the on pattern is a maximally dimmed on, and what a pad does + * with that is not something the spec promises. + */ + @Test + @DisplayName("zero intensity selects the off pattern") + void zeroIntensitySelectsTheOffPattern() { + assertEquals(new GuideButtonLed.State(GuideButtonLed.PATTERN_OFF, 0), + new GuideButtonLed(0, false).startState()); + } + + @Test + @DisplayName("any other intensity selects the on pattern") + void nonZeroSelectsOn() { + assertEquals(new GuideButtonLed.State(GuideButtonLed.PATTERN_ON, 0x14), + new GuideButtonLed(0x14, true).startState()); + } + } + + @Nested + @DisplayName("constants") + class Constants { + + /** + * These cross the JNI boundary as plain ints and are matched against {@code enum LedMode} + * in {@code app/src/main/jni/xow_driver/controller/gip.h} by value. The native side + * refuses a value outside that enum, so a drift would show as a refused command on + * hardware; this pins the Java half so it shows here first. + */ + @Test + @DisplayName("patterns match enum LedMode in gip.h") + void patternsMatchGipHeader() { + assertEquals(0x00, GuideButtonLed.PATTERN_OFF); + assertEquals(0x01, GuideButtonLed.PATTERN_ON); + assertEquals(0x02, GuideButtonLed.PATTERN_BLINK_FAST); + assertEquals(0x03, GuideButtonLed.PATTERN_BLINK_MED); + assertEquals(0x04, GuideButtonLed.PATTERN_BLINK_SLOW); + assertEquals(0x08, GuideButtonLed.PATTERN_FADE_SLOW); + assertEquals(0x09, GuideButtonLed.PATTERN_FADE_FAST); + } + } +} diff --git a/app/src/test/java/com/limelight/preferences/PreferenceConfigurationTest.java b/app/src/test/java/com/limelight/preferences/PreferenceConfigurationTest.java index 7166c92f..bdfb6cd4 100644 --- a/app/src/test/java/com/limelight/preferences/PreferenceConfigurationTest.java +++ b/app/src/test/java/com/limelight/preferences/PreferenceConfigurationTest.java @@ -10,6 +10,7 @@ import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; +import com.limelight.binding.input.driver.GuideButtonLed; import com.limelight.nvstream.StreamConfiguration; /** @@ -75,50 +76,75 @@ void matchesPreferenceEntryValues() { @Nested @DisplayName("guide button LED brightness") - class GuideButtonLed { + class GuideButtonLedSetting { /** * The values are the protocol's, not ours: MS-GIPUSB 3.1.5.5.7 Table 41 defines the LED * command's intensity byte as 0 to 47. Pinning them here is the point of the test — a - * preset that silently changed value would otherwise only be visible on hardware. + * preset that silently changed value would otherwise only be visible on hardware. The + * battery setting starts at normal, since the pad has not said what it has yet; the + * ladder that moves it from there is {@code GuideButtonLedTest}'s to pin. */ - @ParameterizedTest(name = "\"{0}\" maps to {1}") + @ParameterizedTest(name = "\"{0}\" starts at {1}") @CsvSource({ - "off, 0x00", - "dim, 0x0A", - "normal, 0x14", - "bright, 0x2F", + "off, 0x00", + "dim, 0x0A", + "normal, 0x14", + "bright, 0x2F", + "battery, 0x14", }) - @DisplayName("each preset maps to its protocol intensity") + @DisplayName("each preset maps to its starting protocol intensity") void presetsMapToIntensity(String stored, int expected) { - assertEquals(expected, PreferenceConfiguration.getGuideButtonLedValue(stored)); + assertEquals(expected, + PreferenceConfiguration.getGuideButtonLedValue(stored).intensity()); + } + + /** + * Only the battery setting lets battery reports move the LED. A fixed preset that + * followed the battery would drift away from what the user chose; a battery setting that + * did not would be the normal preset under another name. + */ + @ParameterizedTest(name = "\"{0}\" follows the battery: {1}") + @CsvSource({ + "off, false", + "dim, false", + "normal, false", + "bright, false", + "battery, true", + }) + @DisplayName("only the battery setting follows the battery") + void onlyBatteryFollowsTheBattery(String stored, boolean expected) { + assertEquals(expected, + PreferenceConfiguration.getGuideButtonLedValue(stored).followsBattery()); } /** * A preference the user has never touched reads back as null, and a value written by a * future build might be anything. Both have to land on the intensity the driver used - * before this was configurable, rather than leaving the LED off or at full brightness. + * before this was configurable, rather than leaving the LED off or at full brightness — + * and must not follow the battery, which nobody asked for. */ @Test - @DisplayName("null falls back to the normal intensity") + @DisplayName("null falls back to the normal preset") void nullFallsBackToNormal() { - assertEquals(0x14, PreferenceConfiguration.getGuideButtonLedValue(null)); + assertEquals(GuideButtonLed.NORMAL, PreferenceConfiguration.getGuideButtonLedValue(null)); } @Test - @DisplayName("an unrecognised value falls back to the normal intensity") + @DisplayName("an unrecognised value falls back to the normal preset") void unrecognisedFallsBackToNormal() { - assertEquals(0x14, PreferenceConfiguration.getGuideButtonLedValue("chartreuse")); + assertEquals(GuideButtonLed.NORMAL, + PreferenceConfiguration.getGuideButtonLedValue("chartreuse")); } /** Nothing may exceed what the protocol's intensity field can carry. */ @ParameterizedTest - @CsvSource({"off", "dim", "normal", "bright"}) + @CsvSource({"off", "dim", "normal", "bright", "battery"}) @DisplayName("no preset exceeds the protocol maximum of 0x2F") void presetsStayWithinProtocolRange(String stored) { - int intensity = PreferenceConfiguration.getGuideButtonLedValue(stored); + int intensity = PreferenceConfiguration.getGuideButtonLedValue(stored).intensity(); - assertTrue(intensity >= 0x00 && intensity <= 0x2F, + assertTrue(intensity >= 0x00 && intensity <= GuideButtonLed.INTENSITY_MAX, "intensity " + intensity + " is outside the protocol's 0x00-0x2F range"); } }