Merge pull request #1573 from UltimateHackingKeyboard/adjust_scheduling_simplified2

More jitter adjustments. Rest is peripheral vs central clock drifts :-/.

Author: kareltucek

device/src/bt_conn.c

@@ -50,12 +50,6 @@ struct bt_conn *auth_conn;
 
 uint32_t Bt_LastConnectedTime = 0;
 
-// Last negotiated BLE HID connection interval in milliseconds. Updated by
-// infoLatencyParamsUpdated when the active peer is a BLE HID host. Used by the
-// report-throttling logic to size the next-window estimate; otherwise on hosts
-// that negotiate below our default we would systematically miss send slots.
-uint32_t BleHidReportIntervalMs = 11;
-
 /*
  * Reason codes are named from the perspective of the party that receives them.
  *
@@ -273,6 +267,13 @@ static struct bt_conn_le_data_len_param *data_len;
 static void enableDataLengthExtension(struct bt_conn *conn) {
     data_len = BT_LE_DATA_LEN_PARAM_MAX;
 
+    /**
+     * This configures actual transmission length.
+     *
+     * We don't want it too high in order to prevent scheduling conflicts between multiple links.
+     * */
+    data_len->tx_max_time = 2500;
+
     int err = bt_conn_le_data_len_update(conn, data_len);
     if (err) {
         LOG_INF("LE data length update failed: %d", err);
@@ -314,13 +315,10 @@ static void setLatency(struct bt_conn* conn, const struct bt_le_conn_param* para
 static void configureLatency(struct bt_conn *conn, latency_mode_t latencyMode) {
     switch (latencyMode) {
         case LatencyMode_NUS: {
-                // https://developer.apple.com/library/archive/qa/qa1931/_index.html
-                // https://punchthrough.com/manage-ble-connection/
-                // https://devzone.nordicsemi.com/f/nordic-q-a/28058/what-is-connection-parameters
                 const struct bt_le_conn_param conn_params = BT_LE_CONN_PARAM_INIT(
-                    6, 6, // keep it low, lowest allowed is 6 (7.5ms), lowest supported widely is 9 (11.25ms)
-                    0, // keeping it higher allows power saving on peripheral when there's nothing to send (keep it under 30 though)
-                    100 // connection timeout (*10ms)
+                    6, 6,
+                    0,
+                    100
                 );
                 setLatency(conn, &conn_params);
              }
@@ -764,7 +762,7 @@ static void securityChanged(struct bt_conn *conn, bt_security_t level, enum bt_s
         struct bt_conn_info info;
         int err = bt_conn_get_info(conn, &info);
         if (err == 0 && info.state == BT_CONN_STATE_CONNECTED) {
-            bt_conn_auth_cancel(conn);
+            // bt_conn_auth_cancel(conn);
             // bt_conn_disconnect(conn, BT_REASON_PERMANENT);
         } else {
             // Sometimes securityChanged gets called twice, resulting in a race and a crash, so check for it \efp.
@@ -791,22 +789,29 @@ __attribute__((unused)) static void infoLatencyParamsUpdated(struct bt_conn* con
 {
     LOG_DBG("%s conn params: interval=%u ms, latency=%u, timeout=%u ms", GetPeerStringByConn(conn), interval * 5 / 4, latency, timeout * 10);
 
-    connection_type_t connectionType = Connections_Type(Peers[GetPeerIdByConn(conn)].connectionId);
+    int8_t peerId = GetPeerIdByConn(conn);
+    connection_type_t connectionType = Connections_Type(Peers[peerId].connectionId);
     bool isUhkPeer = isUhkDeviceConnection(connectionType);
 
-    if (connectionType == ConnectionType_BtHid) {
+    if (connectionType == ConnectionType_BtHid || connectionType == ConnectionType_NusDongle) {
         uint32_t intervalMs = (interval * 5 + 3) / 4;
         if (intervalMs < 1) {
             intervalMs = 1;
         }
-        BleHidReportIntervalMs = intervalMs;
+        Peers[peerId].bleReportIntervalMs = intervalMs;
     }
 
     if (interval > 10) {
         configureLatency(conn, isUhkPeer ? LatencyMode_NUS : LatencyMode_BleHid);
     }
 }
 
+uint32_t BtConn_GetReportIntervalMs(connection_id_t connectionId) {
+    uint8_t peerId = Connections[connectionId].peerId;
+    uint32_t intervalMs = Peers[peerId].bleReportIntervalMs;
+    return intervalMs != 0 ? intervalMs : 11;
+}
+
 BT_CONN_CB_DEFINE(conn_callbacks) = {
     .connected = connected,
     .disconnected = disconnected,

device/src/bt_conn.h

@@ -45,6 +45,7 @@
         bt_addr_le_t addr;
         struct bt_conn* conn;
         uint32_t lastSwitchover;
+        uint32_t bleReportIntervalMs;
     } peer_t;
 
 typedef enum {
@@ -57,11 +58,11 @@ typedef enum {
     extern peer_t Peers[];
     extern bool Bt_NewPairedDevice;
     extern uint32_t Bt_LastConnectedTime;
-    extern uint32_t BleHidReportIntervalMs;
 
 // Functions:
 
     int8_t GetPeerIdByConn(const struct bt_conn *conn);
+    uint32_t BtConn_GetReportIntervalMs(connection_id_t connectionId);
     char *GetPeerStringByAddr(const bt_addr_le_t *addr);
     char *GetPeerStringByConn(const struct bt_conn *conn);
     char *GetPeerStringByConnId(uint8_t connectionId);

device/src/shell/shell_commands.c

@@ -443,7 +443,11 @@ static int cmd_uhk_jitterTest(const struct shell *shell, size_t argc, char *argv
     if (argc == 1) {
         shell_fprintf(shell, SHELL_NORMAL, "%i\n", JitterTest_Active ? 1 : 0);
     } else {
-        JitterTest_SetActive(argv[1][0] == '1');
+        bool active = argv[1][0] == '1';
+        JitterTest_SetActive(active);
+        if (active) {
+            LogU("For jitter test, please make sure that your right half has keystrokeDelay set to 0.\n");
+        }
     }
     return 0;
 }

doc-dev/other/testing_scenarios.md

@@ -12,6 +12,7 @@ Following is a list testing suggestions. Not all have to be tested each time.
 - Pair two ble hids using `bluetooth pair` while all devices are active.
 - Unregistered slot connections: try to add a ble hid without registering it in Agent. Change registered connection count and save the userconfig. Check that the ble hid remaind in the same slot (unless another host connection took that slot - in that case, it should be in the first availalbe slot). 
 - If moving currently connected device's host connection to another position, check that saving the UserConfig doesn't disconnect it - it should be remapped to the new slot by itself.
+- Connect dongle. Switch to it. Then disconnect it. Connect it again. Does it connect? (Repeat a couple of times.)
 
 Todos:
 - deduplicate and reorder these scenarios.

right/src/config_manager.c

@@ -280,7 +280,7 @@ const config_t DefaultCfg = (config_t){
         .Bt_AllowUnsecuredConnections = false,
         .Bt_MaxPeripheralConnections = 3,
         .Bt_AlwaysAdvertiseHid = true,
-        .Bt_DirectedAdvertisingAllowed = true,
+        .Bt_DirectedAdvertisingAllowed = false,
         .DevMode = false,
         .EmergencyKey = NULL,
         .UiStyle = UiStyle_Classic,

right/src/hid/transport.cpp

@@ -1,6 +1,7 @@
 extern "C" {
 #include "transport.h"
 #ifdef __ZEPHYR__
+    #include "bt_conn.h"
     #include "connections.h"
     #include "link_protocol.h"
     #include "messenger.h"
@@ -49,24 +50,19 @@ static uint32_t dispatchTimeMs = 0;
 // interval" (worst case: we just missed a window). The send-completion
 // callback then reduces the estimate to "now + interval".
 static constexpr uint32_t USB_REPORT_INTERVAL_MS = 1;
-static constexpr uint32_t DONGLE_REPORT_INTERVAL_MS = 7;
-
-#if DEVICE_IS_UHK80_RIGHT
-extern "C" uint32_t BleHidReportIntervalMs;
-static inline uint32_t bleHidReportIntervalMs() { return BleHidReportIntervalMs; }
-#else
-static inline uint32_t bleHidReportIntervalMs() { return 11; }
-#endif
 
 static uint32_t reportIntervalForSink(report_sink_t sink)
 {
     switch (sink) {
     case ReportSink_Usb:
         return USB_REPORT_INTERVAL_MS;
     case ReportSink_BleHid:
-        return bleHidReportIntervalMs();
     case ReportSink_Dongle:
-        return DONGLE_REPORT_INTERVAL_MS;
+#if DEVICE_IS_UHK80_RIGHT
+        return BtConn_GetReportIntervalMs(ActiveHostConnectionId);
+#else
+        return 11;
+#endif
     default:
         return 0;
     }
@@ -79,7 +75,7 @@ static void noteReportDispatched(report_sink_t sink)
             dispatchTimeMs = Timer_GetCurrentTime();
         }
     }
-    UsbReportWindowEstimate = Timer_GetCurrentTime() + 2 * reportIntervalForSink(sink);
+    UsbReportWindowEstimate = UsbReportWindowEstimateLast + 2 * reportIntervalForSink(sink);
 }
 
 static void noteReportSent(report_sink_t transport)
@@ -91,8 +87,17 @@ static void noteReportSent(report_sink_t transport)
             dispatchTimeMs = 0;
         }
     }
-    UsbReportWindowEstimate = Timer_GetCurrentTime() + reportIntervalForSink(transport);
-    EventVector_WakeMain();
+    uint32_t reportInterval = reportIntervalForSink(transport);
+    uint32_t currentTime = Timer_GetCurrentTime();
+    int16_t jitterGuess = (currentTime - UsbReportWindowEstimateLast - reportInterval + 1) / 2;
+    jitterGuess = MAX(0, jitterGuess);
+    UsbReportWindowEstimateLast = currentTime - jitterGuess;
+    UsbReportWindowEstimate = currentTime - jitterGuess + reportInterval;
+    uint32_t nextIn = UsbReportWindowEstimate - USB_REPORT_WINDOW_LOOKAHEAD_MS;
+
+    if (DEVICE_IS_UHK80_RIGHT) {
+        EventScheduler_Schedule(nextIn, EventSchedulerEvent_Postponer, "Report sent. Recalculate report throttles");
+    }
 }
 
 extern "C" void HidTransport_NoteNusReportSent(void)

right/src/usb_report_updater.c

@@ -73,16 +73,9 @@ static key_action_cached_t actionCache[SLOT_COUNT][MAX_KEY_COUNT_PER_MODULE];
 
 uint32_t UsbReportUpdater_LastActivityTime;
 
+uint32_t UsbReportWindowEstimateLast = 0;
 uint32_t UsbReportWindowEstimate = 0;
 
-// This is how much time we leave for report construction. Most of the time is
-// (probably) consumed inside the transport layers.
-//
-// If too low, we will be missing transports. If too high, we will be introducing
-// latency.
-#define USB_REPORT_WINDOW_LOOKAHEAD_MS 6
-
-#define USB_RESEND_DELAY_MS MAX(10, Cfg.KeystrokeDelay)
 
 volatile uint8_t UsbReportUpdateSemaphore = 0;
 

right/src/usb_report_updater.h

@@ -16,6 +16,14 @@
 
     #define USB_SEMAPHORE_TIMEOUT 100 // ms
 
+    // This is how much time we leave for report construction. Most of the time is
+    // (probably) consumed inside the transport layers.
+    //
+    // If too low, we will be missing transports. If too high, we will be introducing
+    // latency.
+    #define USB_REPORT_WINDOW_LOOKAHEAD_MS 3
+    #define USB_RESEND_DELAY_MS MAX(10, Cfg.KeystrokeDelay)
+
 // Typedefs:
 
     typedef struct {
@@ -53,6 +61,7 @@
     extern uint32_t LastUsbGetKeyboardStateRequestTimestamp;
     extern uint32_t UsbReportUpdater_LastActivityTime;
     extern uint32_t UsbReportWindowEstimate;
+    extern uint32_t UsbReportWindowEstimateLast;
 
 // Functions: