[PATCH v0 0/2] HID: nintendo: avoid BT rumble disconnections

[PATCH v0 0/2] HID: nintendo: avoid BT rumble disconnections

[Daniel J. Ogorchock]

The hid-nintendo driver has been plagued by an issue where rumble
command traffic to bluetooth-connected controllers can cause frequent
power downs of the controllers.

Digging into other pro controller driver implementations, I've not found
anything that hid-nintendo is doing incorrectly. Some implementations
seem to be working around the same problem (e.g. libsdl's hidapi
implementation rate limits rumble commands to avoid the problem).

hid-nintendo already rate limits rumble control packets, but that does
not solve the problem.

Using btmon output, I've fuond the the disconnections reliably occur
shortly after the controller's reporting rate become erratic. The
controller is meant to report input packets roughly every 15ms. Sending
rumble commands can sometimes result in the input packets arriving in
bursts/batches. Once the controller and/or BT stack enters this state,
even halting rumble commands will never allow the reporting rate to
recover to nominal. The controller will eventually disconnect.

This patch set strives to avoid the problematic scenario. It detects if
input reports arrive at clearly incorrect deltas. During these times,
the driver will hold off on transmitting any rumble commands to the
controller. This approach has allowed the reporting rate to reliably
recover in my testing. I've not been able to generate a controller
disconnection during hours of testing games with frequent rumble.

The behavior of this mechanism is tunable using #defines. We may need to
tweak/tune as the mitigation is used on different hardware setups.

My suspicion is that the core issue is somewhere in the bluez stack. My
next step is to investigate that lead in more detail. This patchset at
least allows for use of the controllers via bluetooth with rumble
enabled without frequently disconnecting.

Daniel J. Ogorchock (2):
  HID: nintendo: prevent rumble queue overruns
  HID: nintendo: fix rumble rate limiter

 drivers/hid/hid-nintendo.c | 95 ++++++++++++++++++++++++++++++++++----
 1 file changed, 86 insertions(+), 9 deletions(-)

-- 
2.39.1

[PATCH v0 1/2] HID: nintendo: prevent rumble queue overruns

[Daniel J. Ogorchock]

Make sure that we never throw out the most recent rumble setting,
opting to overwrite the prior queue head instead. This prevents
instances where we could get rumble stuck on if there were an overrun at
the wrong time.

