From a5bf7dfe17749691e56f7098de476e6ceb91c2bf Mon Sep 17 00:00:00 2001 From: Turan Asikoglu <874158+trexx@users.noreply.github.com> Date: Wed, 16 Sep 2026 21:54:58 +0200 Subject: [PATCH 1/2] Show controller batteries under the game menu, and let the guide LED follow them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Battery state was already collected for every supported pad - GIP pads through Controller::statusReceived, Shield and Bluetooth pads through the 120 s poll - but only ever forwarded to the host, so nothing on the box could show it. The label: while any level of the in-game menu is up, a small end-aligned line along the bottom of the screen lists every controller's battery by host player number - "Controller 1: 75% · Controller 2: 50%, charging · Controller 3: wired". A new Android-free ControllerBatteries store, fed by both battery paths, keeps one reading per number; Game formats it. The menu is a chain of AlertDialogs whose onDismiss fires for a parent after its child has opened, so a DialogChain token decides which dismissal is the last one and hides the label. The dialog's dim stays; the text is the primary colour so it reads through it. The LED: "Battery level" is a fifth value of the Xbox button brightness setting. The guide LED is monochrome, so it steps through the presets - full Bright, medium Normal, low Dim - and slow-blinks at critical, since a warning has to look like one. GuideButtonLed holds every LED constant and the ladder on the Java side, where it is tested; the native side gains Controller::setLed, which stores the command and sends it once the device has started, and startDevice() applies whatever is stored through the same applyLed() so the frame is built in one place. ledBrightness loses its const and gains a pattern, as std::atomic against the startThread fallback. Two pre-existing faults fixed on the way, both found while wiring this up: - A GIP pad's first status packet usually lands before its first input report, which is what assigns its player number, so its first battery report went to the host as player 0. It is now held on the context until the number is assigned. - On the cabled path the read thread runs before the Java object exists, and a status in that window armed the driver's change filter with nobody listening, so Java heard nothing until the battery actually moved. 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. Latency: nothing on a per-frame path. Battery writes happen at battery-change frequency on driver threads; the label is built once per menu level on the UI thread while the decoder is already stalled behind the dialog, and is a child View of the window rather than a layer; the LED sends one GIP frame per level change on the driver's read thread, deduplicated against charge-only changes. Tests: ControllerBatteriesTest, DialogChainTest, GuideButtonLedTest, and the guide LED cases in PreferenceConfigurationTest. HARDWARE_TESTING.md 31 lists what needs a device - in particular whether a pad honours LED_BLINK_SLOW at all, which Table 42 leaves open. Co-Authored-By: Claude Opus 5 --- HARDWARE_TESTING.md | 95 +++++++++- app/src/main/java/com/limelight/Game.java | 54 +++++- app/src/main/java/com/limelight/GameMenu.java | 22 ++- .../binding/input/ControllerBatteries.java | 142 +++++++++++++++ .../binding/input/ControllerHandler.java | 117 ++++++++---- .../binding/input/driver/GipController.java | 65 ++++++- .../binding/input/driver/GuideButtonLed.java | 96 ++++++++++ .../input/driver/UsbDriverListener.java | 4 +- .../input/driver/XboxWiredGipController.java | 21 ++- .../input/driver/XboxWirelessDongle.java | 28 +-- .../preferences/PreferenceConfiguration.java | 35 ++-- .../java/com/limelight/utils/DialogChain.java | 35 ++++ .../java/com/limelight/utils/MenuDialog.java | 4 +- .../jni/xow_driver/controller/controller.cpp | 121 ++++++++++--- .../jni/xow_driver/controller/controller.h | 53 ++++-- .../main/jni/xow_driver/xow_driver_jni.cpp | 24 ++- app/src/main/res/layout/activity_game.xml | 34 ++++ app/src/main/res/values/arrays.xml | 2 + app/src/main/res/values/strings.xml | 8 + .../input/ControllerBatteriesTest.java | 166 ++++++++++++++++++ .../input/driver/GuideButtonLedTest.java | 153 ++++++++++++++++ .../PreferenceConfigurationTest.java | 60 +++++-- .../com/limelight/utils/DialogChainTest.java | 72 ++++++++ 23 files changed, 1270 insertions(+), 141 deletions(-) create mode 100644 app/src/main/java/com/limelight/binding/input/ControllerBatteries.java create mode 100644 app/src/main/java/com/limelight/binding/input/driver/GuideButtonLed.java create mode 100644 app/src/main/java/com/limelight/utils/DialogChain.java create mode 100644 app/src/test/java/com/limelight/binding/input/ControllerBatteriesTest.java create mode 100644 app/src/test/java/com/limelight/binding/input/driver/GuideButtonLedTest.java create mode 100644 app/src/test/java/com/limelight/utils/DialogChainTest.java diff --git a/HARDWARE_TESTING.md b/HARDWARE_TESTING.md index 6250eb0d..05b51a70 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,89 @@ it would say whether "never drop" was ever more than an accident of the PTS base --- +## 31. Controller battery: the in-game label, and the guide LED following it + +Two additions built on the battery reports the app already collected and only ever forwarded to +the host. Neither touches a per-frame path: battery writes happen at battery-change frequency on +driver threads, the label is built once per menu level while the decoder is already stalled behind +the dialog, and the LED sends one GIP frame per *level* change on the driver's read thread. + +**The label.** While any level of the in-game menu is up, a small end-aligned line along the +bottom of the screen lists every controller's battery, one entry per host player number: +`Controller 1: 75% · Controller 2: 50%, charging · Controller 3: wired`. "wired" is a GIP pad +running off its cable with no battery pack — it reports `NOT_CHARGING` and no percentage, and the +cable is the only fact in that. The store behind it is `ControllerBatteries`; the menu keeps it +shown across submenus with a `DialogChain` token and hides it when its last dialog closes. + +The GIP percentages are the spec's four levels mapped as `GipController.updateBattery()` always +has: full 100, medium 50, low 25, critical 10. Shield and Bluetooth pads report real percentages. + +Also fixed on the way: a GIP pad's first status packet usually arrives before its first input +report, which is what assigns its player number, so that first battery report used to reach the +host as player 0. It is now held on the context until the number is assigned. And on the cabled +path, a status that landed before the Java object existed used to arm the driver's change filter +with nobody listening, so Java heard nothing until the battery actually moved; the filter now +stays unarmed until a report is delivered, and MS-GIPUSB 2.2.9 has the pad repeat status every +second for its first ten seconds. + +**The LED.** "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`. + +Like section 22, the setting is read once in `UsbDriverService.onCreate()`, so the procedure is +"change setting, leave the stream, start it again". + +The open hardware questions: + +- [ ] **The label is legible under the dim.** Open the menu with two pads in different states. + The dialog dims everything behind it at the theme's default 60%; the text is the primary + colour rather than the secondary one the settings version label uses for exactly this + reason. If it is still too faint from the sofa, that is a colour change, not a dim change. +- [ ] **Several controllers, in player order.** Both pads listed, numbered as the host numbers + them. Note the pad-audio submenu numbers GIP pads by their position in the driver's list + instead, so the two can disagree when an Android-enumerated pad is also present. +- [ ] **The label survives the menu's own navigation.** Open Controllers, then Back: the label + stays. Cancel, or Back at the root: the label goes. Choose Disconnect: the label goes. +- [ ] **A pad that unplugs mid-menu is gone on the next open.** Its number is released, which + clears its slot. +- [ ] **A battery-less cabled pad reads "wired".** Requires a pad with the pack removed, on the + wired GIP path (`wiredPadAudio` on). +- [ ] **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). +- [ ] **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 label and the LED should both 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 +2884,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 "wired" and type-0 cases | | §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/Game.java b/app/src/main/java/com/limelight/Game.java index e6df26c0..9f0e0330 100644 --- a/app/src/main/java/com/limelight/Game.java +++ b/app/src/main/java/com/limelight/Game.java @@ -4,6 +4,7 @@ import com.limelight.binding.PlatformBinding; import com.limelight.binding.audio.LowLatencyAudioRenderer; import com.limelight.binding.audio.PadAudioSink; +import com.limelight.binding.input.ControllerBatteries; import com.limelight.binding.input.ControllerHandler; import com.limelight.binding.input.GameInputDevice; import com.limelight.binding.input.KeyboardTranslator; @@ -89,6 +90,7 @@ import java.security.cert.CertificateFactory; import java.security.cert.X509Certificate; import java.util.ArrayDeque; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Locale; @@ -166,6 +168,10 @@ private record ScheduledStep(long uptimeMs, TextKeyPlanner.Step step) {} // comment. private TextView imePreviewOverlay; + // The controllers' battery levels, along the bottom edge while the in-game menu is up. Never + // focused either; the menu's dialog owns focus for as long as this is visible. + private TextView controllerBatteryOverlay; + // Bottom margin currently applied to the echo, so the insets listener only re-lays it out when // the keyboard actually changes size rather than on every insets change during a stream. private int imePreviewInset = -1; @@ -446,6 +452,7 @@ protected void onCreate(Bundle savedInstanceState) { performanceSparklines = findViewById(R.id.performanceSparklines); imePreviewOverlay = findViewById(R.id.imePreviewOverlay); + controllerBatteryOverlay = findViewById(R.id.controllerBatteryOverlay); inputCaptureProvider = InputCaptureManager.getInputCaptureProvider(this); @@ -2470,7 +2477,8 @@ public void togglePerformanceOverlay() { } /** - * Opens the in-stream menu. + * Opens the in-stream menu. The menu also shows every controller's battery along the bottom + * of the screen for as long as it is up - see {@link #showControllerBatteryLabel}. * * @param device the controller that requested it, so the menu can offer that controller's own * options, or null when opened from a keyboard or gesture @@ -2485,6 +2493,50 @@ public void showGameMenu(GameInputDevice device) { new GameMenu(this, conn, device); } + /** + * Fills the battery label from the controllers' last readings and shows it, or hides it when + * no controller has anything to show. One entry per controller, in player order, so several + * pads read side by side rather than the last one reported winning. + * + *

