Player LED behavior is wrong

[SuperSamus]

(Copied from https://bugzilla.kernel.org/show_bug.cgi?id=216225)

The behavior of the Nintendo Switch Pro Controller player LED on Linux is this: the first time it's connected the first led turns on, the second time it's connected the first two LEDs (not just the second) turn on, the third time the first three LEDs (not just the third) turn on, the fourth time all LEDs (not just the fourth) turn on, and then it cycles back to the beginning.

This is completely different from the behavior when connected to the Switch console, where only one player LED ever turns on, and it's the one associated with the player (first LED for player one, second LED for player two...), not the number of times it's been connected.

Joycond does fix this, but there shouldn't be the need of using a daemon for something simple like this.

I don't know if from the kernel it's possible to get the "player number", but it would still be an improvement to only have one LED turning on.

Tested on a Pro Controller 050000007e0500000920000001800000, don't own a Joy-Con.

[SuperSamus]

I gave a look to hid-nintendo.c to see why it behaves like this. From what I think I've gathered:

The static variable that controls the LED number is input_num. It increases by 1 (and resets to 1 when it reaches 5) every time after LEDs are initialized (joycon_leds_create). It never decreases. Finally, the reason why multiple LEDs are turned on instead of just one, seems to be in led->brightness = ((i + 1) <= input_num) ? 1 : 0; (it's a <= instead of a ==).

Instead of doing this, would it be possible to have a static variable that increases by 1 when a controller is connected (nintendo_hid_probe) and decreases by 1 when disconnected nintendo_hid_remove? (With the necessary mutexes, of course.)

I don't know how to test kernel stuff, so that's why I'm just leaving this comment.

[SuperSamus]

I just realized that using one global counter would have a problem. If two controllers are connected, player one disconnects, then a controller is connected, they will both be "player two" according to the leds.

The solution would be to store the controllers in a list, and when a controller disconnects either:

• All the player of a higher number will shift to the left, and the leds updated accordingly;
• Leave an empty slot. When a controller is connected, it takes the first empty slot available (this is what's done on the Switch console).

[JohnoKing]

I have begun working on a patch to implement this LED number functionality. The patch is available below for testing (patch updated on 2022-10-13; more testing is needed):

wip-player-led-fix.patch

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 5bfc0c450460..3eecc48ef02d 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -176,6 +176,12 @@
 #define JC_IMU_GYRO_FUZZ		10
 #define JC_IMU_GYRO_FLAT		0
 
+/* bits corresponding to controller LEDs */
+#define JC_LED_ONE   (1 << 0)
+#define JC_LED_TWO   (1 << 1)
+#define JC_LED_THREE (1 << 2)
+#define JC_LED_FOUR  (1 << 3)
+
 /* frequency/amplitude tables for rumble */
 struct joycon_rumble_freq_data {
 	u16 high;
@@ -419,6 +425,7 @@ struct joycon_ctlr {
 	struct led_classdev home_led;
 	enum joycon_ctlr_state ctlr_state;
 	spinlock_t lock;
+	u8 controller_num;
 	u8 mac_addr[6];
 	char *mac_addr_str;
 	enum joycon_ctlr_type ctlr_type;
@@ -1836,6 +1843,8 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
 }
 
 static DEFINE_MUTEX(joycon_input_num_mutex);
+static u8 input_num[9];
+
 static int joycon_leds_create(struct joycon_ctlr *ctlr)
 {
 	struct hid_device *hdev = ctlr->hdev;
@@ -1845,17 +1854,69 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
 	char *name;
 	int ret = 0;
 	int i;
-	static int input_num = 1;
+	u8 val;
 
-	/* Set the default controller player leds based on controller number */
 	mutex_lock(&joycon_input_num_mutex);
 	mutex_lock(&ctlr->output_mutex);
-	ret = joycon_set_player_leds(ctlr, 0, 0xF >> (4 - input_num));
+
+	/*
+	 * Use the first available controller number. Extra controllers
+	 * max out at a ninth slot and use the same LED order.
+	 */
+	for (i = 0; i < 9; i++) {
+		if (!input_num[i] || i == 9) {
+			/* handle possible (but very unlikely) overflow) */
+			if (unlikely(input_num[i] == U8_MAX))
+				return -EOVERFLOW;
+			input_num[i]++;
+			break;
+		}
+	}
+	ctlr->controller_num = i;
+
+	/*
+	 * Set the player LEDs according to the controller number. The
+	 * LEDs for players one through eight use the same order as
+	 * the Nintendo Switch:
+	 * https://en-americas-support.nintendo.com/app/answers/detail/a_id/22424/~/controller-pairing-faq#s1q4
+	 */
+	switch (ctlr->controller_num + 1) {
+	case 1:  /* player one:    ■ □ □ □ */
+	case 2:  /* player two:    ■ ■ □ □ */
+	case 3:  /* player three:  ■ ■ ■ □ */
+	case 4:  /* player four:   ■ ■ ■ ■ */
+		val = 0xF >> (4 - (ctlr->controller_num + 1));
+		break;
+	case 5:  /* player five:   ■ □ ■ ■ */
+		val = JC_LED_ONE | JC_LED_FOUR;
+		break;
+	case 6:  /* player six:    ■ □ ■ □ */
+		val = JC_LED_ONE | JC_LED_THREE;
+		break;
+	case 7:  /* player seven:  ■ □ ■ ■ */
+		val = JC_LED_ONE | JC_LED_THREE | JC_LED_FOUR;
+		break;
+	case 8:  /* player eight:  □ ■ ■ □ */
+		val = JC_LED_TWO | JC_LED_THREE;
+		break;
+	default: /* players nine+: □ ■ ■ ■ */
+		val = JC_LED_TWO | JC_LED_THREE | JC_LED_FOUR;
+	}
+	ret = joycon_set_player_leds(ctlr, 0, val);
 	if (ret)
-		hid_warn(ctlr->hdev, "Failed to set leds; ret=%d\n", ret);
+		hid_warn(ctlr->hdev, "Failed to set LEDs; ret=%d\n", ret);
 	mutex_unlock(&ctlr->output_mutex);
 
 	/* configure the player LEDs */
+	if (val & JC_LED_ONE)
+		ctlr->leds[0].brightness = 1;
+	if (val & JC_LED_TWO)
+		ctlr->leds[1].brightness = 1;
+	if (val & JC_LED_THREE)
+		ctlr->leds[2].brightness = 1;
+	if (val & JC_LED_FOUR)
+		ctlr->leds[3].brightness = 1;
+
 	for (i = 0; i < JC_NUM_LEDS; i++) {
 		name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s:%s",
 				      d_name,
@@ -1868,7 +1929,6 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
 
 		led = &ctlr->leds[i];
 		led->name = name;
-		led->brightness = ((i + 1) <= input_num) ? 1 : 0;
 		led->max_brightness = 1;
 		led->brightness_set_blocking =
 					joycon_player_led_brightness_set;
@@ -1881,9 +1941,6 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
 			return ret;
 		}
 	}
-
-	if (++input_num > 4)
-		input_num = 1;
 	mutex_unlock(&joycon_input_num_mutex);
 
 	/* configure the home LED */
@@ -2295,6 +2352,11 @@ static void nintendo_hid_remove(struct hid_device *hdev)
 
 	hid_dbg(hdev, "remove\n");
 
+	/* Free up the player LED for later reuse */
+	mutex_lock(&joycon_input_num_mutex);
+	input_num[ctlr->controller_num]--;
+	mutex_unlock(&joycon_input_num_mutex);
+
 	/* Prevent further attempts at sending subcommands. */
 	spin_lock_irqsave(&ctlr->lock, flags);
 	ctlr->ctlr_state = JOYCON_CTLR_STATE_REMOVED;

[SuperSamus]

Fixed in Linux 6.10.