Signed-off-by: Daniel J. Ogorchock <djogorchock@gmail.com>
---
 drivers/hid/hid-nintendo.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 5bfc0c4504608..2b781cc9082b4 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -1527,6 +1527,7 @@ static int joycon_set_rumble(struct joycon_ctlr *ctlr, u16 amp_r, u16 amp_l,
 	u16 freq_l_low;
 	u16 freq_l_high;
 	unsigned long flags;
+	int next_rq_head;
 
 	spin_lock_irqsave(&ctlr->lock, flags);
 	freq_r_low = ctlr->rumble_rl_freq;
@@ -1547,8 +1548,21 @@ static int joycon_set_rumble(struct joycon_ctlr *ctlr, u16 amp_r, u16 amp_l,
 	joycon_encode_rumble(data, freq_l_low, freq_l_high, amp);
 
 	spin_lock_irqsave(&ctlr->lock, flags);
-	if (++ctlr->rumble_queue_head >= JC_RUMBLE_QUEUE_SIZE)
-		ctlr->rumble_queue_head = 0;
+
+	next_rq_head = ctlr->rumble_queue_head + 1;
+	if (next_rq_head >= JC_RUMBLE_QUEUE_SIZE)
+		next_rq_head = 0;
+
+	/* Did we overrun the circular buffer?
+	 * If so, be sure we keep the latest intended rumble state.
+	 */
+	if (next_rq_head == ctlr->rumble_queue_tail) {
+		hid_dbg(ctlr->hdev, "rumble queue is full");
+		/* overwrite the prior value at the end of the circular buf */
+		next_rq_head = ctlr->rumble_queue_head;
+	}
+
+	ctlr->rumble_queue_head = next_rq_head;
 	memcpy(ctlr->rumble_data[ctlr->rumble_queue_head], data,
 	       JC_RUMBLE_DATA_SIZE);
 
@@ -2128,7 +2142,7 @@ static int nintendo_hid_probe(struct hid_device *hdev,
 
 	ctlr->hdev = hdev;
 	ctlr->ctlr_state = JOYCON_CTLR_STATE_INIT;
-	ctlr->rumble_queue_head = JC_RUMBLE_QUEUE_SIZE - 1;
+	ctlr->rumble_queue_head = 0;
 	ctlr->rumble_queue_tail = 0;
 	hid_set_drvdata(hdev, ctlr);
 	mutex_init(&ctlr->output_mutex);
-- 
2.39.1

[PATCH v0 2/2] HID: nintendo: fix rumble rate limiter

[Daniel J. Ogorchock]

It's been discovered that BT controller disconnect events correlate to
erratic input report timestamp deltas.

In experimentation, it's been found that ensuring that multiple
timestamp deltas are consistent prior to transmitting a rumble packet
drastically reduces the occurence rate of BT disconnects.

Alter the joycon_enforce_subcmd_rate() function to use this new
approach.

Signed-off-by: Daniel J. Ogorchock <djogorchock@gmail.com>
---
 drivers/hid/hid-nintendo.c | 75 +++++++++++++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 2b781cc9082b4..250f5d2f888ab 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -433,7 +433,9 @@ struct joycon_ctlr {
 	u8 usb_ack_match;
 	u8 subcmd_ack_match;
 	bool received_input_report;
+	unsigned int last_input_report_msecs;
 	unsigned int last_subcmd_sent_msecs;
+	unsigned int consecutive_valid_report_deltas;
 
 	/* factory calibration data */
 	struct joycon_stick_cal left_stick_cal_x;
@@ -543,19 +545,54 @@ static void joycon_wait_for_input_report(struct joycon_ctlr *ctlr)
  * Sending subcommands and/or rumble data at too high a rate can cause bluetooth
  * controller disconnections.
  */
+#define JC_INPUT_REPORT_MIN_DELTA	8
+#define JC_INPUT_REPORT_MAX_DELTA	17
+#define JC_SUBCMD_TX_OFFSET_MS		4
+#define JC_SUBCMD_VALID_DELTA_REQ	3
+#define JC_SUBCMD_RATE_MAX_ATTEMPTS	500
+#define JC_SUBCMD_RATE_LIMITER_USB_MS	20
+#define JC_SUBCMD_RATE_LIMITER_BT_MS	60
+#define JC_SUBCMD_RATE_LIMITER_MS(ctlr)	((ctlr)->hdev->bus == BUS_USB ? JC_SUBCMD_RATE_LIMITER_USB_MS : JC_SUBCMD_RATE_LIMITER_BT_MS)
 static void joycon_enforce_subcmd_rate(struct joycon_ctlr *ctlr)
 {
-	static const unsigned int max_subcmd_rate_ms = 25;
-	unsigned int current_ms = jiffies_to_msecs(jiffies);
-	unsigned int delta_ms = current_ms - ctlr->last_subcmd_sent_msecs;
+	unsigned int current_ms;
+	unsigned long subcmd_delta;
+	int consecutive_valid_deltas = 0;
+	int attempts = 0;
+	unsigned long flags;
+
+	if (unlikely(ctlr->ctlr_state != JOYCON_CTLR_STATE_READ))
+		return;
 
-	while (delta_ms < max_subcmd_rate_ms &&
-	       ctlr->ctlr_state == JOYCON_CTLR_STATE_READ) {
+	do {
 		joycon_wait_for_input_report(ctlr);
 		current_ms = jiffies_to_msecs(jiffies);
-		delta_ms = current_ms - ctlr->last_subcmd_sent_msecs;
+		subcmd_delta = current_ms - ctlr->last_subcmd_sent_msecs;
+
+		spin_lock_irqsave(&ctlr->lock, flags);
+		consecutive_valid_deltas = ctlr->consecutive_valid_report_deltas;
+		spin_unlock_irqrestore(&ctlr->lock, flags);
+
+		attempts++;
+	} while ((consecutive_valid_deltas < JC_SUBCMD_VALID_DELTA_REQ ||
+		  subcmd_delta < JC_SUBCMD_RATE_LIMITER_MS(ctlr)) &&
+		 ctlr->ctlr_state == JOYCON_CTLR_STATE_READ &&
+		 attempts < JC_SUBCMD_RATE_MAX_ATTEMPTS);
+
+	if (attempts >= JC_SUBCMD_RATE_MAX_ATTEMPTS) {
+		hid_warn(ctlr->hdev, "%s: exceeded max attempts", __func__);
+		return;
 	}
+
 	ctlr->last_subcmd_sent_msecs = current_ms;
+
+	/*
+	 * Wait a short time after receiving an input report before
+	 * transmitting. This should reduce odds of a TX coinciding with an RX.
+	 * Minimizing concurrent BT traffic with the controller seems to lower
+	 * the rate of disconnections.
+	 */
+	msleep(JC_SUBCMD_TX_OFFSET_MS);
 }
 
 static int joycon_hid_send_sync(struct joycon_ctlr *ctlr, u8 *data, size_t len,
@@ -1223,6 +1260,7 @@ static void joycon_parse_report(struct joycon_ctlr *ctlr,
 	u8 tmp;
 	u32 btns;
 	unsigned long msecs = jiffies_to_msecs(jiffies);
+	unsigned long report_delta_ms = msecs - ctlr->last_input_report_msecs;
 
 	spin_lock_irqsave(&ctlr->lock, flags);
 	if (IS_ENABLED(CONFIG_NINTENDO_FF) && rep->vibrator_report &&
@@ -1364,6 +1402,31 @@ static void joycon_parse_report(struct joycon_ctlr *ctlr,
 
 	input_sync(dev);
 
+	spin_lock_irqsave(&ctlr->lock, flags);
+	ctlr->last_input_report_msecs = msecs;
+	/*
+	 * Was this input report a reasonable time delta compared to the prior
+	 * report? We use this information to decide when a safe time is to send
+	 * rumble packets or subcommand packets.
+	 */
+	if (report_delta_ms >= JC_INPUT_REPORT_MIN_DELTA &&
+	    report_delta_ms <= JC_INPUT_REPORT_MAX_DELTA) {
+		if (ctlr->consecutive_valid_report_deltas < JC_SUBCMD_VALID_DELTA_REQ)
+			ctlr->consecutive_valid_report_deltas++;
+	} else {
+		ctlr->consecutive_valid_report_deltas = 0;
+	}
+	/*
+	 * Our consecutive valid report tracking is only relevant for
+	 * bluetooth-connected controllers. For USB devices, we're beholden to
+	 * USB's underlying polling rate anyway. Always set to the consecutive
+	 * delta requirement.
+	 */
+	if (ctlr->hdev->bus == BUS_USB)
+		ctlr->consecutive_valid_report_deltas = JC_SUBCMD_VALID_DELTA_REQ;
+
+	spin_unlock_irqrestore(&ctlr->lock, flags);
+
 	/*
 	 * Immediately after receiving a report is the most reliable time to
 	 * send a subcommand to the controller. Wake any subcommand senders
-- 
2.39.1

Re: [PATCH v0 0/2] HID: nintendo: avoid BT rumble disconnections

[Silvan Jegen]

Hi

"Daniel J. Ogorchock" <djogorchock@gmail.com> wrote:
> The hid-nintendo driver has been plagued by an issue where rumble
> command traffic to bluetooth-connected controllers can cause frequent
> power downs of the controllers.
> 
> Digging into other pro controller driver implementations, I've not found
> anything that hid-nintendo is doing incorrectly. Some implementations
> seem to be working around the same problem (e.g. libsdl's hidapi
> implementation rate limits rumble commands to avoid the problem).
> 
> [...]
> 
> Daniel J. Ogorchock (2):
>   HID: nintendo: prevent rumble queue overruns
>   HID: nintendo: fix rumble rate limiter
> 
>  drivers/hid/hid-nintendo.c | 95 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 86 insertions(+), 9 deletions(-)

I haven't tested this but the code of this series looks good to me.

Reviewed-by: Silvan Jegen <s.jegen@gmail.com>

Re: [PATCH v0 0/2] HID: nintendo: avoid BT rumble disconnections

[Silvan Jegen]

"Daniel J. Ogorchock" <djogorchock@gmail.com> wrote:
> The hid-nintendo driver has been plagued by an issue where rumble
> command traffic to bluetooth-connected controllers can cause frequent
> power downs of the controllers.
> 
> [...] 
> 
> My suspicion is that the core issue is somewhere in the bluez stack. My
> next step is to investigate that lead in more detail. This patchset at
> least allows for use of the controllers via bluetooth with rumble
> enabled without frequently disconnecting.
> 
> Daniel J. Ogorchock (2):
>   HID: nintendo: prevent rumble queue overruns
>   HID: nintendo: fix rumble rate limiter
> 
>  drivers/hid/hid-nintendo.c | 95 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 86 insertions(+), 9 deletions(-)

I haven't tested this but the code looks good to me.

Reviewed-by: Silvan Jegen <s.jegen@gmail.com>

Re: [PATCH v0 0/2] HID: nintendo: avoid BT rumble disconnections

[Jiri Kosina]

On Fri, 3 Feb 2023, Daniel J. Ogorchock wrote:

> The hid-nintendo driver has been plagued by an issue where rumble
> command traffic to bluetooth-connected controllers can cause frequent
> power downs of the controllers.
> 
> Digging into other pro controller driver implementations, I've not found
> anything that hid-nintendo is doing incorrectly. Some implementations
> seem to be working around the same problem (e.g. libsdl's hidapi
> implementation rate limits rumble commands to avoid the problem).
> 
> hid-nintendo already rate limits rumble control packets, but that does
> not solve the problem.
> 
> Using btmon output, I've fuond the the disconnections reliably occur
> shortly after the controller's reporting rate become erratic. The
> controller is meant to report input packets roughly every 15ms. Sending
> rumble commands can sometimes result in the input packets arriving in
> bursts/batches. Once the controller and/or BT stack enters this state,
> even halting rumble commands will never allow the reporting rate to
> recover to nominal. The controller will eventually disconnect.
> 
> This patch set strives to avoid the problematic scenario. It detects if
> input reports arrive at clearly incorrect deltas. During these times,
> the driver will hold off on transmitting any rumble commands to the
> controller. This approach has allowed the reporting rate to reliably
> recover in my testing. I've not been able to generate a controller
> disconnection during hours of testing games with frequent rumble.
> 
> The behavior of this mechanism is tunable using #defines. We may need to
> tweak/tune as the mitigation is used on different hardware setups.
> 
> My suspicion is that the core issue is somewhere in the bluez stack. My
> next step is to investigate that lead in more detail. This patchset at
> least allows for use of the controllers via bluetooth with rumble
> enabled without frequently disconnecting.
> 
> Daniel J. Ogorchock (2):
>   HID: nintendo: prevent rumble queue overruns
>   HID: nintendo: fix rumble rate limiter
> 
>  drivers/hid/hid-nintendo.c | 95 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 86 insertions(+), 9 deletions(-)

Now queued in hid.git#for-6.4/nintendo. Thanks,

-- 
Jiri Kosina
SUSE Labs