Numbered as the host numbers them - player 1 is controller number 0 - which is not the + * order the pad-audio submenu uses; that lists GIP pads by their position in the driver's + * list, so the two can disagree when an Android-enumerated pad is also attached. + * + *

UI thread only: the menu opens from {@link #onBackPressed} or a controller button on the + * main thread and calls this before showing each level. The label is a child of this + * activity's window, not a new layer, and the decoder is already stalled behind the menu's + * dialog, so the cost of building the text is not on any frame. + */ + public void showControllerBatteryLabel() { + var parts = new ArrayList(); + for (ControllerBatteries.Reading reading : controllerHandler.batterySnapshot()) { + int player = reading.controllerNumber() + 1; + switch (reading.display()) { + case LEVEL -> parts.add(getString(R.string.controller_battery_level, + player, reading.percentage())); + case CHARGING -> parts.add(getString(R.string.controller_battery_charging, + player, reading.percentage())); + case WIRED -> parts.add(getString(R.string.controller_battery_wired, player)); + // Never in a snapshot; listed so the switch stays exhaustive when Display grows + case HIDDEN -> { } + } + } + + if (parts.isEmpty()) { + controllerBatteryOverlay.setVisibility(View.GONE); + return; + } + + controllerBatteryOverlay.setText( + String.join(getString(R.string.controller_battery_separator), parts)); + controllerBatteryOverlay.setVisibility(View.VISIBLE); + } + + /** Takes the battery label down. UI thread; the menu calls it when its last dialog closes. */ + public void hideControllerBatteryLabel() { + controllerBatteryOverlay.setVisibility(View.GONE); + } + /** * @return true if the USB driver has an Xbox wireless adapter running, which is what decides * whether the game menu offers to pair a controller with it. False when the USB diff --git a/app/src/main/java/com/limelight/GameMenu.java b/app/src/main/java/com/limelight/GameMenu.java index 8c6a18e2..3d15f5ec 100644 --- a/app/src/main/java/com/limelight/GameMenu.java +++ b/app/src/main/java/com/limelight/GameMenu.java @@ -9,6 +9,7 @@ import com.limelight.binding.input.driver.GipController; import com.limelight.nvstream.NvConnection; import com.limelight.nvstream.input.KeyboardPacket; +import com.limelight.utils.DialogChain; import com.limelight.utils.MenuDialog; import java.util.ArrayList; @@ -17,7 +18,9 @@ /** * Provide options for ongoing Game Stream. *

- * Shown on back action in game activity. + * Shown on back action in game activity. While any level of it is up, {@link Game} also shows the + * controllers' battery levels along the bottom of the screen; the menu is what decides when that + * label appears and goes away. */ public class GameMenu { @@ -41,6 +44,9 @@ public MenuOption(String label, Runnable runnable) { private final NvConnection conn; private final GameInputDevice device; + // One menu is one chain of dialogs; see showMenuDialog for what it settles. + private final DialogChain chain = new DialogChain(); + /** Building the menu shows it immediately; there is no separate show call. */ public GameMenu(Game game, NvConnection conn, GameInputDevice device) { this.game = game; @@ -129,6 +135,11 @@ private void showMenuDialog(String title, MenuOption[] options) { * own dialog with no relationship to the one that opened it, so without this a submenu's Back * dropped straight to the stream - further out than the user asked to go. The root passes * null, where dismissing really does mean returning to the game. + * + *

The battery label is shown afresh at every level, so a reading that changed while a + * submenu was up is picked up on the way back, and hidden only when the whole chain ends. + * Every dialog's {@code onDismiss} fires, including a parent's after its child has opened, so + * each one takes a {@link DialogChain} token and only the latest holder hides the label. */ private void showMenuDialog(String title, MenuOption[] options, Runnable onBack) { List rows = new ArrayList<>(); @@ -142,7 +153,14 @@ private void showMenuDialog(String title, MenuOption[] options, Runnable onBack) : runnable)); } - MenuDialog.show(game, title, 0, rows, onBack, null); + int token = chain.opened(); + game.showControllerBatteryLabel(); + + MenuDialog.show(game, title, 0, rows, onBack, () -> { + if (chain.closes(token)) { + game.hideControllerBatteryLabel(); + } + }); } /** diff --git a/app/src/main/java/com/limelight/binding/input/ControllerBatteries.java b/app/src/main/java/com/limelight/binding/input/ControllerBatteries.java new file mode 100644 index 00000000..de33109e --- /dev/null +++ b/app/src/main/java/com/limelight/binding/input/ControllerBatteries.java @@ -0,0 +1,142 @@ +package com.limelight.binding.input; + +import com.limelight.nvstream.jni.MoonBridge; + +import java.util.ArrayList; +import java.util.List; + +/** + * The last battery reading of every attached controller, kept so the in-game menu can show them. + * + *

Split out of {@link ControllerHandler} for the same reason {@link ControllerLedColor} is: the + * handler needs a real {@code InputDevice} and cannot be loaded from a JVM test. Battery state used + * to be fire-and-forget to the host, so nothing on the box could show it; this is the one place it + * is retained, and the only consumer is the label the menu shows. + * + *

Slots are indexed by host player number rather than device id, because that is the number the + * user sees on the host and the one the label prints. The attached-controller mask is a + * {@code short}, which is where {@link #MAX_CONTROLLERS} comes from. + * + *

Written from two threads that never meet the reader: a GIP pad reports from its driver's read + * thread, an Android-enumerated pad from the battery {@code HandlerThread}, and the label reads on + * the UI thread once per menu level. {@code synchronized} is the right tool at those rates — a few + * writes per session, one read per menu open — and keeps the snapshot consistent without any + * cleverness. + * + *

References {@code MoonBridge.LI_BATTERY_*} freely: they are {@code static final byte} + * compile-time constants, so {@code javac} inlines them and this class never triggers + * {@code MoonBridge}'s static initialiser, which loads a native library and would kill a JVM test. + * Nothing else in {@code MoonBridge} may be touched from here. + * + *

Latency: none. Nothing here is on the frame path or the input path. + */ +public final class ControllerBatteries { + + /** One more than the highest player number; the attached-controller mask is 16 bits wide. */ + public static final int MAX_CONTROLLERS = 16; + + /** + * What a reading is worth showing as. {@link #HIDDEN} never leaves {@link #snapshot()}: it is + * the classifier's way of saying the reading carries nothing the user can act on. + */ + public enum Display { + /** A percentage, discharging or otherwise not being filled. */ + LEVEL, + /** A percentage, and the pad is charging. */ + CHARGING, + /** No battery level at all, but the pad is on a cable — a GIP pad with no battery pack. */ + WIRED, + /** Nothing to show. */ + HIDDEN + } + + /** + * One controller worth showing. + * + * @param controllerNumber host player number, 0-based + * @param display what to show it as; never {@link Display#HIDDEN} + * @param percentage 0 to 100; only meaningful for {@link Display#LEVEL} and + * {@link Display#CHARGING} + */ + public record Reading(int controllerNumber, Display display, int percentage) { + } + + // Moonlight's own units, exactly as they go to the host, so the label and the host never + // disagree about a pad. Percentage is 0 - 100 or LI_BATTERY_PERCENTAGE_UNKNOWN. + private final byte[] states = new byte[MAX_CONTROLLERS]; + private final byte[] percentages = new byte[MAX_CONTROLLERS]; + private final boolean[] present = new boolean[MAX_CONTROLLERS]; + + /** + * Records a reading, replacing whatever the slot held. + * + *

An out-of-range number is ignored rather than thrown on: this is fed from driver threads, + * and an exception there takes the pad's read loop down with it. + * + * @param controllerNumber host player number + * @param state one of {@code MoonBridge.LI_BATTERY_STATE_*} + * @param percentage 0 to 100, or {@code MoonBridge.LI_BATTERY_PERCENTAGE_UNKNOWN} + */ + public synchronized void update(int controllerNumber, byte state, byte percentage) { + if (controllerNumber < 0 || controllerNumber >= MAX_CONTROLLERS) { + return; + } + states[controllerNumber] = state; + percentages[controllerNumber] = percentage; + present[controllerNumber] = true; + } + + /** Forgets a slot, for a controller whose player number has been released. */ + public synchronized void clear(int controllerNumber) { + if (controllerNumber < 0 || controllerNumber >= MAX_CONTROLLERS) { + return; + } + present[controllerNumber] = false; + } + + /** + * @return every controller with something to show, in ascending player number. Allocates a + * small list per call, which is fine: it is called once per menu level, never per frame. + */ + public synchronized List snapshot() { + List readings = new ArrayList<>(); + for (int i = 0; i < MAX_CONTROLLERS; i++) { + if (!present[i]) { + continue; + } + Display display = classify(states[i], percentages[i]); + if (display == Display.HIDDEN) { + continue; + } + readings.add(new Reading(i, display, percentages[i] & 0xFF)); + } + return readings; + } + + /** + * Decides what a reading shows as. Package-private so the table can be tested directly. + * + *

The unknown-percentage cases are the non-obvious ones. A GIP pad running off its cable + * with no battery pack reports {@code NOT_CHARGING} with an unknown percentage, because the spec + * gives it no level to report; the only fact in that reading is that it is on a cable, which is + * still worth a word. {@code CHARGING} with no percentage is the same fact. Everything else + * without a percentage says nothing. + * + *

With a percentage, only {@code CHARGING} needs distinguishing: {@code DISCHARGING}, + * {@code FULL}, {@code NOT_CHARGING} and even {@code UNKNOWN} — which Android's + * {@code BatteryState} can report alongside a real capacity — all just show the level. + */ + static Display classify(byte state, byte percentage) { + if (state == MoonBridge.LI_BATTERY_STATE_NOT_PRESENT) { + return Display.HIDDEN; + } + if (percentage == MoonBridge.LI_BATTERY_PERCENTAGE_UNKNOWN) { + return switch (state) { + case MoonBridge.LI_BATTERY_STATE_CHARGING, + MoonBridge.LI_BATTERY_STATE_NOT_CHARGING -> Display.WIRED; + default -> Display.HIDDEN; + }; + } + return state == MoonBridge.LI_BATTERY_STATE_CHARGING ? Display.CHARGING : Display.LEVEL; + } +} diff --git a/app/src/main/java/com/limelight/binding/input/ControllerHandler.java b/app/src/main/java/com/limelight/binding/input/ControllerHandler.java index a4b57564..a6390c5b 100644 --- a/app/src/main/java/com/limelight/binding/input/ControllerHandler.java +++ b/app/src/main/java/com/limelight/binding/input/ControllerHandler.java @@ -145,6 +145,11 @@ public class ControllerHandler implements InputManager.InputDeviceListener, UsbD private final SparseArray inputDeviceContexts = new SparseArray<>(); private final SparseArray usbDeviceContexts = new SparseArray<>(); + // What the in-game menu's battery label reads. Kept apart from the contexts above because + // those are touched from the USB driver thread (deviceAdded) and the label reads on the UI + // thread; iterating a SparseArray across that pair is a race, this store is not. + private final ControllerBatteries batteries = new ControllerBatteries(); + private final NvConnection conn; private final Activity activityContext; private final double stickDeadzone; @@ -448,6 +453,11 @@ private void releaseControllerNumber(GenericControllerContext context) { if (context.reservedControllerNumber) { LimeLog.info("Controller number "+context.controllerNumber+" is now available"); currentControllers &= ~(1 << context.controllerNumber); + + // Only a reserved number is a pad's own. With multi-controller off every pad shares + // number 0, the last reporter's battery is the one shown, and nothing clears it until + // the stream ends - accepted, since that mode merges the pads for the host too. + batteries.clear(context.controllerNumber); } // If this device sent data as a gamepad, zero the values before removing. @@ -603,6 +613,30 @@ else if (!devContext.hasJoystickAxes) { // Report attributes of this new controller to the host context.sendControllerArrival(); + + // A GIP pad's first status packet usually lands before its first input report, which is + // what brought us here, so its battery was cached rather than sent - see + // reportControllerBattery. Now that the pad has a number, hand it on. + if (context instanceof UsbDeviceContext usbContext && usbContext.hasBatteryReading) { + publishBattery(usbContext, usbContext.batteryState, usbContext.batteryPercentage); + } + } + + /** + * Reports a controller's battery to the host and to the in-game menu's label together, so the + * two never disagree about a pad. + */ + private void publishBattery(GenericControllerContext context, byte state, byte percentage) { + conn.sendControllerBatteryEvent((byte) context.controllerNumber, state, percentage); + batteries.update(context.controllerNumber, state, percentage); + } + + /** + * @return every controller with a battery reading worth showing, in player order. For the + * in-game menu's label; safe to call from the UI thread. + */ + public List batterySnapshot() { + return batteries.snapshot(); } /** @@ -1157,42 +1191,25 @@ else if (chargingState == SceChargingState.CHARGING) { if (currentBatteryStatus != context.lastReportedBatteryStatus || !areBatteryCapacitiesEqual(currentBatteryCapacity, context.lastReportedBatteryCapacity)) { - byte state; - byte percentage; - - switch (currentBatteryStatus) { - case BatteryState.STATUS_UNKNOWN: - state = MoonBridge.LI_BATTERY_STATE_UNKNOWN; - break; - - case BatteryState.STATUS_CHARGING: - state = MoonBridge.LI_BATTERY_STATE_CHARGING; - break; - - case BatteryState.STATUS_DISCHARGING: - state = MoonBridge.LI_BATTERY_STATE_DISCHARGING; - break; - - case BatteryState.STATUS_NOT_CHARGING: - state = MoonBridge.LI_BATTERY_STATE_NOT_CHARGING; - break; - - case BatteryState.STATUS_FULL: - state = MoonBridge.LI_BATTERY_STATE_FULL; - break; - - default: - return; - } - - if (Float.isNaN(currentBatteryCapacity)) { - percentage = MoonBridge.LI_BATTERY_PERCENTAGE_UNKNOWN; - } - else { - percentage = (byte)(currentBatteryCapacity * 100); - } - - conn.sendControllerBatteryEvent((byte)context.controllerNumber, state, percentage); + byte state = switch (currentBatteryStatus) { + case BatteryState.STATUS_UNKNOWN -> MoonBridge.LI_BATTERY_STATE_UNKNOWN; + case BatteryState.STATUS_CHARGING -> MoonBridge.LI_BATTERY_STATE_CHARGING; + case BatteryState.STATUS_DISCHARGING -> MoonBridge.LI_BATTERY_STATE_DISCHARGING; + case BatteryState.STATUS_NOT_CHARGING -> MoonBridge.LI_BATTERY_STATE_NOT_CHARGING; + case BatteryState.STATUS_FULL -> MoonBridge.LI_BATTERY_STATE_FULL; + // Not a status Moonlight has a word for; leave the last report standing. A + // switch expression cannot return from the method, hence the sentinel. + default -> (byte) -1; + }; + if (state < 0) { + return; + } + + byte percentage = Float.isNaN(currentBatteryCapacity) + ? MoonBridge.LI_BATTERY_PERCENTAGE_UNKNOWN + : (byte)(currentBatteryCapacity * 100); + + publishBattery(context, state, percentage); context.lastReportedBatteryStatus = currentBatteryStatus; context.lastReportedBatteryCapacity = currentBatteryCapacity; @@ -2929,19 +2946,33 @@ public void reportControllerMotion(int controllerId, byte motionType, float moti conn.sendControllerMotionEvent((byte) context.controllerNumber, motionType, motionX, motionY, motionZ); } - /** {@inheritDoc} Battery state from a controller driven by our own USB drivers. */ + /** + * {@inheritDoc} Battery state from a controller driven by our own USB drivers. + * + *

Held back until the pad has a player number. A USB pad is numbered on its first input + * report, and a GIP pad's first status packet usually arrives before that, so this used to + * reach the host as player 0 whatever slot the pad went on to take. The reading is cached on + * the context instead and sent by {@link #assignControllerNumberIfNeeded} once the number is + * known; anything after that goes straight through to the host and the menu's label. + */ @Override public void reportControllerBattery(int controllerId, byte batteryState, byte batteryPercentage) { if (stopped) { return; } - GenericControllerContext context = usbDeviceContexts.get(controllerId); + UsbDeviceContext context = usbDeviceContexts.get(controllerId); if (context == null) { return; } - conn.sendControllerBatteryEvent((byte) context.controllerNumber, batteryState, batteryPercentage); + context.batteryState = batteryState; + context.batteryPercentage = batteryPercentage; + context.hasBatteryReading = true; + + if (context.assignedControllerNumber) { + publishBattery(context, batteryState, batteryPercentage); + } } /** {@inheritDoc} Releases the context and player number for a USB controller that went away. */ @@ -3472,6 +3503,14 @@ public void enableSensors() { class UsbDeviceContext extends GenericControllerContext { public AbstractController device; + // The last battery the driver reported, in Moonlight's units, kept until the pad has a + // player number to report it under. Written by reportControllerBattery and read by + // assignControllerNumberIfNeeded, both on this device's own driver thread - the same + // thread reportControllerState arrives on - so no volatile, and no lock. + public byte batteryState; + public byte batteryPercentage; + public boolean hasBatteryReading; + @Override public void destroy() { super.destroy(); 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/UsbDriverListener.java b/app/src/main/java/com/limelight/binding/input/driver/UsbDriverListener.java index 5d76f45b..961812b9 100644 --- a/app/src/main/java/com/limelight/binding/input/driver/UsbDriverListener.java +++ b/app/src/main/java/com/limelight/binding/input/driver/UsbDriverListener.java @@ -33,7 +33,9 @@ void reportControllerState(int controllerId, int buttonFlags, /** * Reports a change in the controller's battery. Sent only when the level actually moves, not - * on a timer, so implementations should not expect a steady cadence. + * on a timer, so implementations should not expect a steady cadence. It may also arrive before + * the controller's first input report, so an implementation that needs a player number for it + * has to hold on to it. * * @param batteryState one of {@code MoonBridge.LI_BATTERY_STATE_*} * @param batteryPercentage 0 to 100, or {@code MoonBridge.LI_BATTERY_PERCENTAGE_UNKNOWN} 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/java/com/limelight/utils/DialogChain.java b/app/src/main/java/com/limelight/utils/DialogChain.java new file mode 100644 index 00000000..33bd4998 --- /dev/null +++ b/app/src/main/java/com/limelight/utils/DialogChain.java @@ -0,0 +1,35 @@ +package com.limelight.utils; + +/** + * Which dialog in a chain of replacements is the live one. + * + *

The in-game menu is a chain of {@link MenuDialog}s: each submenu is its own dialog, opened + * from a row of the one above, and Back reopens the parent as yet another new dialog. Something + * shown for the whole menu — the battery label — has to go away when the chain ends, not + * when any one dialog does, and the framework's ordering makes that harder than it sounds: + * {@code AlertController} runs a row's click listener before it dismisses the dialog, and + * {@code onDismiss} is delivered as a posted message. So the child opens first and only then does + * the parent's {@code onDismiss} arrive. Hiding on every dismiss hid the label the moment any + * submenu opened. + * + *

Each dialog takes a token when it opens; its {@code onDismiss} asks whether it is still the + * latest. Only the last dialog in the chain is, so only it ends the chain. + * + *

Main thread only — the menu opens and dismisses there and nowhere else. + */ +public final class DialogChain { + private int generation; + + /** Records a dialog opening. @return the token its {@code onDismiss} must present. */ + public int opened() { + return ++generation; + } + + /** + * @return true if no dialog has opened since {@code token} was issued, which is to say this + * dismissal ends the chain + */ + public boolean closes(int token) { + return token == generation; + } +} diff --git a/app/src/main/java/com/limelight/utils/MenuDialog.java b/app/src/main/java/com/limelight/utils/MenuDialog.java index 6f2f8d31..64c6cc7f 100644 --- a/app/src/main/java/com/limelight/utils/MenuDialog.java +++ b/app/src/main/java/com/limelight/utils/MenuDialog.java @@ -57,7 +57,9 @@ public static void show(Activity activity, String title, List

{@code MoonBridge.LI_BATTERY_*} are compile-time constants, so referencing them here inlines + * them and never loads {@code MoonBridge}, whose static initialiser would call + * {@code System.loadLibrary}. Nothing else in that class may be touched from a test. + */ +class ControllerBatteriesTest { + + private static final byte UNKNOWN = MoonBridge.LI_BATTERY_PERCENTAGE_UNKNOWN; + + @Nested + @DisplayName("classify()") + class Classification { + + /** + * The whole table. The unknown-percentage rows are the ones worth reading: a GIP pad with + * no battery pack reports {@code NOT_CHARGING} and no percentage, and the only fact in + * that is the cable, which is still worth a word; anything else without a percentage + * says nothing. Android can report {@code UNKNOWN} with a real capacity, which is a + * level, not nothing. + */ + @ParameterizedTest(name = "state {0} percentage {1} -> {2}") + @CsvSource({ + "2, 75, LEVEL", // discharging + "5, 100, LEVEL", // full + "0, 40, LEVEL", // unknown state, known level + "4, 60, LEVEL", // not charging, known level + "3, 50, CHARGING", + "4, -1, WIRED", // not charging, no level: a pad on its cable + "3, -1, WIRED", // charging, no level + "0, -1, HIDDEN", + "2, -1, HIDDEN", + "1, 50, HIDDEN", // not present + }) + @DisplayName("each reading classifies as the table says") + void classifiesAsTheTableSays(byte state, byte percentage, Display expected) { + assertEquals(expected, ControllerBatteries.classify(state, percentage)); + } + } + + @Nested + @DisplayName("snapshot()") + class Snapshot { + + @Test + @DisplayName("an empty store yields an empty list") + void emptyStoreYieldsEmptyList() { + assertTrue(new ControllerBatteries().snapshot().isEmpty()); + } + + /** + * Several pads at once is the case the label exists for. They come back in player order + * whatever order they reported in, each with its own display kind. + */ + @Test + @DisplayName("mixed controllers come back in player order") + void mixedControllersComeBackInPlayerOrder() { + ControllerBatteries batteries = new ControllerBatteries(); + batteries.update(3, MoonBridge.LI_BATTERY_STATE_NOT_CHARGING, UNKNOWN); + batteries.update(0, MoonBridge.LI_BATTERY_STATE_DISCHARGING, (byte) 75); + batteries.update(2, MoonBridge.LI_BATTERY_STATE_CHARGING, (byte) 50); + + assertEquals(List.of( + new Reading(0, Display.LEVEL, 75), + new Reading(2, Display.CHARGING, 50), + new Reading(3, Display.WIRED, UNKNOWN & 0xFF)), + batteries.snapshot()); + } + + /** A reading with nothing to say is not a row; the label would only print "unknown". */ + @Test + @DisplayName("readings with nothing to show are excluded") + void unknownReadingsAreExcluded() { + ControllerBatteries batteries = new ControllerBatteries(); + batteries.update(0, MoonBridge.LI_BATTERY_STATE_DISCHARGING, UNKNOWN); + batteries.update(1, MoonBridge.LI_BATTERY_STATE_NOT_PRESENT, (byte) 50); + + assertTrue(batteries.snapshot().isEmpty()); + } + + /** A pad that unplugged must not linger on the label under its old number. */ + @Test + @DisplayName("a cleared controller disappears") + void removedControllerDisappears() { + ControllerBatteries batteries = new ControllerBatteries(); + batteries.update(1, MoonBridge.LI_BATTERY_STATE_DISCHARGING, (byte) 75); + batteries.clear(1); + + assertTrue(batteries.snapshot().isEmpty()); + } + + /** The number a pad vacated goes to the next pad; the slot must take the new reading. */ + @Test + @DisplayName("a re-added controller reuses its slot") + void reAddedControllerReusesItsSlot() { + ControllerBatteries batteries = new ControllerBatteries(); + batteries.update(1, MoonBridge.LI_BATTERY_STATE_DISCHARGING, (byte) 75); + batteries.clear(1); + batteries.update(1, MoonBridge.LI_BATTERY_STATE_CHARGING, (byte) 20); + + assertEquals(List.of(new Reading(1, Display.CHARGING, 20)), batteries.snapshot()); + } + + @Test + @DisplayName("the latest update wins") + void latestUpdateWins() { + ControllerBatteries batteries = new ControllerBatteries(); + batteries.update(0, MoonBridge.LI_BATTERY_STATE_DISCHARGING, (byte) 75); + batteries.update(0, MoonBridge.LI_BATTERY_STATE_DISCHARGING, (byte) 74); + + assertEquals(List.of(new Reading(0, Display.LEVEL, 74)), batteries.snapshot()); + } + + /** + * The percentage travels as a Java byte, which is signed. 100 fits, but the arithmetic + * that reads it back must not sign-extend anything, or a value over 127 - which the + * unknown marker is - would come back negative. + */ + @Test + @DisplayName("the percentage is read unsigned") + void percentageIsUnsigned() { + ControllerBatteries batteries = new ControllerBatteries(); + batteries.update(0, MoonBridge.LI_BATTERY_STATE_FULL, (byte) 100); + batteries.update(1, MoonBridge.LI_BATTERY_STATE_CHARGING, UNKNOWN); + + List readings = batteries.snapshot(); + assertEquals(100, readings.get(0).percentage()); + assertEquals(0xFF, readings.get(1).percentage()); + } + + /** + * Fed from driver threads, where an exception takes the pad's read loop down. A number + * outside the slots is dropped, not thrown on, and leaves nothing behind. + */ + @Test + @DisplayName("out-of-range controller numbers are ignored") + void outOfRangeNumbersAreIgnored() { + ControllerBatteries batteries = new ControllerBatteries(); + batteries.update(-1, MoonBridge.LI_BATTERY_STATE_DISCHARGING, (byte) 75); + batteries.update(ControllerBatteries.MAX_CONTROLLERS, + MoonBridge.LI_BATTERY_STATE_DISCHARGING, (byte) 75); + batteries.clear(-1); + batteries.clear(ControllerBatteries.MAX_CONTROLLERS); + + assertTrue(batteries.snapshot().isEmpty()); + } + } +} 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"); } } diff --git a/app/src/test/java/com/limelight/utils/DialogChainTest.java b/app/src/test/java/com/limelight/utils/DialogChainTest.java new file mode 100644 index 00000000..91a39e38 --- /dev/null +++ b/app/src/test/java/com/limelight/utils/DialogChainTest.java @@ -0,0 +1,72 @@ +package com.limelight.utils; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Tests for the token that tells the last dialog in the in-game menu's chain from the rest. + * + *

Each scenario is a sequence the menu actually produces; the order of opens and dismissals + * follows the framework's, where a row's action opens the child before the parent's dismissal is + * delivered. + */ +class DialogChainTest { + + /** Cancel on the root, or Back at the root: nothing else opened, so this ends the chain. */ + @Test + @DisplayName("dismissing the only dialog ends the chain") + void rootDismissEndsTheChain() { + DialogChain chain = new DialogChain(); + int root = chain.opened(); + + assertTrue(chain.closes(root)); + } + + /** + * A row opens a submenu. The child has opened by the time the parent's dismissal arrives, so + * the parent must not end the chain - that is the case that hid the label the moment any + * submenu opened. + */ + @Test + @DisplayName("a parent's dismissal after its child opened does not end the chain") + void parentDismissAfterChildOpensDoesNotEndIt() { + DialogChain chain = new DialogChain(); + int root = chain.opened(); + int child = chain.opened(); + + assertFalse(chain.closes(root)); + assertTrue(chain.closes(child)); + } + + /** + * Back from a submenu reopens the parent as a new dialog, not the old one. The reopened + * parent is then the live dialog and the submenu's own dismissal must not end the chain. + */ + @Test + @DisplayName("reopening the parent on Back is a new generation") + void backReopensParentAsNewGeneration() { + DialogChain chain = new DialogChain(); + chain.opened(); // root + int child = chain.opened(); + int reopenedRoot = chain.opened(); // Back: root again, as a fresh dialog + + assertFalse(chain.closes(child)); + assertTrue(chain.closes(reopenedRoot)); + } + + /** A token from further back in the chain never closes it, however many opens later. */ + @Test + @DisplayName("a stale token never ends the chain") + void staleTokenNeverCloses() { + DialogChain chain = new DialogChain(); + int first = chain.opened(); + for (int i = 0; i < 5; i++) { + chain.opened(); + } + + assertFalse(chain.closes(first)); + } +} From 9ba19dcc10458791f3260aeff15502cfd11abaed Mon Sep 17 00:00:00 2001 From: Turan Asikoglu <874158+trexx@users.noreply.github.com> Date: Wed, 16 Sep 2026 22:19:13 +0200 Subject: [PATCH 2/2] Name pads in the audio menu by their host player number The pad-audio submenu counted GIP pads by their position in the driver's list, while the battery label - and the game on the host - use the player number ControllerHandler assigns. The two disagreed whenever the Shield controller or a Bluetooth pad held a slot: the first GIP pad read "Controller 1" in the menu and was player 2 everywhere else. The number now lives on the AbstractController too, written when the handler assigns it and cleared when the pad goes, so the menu - which holds the controller and nothing else - can read it without reaching into the handler's maps from the UI thread. GameMenuLayout.padNumber() is the rule, tested: the player number, or "Unused controller" for a pad that has not sent input yet, or the position when multi-controller is off and every pad is player 0. Numbering is still assigned on a pad's first input, deliberately. Assigning at deviceAdded() was considered and dropped: the USB service binds in onCreate(), and LiSendControllerArrivalEvent() returns -2 until the stream's input channel exists with nothing re-sending it, so a pad attached at launch would have been announced into the void and the host would never learn its type or capabilities. Latency: none; the menu builds its rows once per open. Co-Authored-By: Claude Opus 5 --- HARDWARE_TESTING.md | 8 +++-- app/src/main/java/com/limelight/Game.java | 10 ++++-- app/src/main/java/com/limelight/GameMenu.java | 29 ++++++++++++--- .../java/com/limelight/GameMenuLayout.java | 27 ++++++++++++++ .../binding/input/ControllerHandler.java | 19 +++++++--- .../input/driver/AbstractController.java | 25 +++++++++++++ app/src/main/res/values/strings.xml | 2 ++ .../com/limelight/GameMenuLayoutTest.java | 36 +++++++++++++++++++ 8 files changed, 142 insertions(+), 14 deletions(-) diff --git a/HARDWARE_TESTING.md b/HARDWARE_TESTING.md index 05b51a70..1468d495 100644 --- a/HARDWARE_TESTING.md +++ b/HARDWARE_TESTING.md @@ -2819,8 +2819,12 @@ The open hardware questions: colour rather than the secondary one the settings version label uses for exactly this reason. If it is still too faint from the sofa, that is a colour change, not a dim change. - [ ] **Several controllers, in player order.** Both pads listed, numbered as the host numbers - them. Note the pad-audio submenu numbers GIP pads by their position in the driver's list - instead, so the two can disagree when an Android-enumerated pad is also present. + them. The pad-audio submenu now uses the same numbers (it used to count pads by their + position in the driver's list, which disagreed whenever the Shield controller or a + Bluetooth pad held a slot): with the Shield controller as player 1, a GIP pad should read + "Controller 2" in both places. A paired pad nobody has touched reads "Unused controller" in + the audio menu until it sends input, and is absent from the label. With multi-controller + off the audio menu keeps positional numbers, since every pad is player 0 there. - [ ] **The label survives the menu's own navigation.** Open Controllers, then Back: the label stays. Cancel, or Back at the root: the label goes. Choose Disconnect: the label goes. - [ ] **A pad that unplugs mid-menu is gone on the next open.** Its number is released, which diff --git a/app/src/main/java/com/limelight/Game.java b/app/src/main/java/com/limelight/Game.java index 9f0e0330..50a4f793 100644 --- a/app/src/main/java/com/limelight/Game.java +++ b/app/src/main/java/com/limelight/Game.java @@ -2498,9 +2498,8 @@ public void showGameMenu(GameInputDevice device) { * no controller has anything to show. One entry per controller, in player order, so several * pads read side by side rather than the last one reported winning. * - *

Numbered as the host numbers them - player 1 is controller number 0 - which is not the - * order the pad-audio submenu uses; that lists GIP pads by their position in the driver's - * list, so the two can disagree when an Android-enumerated pad is also attached. + *

Numbered as the host numbers them - player 1 is controller number 0 - which is also how + * the pad-audio submenu names pads, so a number here is the same pad there. * *

UI thread only: the menu opens from {@link #onBackPressed} or a controller button on the * main thread and calls this before showing each level. The label is a child of this @@ -2572,6 +2571,11 @@ public PadAudioSink getPadAudioSink() { return padAudioSink; } + /** @return the multi-controller setting, which decides how the game menu numbers pads */ + public boolean isMultiControllerEnabled() { + return prefConfig.multiController; + } + /** * Turns audio to a pad's headphone jack on or off mid-stream, reporting the outcome. A refusal * is either the two-pad bandwidth limit or a pad with no audio endpoint at all, and the toast diff --git a/app/src/main/java/com/limelight/GameMenu.java b/app/src/main/java/com/limelight/GameMenu.java index 3d15f5ec..31931d26 100644 --- a/app/src/main/java/com/limelight/GameMenu.java +++ b/app/src/main/java/com/limelight/GameMenu.java @@ -13,6 +13,7 @@ import com.limelight.utils.MenuDialog; import java.util.ArrayList; +import java.util.Comparator; import java.util.List; /** @@ -170,14 +171,33 @@ private void showMenuDialog(String title, MenuOption[] options, Runnable onBack) * cannot be enabled because the two-pad limit is reached says so on the row. The cap is a * bandwidth budget on a link shared with controller input, so it is worth showing rather * than letting a selection quietly do nothing. + * + *

Pads are named by the host's player number - see {@link GameMenuLayout#padNumber} for + * why, and for the two cases that cannot be - and listed in that order, with any pad that + * has no number yet at the end. */ private void showPadAudioMenu() { List controllers = game.getGipControllers(); PadAudioSink sink = game.getPadAudioSink(); List options = new ArrayList<>(); + boolean multiController = game.isMultiControllerEnabled(); - int number = 1; - for (GipController controller : controllers) { + // Number before sorting: the position fallback is the driver's order, which is this one + var numbers = new int[controllers.size()]; + for (int i = 0; i < controllers.size(); i++) { + numbers[i] = GameMenuLayout.padNumber(multiController, + controllers.get(i).getPlayerNumber(), i + 1); + } + var order = new ArrayList(); + for (int i = 0; i < controllers.size(); i++) { + order.add(i); + } + order.sort(Comparator.comparingInt(i -> + numbers[i] == GameMenuLayout.PAD_UNNUMBERED ? Integer.MAX_VALUE : numbers[i])); + + for (int i : order) { + GipController controller = controllers.get(i); + int number = numbers[i]; boolean enabled = sink.isEnabled(controller); /* @@ -215,8 +235,9 @@ else if (sink.canEnableMore()) { state = getString(R.string.game_menu_pad_audio_unavailable); } - String label = game.getResources().getString( - R.string.game_menu_pad_audio_entry, number++, state); + String label = number == GameMenuLayout.PAD_UNNUMBERED + ? game.getResources().getString(R.string.game_menu_pad_audio_entry_unnumbered, state) + : game.getResources().getString(R.string.game_menu_pad_audio_entry, number, state); options.add(new MenuOption(label, () -> game.togglePadAudio(controller))); } diff --git a/app/src/main/java/com/limelight/GameMenuLayout.java b/app/src/main/java/com/limelight/GameMenuLayout.java index 4bcca225..d05504aa 100644 --- a/app/src/main/java/com/limelight/GameMenuLayout.java +++ b/app/src/main/java/com/limelight/GameMenuLayout.java @@ -115,4 +115,31 @@ public static boolean hasControllerOptions(State state) { private static boolean padAudioAvailable(State state) { return state.hasGipPads() && state.padAudioFormatSupported(); } + + /** {@link #padNumber} for a pad the host has not been told about yet. */ + public static final int PAD_UNNUMBERED = 0; + + /** + * The number a pad-audio row shows for a pad: the host's player number, so it is the number + * the game on the host uses for that pad, and the same one the battery label along the + * bottom of the screen uses. The rows used to count pads by their position in the driver's + * list, which disagreed with both whenever an Android-enumerated pad held a slot. + * + *

Two cases fall back. A pad is numbered on its first input report, not before, so a + * paired pad nobody has touched has no number yet and is reported as + * {@link #PAD_UNNUMBERED} for the row to say so - pressing anything on it numbers it. With + * multi-controller off every pad is player 0, so player numbers would make every row + * "Controller 1"; that mode keeps the positional count, which at least tells the rows apart. + * + * @param multiController the multi-controller setting + * @param playerNumber the pad's 0-based player number, or negative if it has none yet + * @param position the pad's 1-based position in the driver's list + * @return the 1-based number to show, or {@link #PAD_UNNUMBERED} + */ + public static int padNumber(boolean multiController, int playerNumber, int position) { + if (!multiController) { + return position; + } + return playerNumber < 0 ? PAD_UNNUMBERED : playerNumber + 1; + } } diff --git a/app/src/main/java/com/limelight/binding/input/ControllerHandler.java b/app/src/main/java/com/limelight/binding/input/ControllerHandler.java index a6390c5b..e47c560a 100644 --- a/app/src/main/java/com/limelight/binding/input/ControllerHandler.java +++ b/app/src/main/java/com/limelight/binding/input/ControllerHandler.java @@ -460,6 +460,10 @@ private void releaseControllerNumber(GenericControllerContext context) { batteries.clear(context.controllerNumber); } + if (context instanceof UsbDeviceContext usbContext) { + usbContext.device.setPlayerNumber(AbstractController.NO_PLAYER_NUMBER); + } + // If this device sent data as a gamepad, zero the values before removing. // We must do this after clearing the currentControllers entry so this // causes the device to be removed on the server PC. @@ -614,11 +618,16 @@ else if (!devContext.hasJoystickAxes) { // Report attributes of this new controller to the host context.sendControllerArrival(); - // A GIP pad's first status packet usually lands before its first input report, which is - // what brought us here, so its battery was cached rather than sent - see - // reportControllerBattery. Now that the pad has a number, hand it on. - if (context instanceof UsbDeviceContext usbContext && usbContext.hasBatteryReading) { - publishBattery(usbContext, usbContext.batteryState, usbContext.batteryPercentage); + if (context instanceof UsbDeviceContext usbContext) { + // The game menu names a pad by this number, and holds only the controller + usbContext.device.setPlayerNumber(context.controllerNumber); + + // A GIP pad's first status packet usually lands before its first input report, which + // is what brought us here, so its battery was cached rather than sent - see + // reportControllerBattery. Now that the pad has a number, hand it on. + if (usbContext.hasBatteryReading) { + publishBattery(usbContext, usbContext.batteryState, usbContext.batteryPercentage); + } } } diff --git a/app/src/main/java/com/limelight/binding/input/driver/AbstractController.java b/app/src/main/java/com/limelight/binding/input/driver/AbstractController.java index df5c9a45..b2b4ea70 100644 --- a/app/src/main/java/com/limelight/binding/input/driver/AbstractController.java +++ b/app/src/main/java/com/limelight/binding/input/driver/AbstractController.java @@ -24,6 +24,17 @@ public abstract class AbstractController { private final int vendorId; private final int productId; + /** {@link #getPlayerNumber()} before the host has been told about this pad. */ + public static final int NO_PLAYER_NUMBER = -1; + + // The host's player number for this pad, 0-based. Assigned by ControllerHandler on the pad's + // first input report - not before, because the arrival it sends with it is lost if the + // stream is not up yet - and cleared when the pad goes away. Written on the driver thread, + // read on the UI thread by the game menu, hence volatile. Lives here rather than only on the + // handler's context so the menu, which holds the controller and nothing else, need not reach + // into the handler's maps from a thread that does not own them. + private volatile int playerNumber = NO_PLAYER_NUMBER; + private UsbDriverListener listener; // Current controller state, written by the subclass's parser and read by reportInput(). @@ -46,6 +57,20 @@ public int getControllerId() { return deviceId; } + /** + * @return the host's player number for this pad, 0-based, or {@link #NO_PLAYER_NUMBER} until + * its first input report has been sent. It is what the game on the host calls this + * pad, so it is what the game menu calls it too. + */ + public int getPlayerNumber() { + return playerNumber; + } + + /** For {@code ControllerHandler}, which owns the numbering. */ + public void setPlayerNumber(int playerNumber) { + this.playerNumber = playerNumber; + } + /** @return the USB vendor ID of the physical device */ public int getVendorId() { return vendorId; diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 36b2e062..dab4fbd4 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -132,6 +132,8 @@ Pair Xbox Wireless Controller Controller headphone audio Controller %1$d: %2$s + + Unused controller: %1$s On Off Off (two controllers already) diff --git a/app/src/test/java/com/limelight/GameMenuLayoutTest.java b/app/src/test/java/com/limelight/GameMenuLayoutTest.java index 3ac93401..016d411d 100644 --- a/app/src/test/java/com/limelight/GameMenuLayoutTest.java +++ b/app/src/test/java/com/limelight/GameMenuLayoutTest.java @@ -165,4 +165,40 @@ void matchesTheRows(boolean fromGamepad, boolean dongle, boolean pads, new State(fromGamepad, dongle, pads, formatSupported))); } } + + @Nested + @DisplayName("padNumber") + class PadNumber { + + /** + * The player number is what the game on the host shows and what the battery label + * shows, so it is what a pad-audio row must show: a pad that is player 2 because the + * Shield controller holds slot 1 reads "Controller 2", whatever its position in the + * driver's list. The position is only a fallback. + */ + @ParameterizedTest(name = "multi={0} player={1} position={2} -> {3}") + @CsvSource({ + "true, 0, 1, 1", + "true, 1, 1, 2", // player 2 in position 1: the Shield controller is player 1 + "true, 0, 2, 1", // player 1 in position 2: touched second, paired first + "true, -1, 1, 0", // no number yet: nothing has been pressed on it + "false, 0, 1, 1", + "false, 0, 2, 2", // every pad is player 0 here, so the position tells them apart + "false, -1, 3, 3", + }) + @DisplayName("shows the player number, falling back to the position") + void showsThePlayerNumber(boolean multiController, int playerNumber, int position, + int expected) { + assertEquals(expected, GameMenuLayout.padNumber(multiController, playerNumber, position)); + } + + /** Zero is the sentinel because rows count from one; a real number can never be zero. */ + @Test + @DisplayName("the unnumbered sentinel is not a row number") + void unnumberedIsNotARowNumber() { + assertEquals(0, GameMenuLayout.PAD_UNNUMBERED); + assertTrue(GameMenuLayout.padNumber(true, 0, 1) > GameMenuLayout.PAD_UNNUMBERED); + assertTrue(GameMenuLayout.padNumber(false, -1, 1) > GameMenuLayout.PAD_UNNUMBERED); + } + } }