Reduce maximum packet size.

Author: kareltucek

device/prj.conf.overlays/nrf_shared.conf

@@ -37,19 +37,31 @@ CONFIG_BT_FILTER_ACCEPT_LIST=y
 CONFIG_BT_CTLR_SDC_MAX_CONN_EVENT_LEN_DEFAULT=2500
 CONFIG_BT_CTLR_SDC_CENTRAL_ACL_EVENT_SPACING_DEFAULT=2500
 
-# increase these to make multiple connections more reliable
-# this is a generic ai advice. 
-CONFIG_BT_ATT_TX_COUNT=10
-CONFIG_BT_CONN_TX_MAX=6
-CONFIG_BT_L2CAP_TX_BUF_COUNT=12
-
-# negotiate larger MTU for NUS
+# BLE buffer counts, sized for ~2 active links (L=2) with double buffering
+# (D=2), no HCI fragmentation (LL unit == L2CAP MTU, so frags/PDU F=1). Each
+# line notes the constraint it must satisfy. BT_BUF_ACL_TX_COUNT (the HCI TX
+# credit window) is left at its default 3: it is capped by the controller's
+# BT_CTLR_SDC_TX_PACKET_COUNT and 3 credits already cover 2 links.
+# ATT TX PDUs in flight: >= L*D (freed on HCI handoff, not air-ack)
+CONFIG_BT_ATT_TX_COUNT=5
+# L2CAP signalling/SMP only (no CoC): a handful
+CONFIG_BT_L2CAP_TX_BUF_COUNT=4
+# HCI event RX: > BT_BUF_ACL_TX_COUNT (build assert)
+CONFIG_BT_BUF_EVT_RX_COUNT=6
+# ACL RX reassembly: 1+EXTRA >= L+1 (one/link + driver spare)
+CONFIG_BT_BUF_ACL_RX_COUNT_EXTRA=2
+
+# NUS packet sizing. We cap the link packet at MAX_LINK_PACKET_LENGTH=128
+# (link_protocol.h) rather than the DLE maximum, to save RAM and cut BLE latency.
+#   ACL buffer = MAX_LINK_PACKET_LENGTH + L2CAP(4) + ATT(3) = 135
+#   L2CAP MTU  = MAX_LINK_PACKET_LENGTH + ATT(3)            = 131
+# bt_conn.c caps the negotiated DLE tx length to CONFIG_BT_BUF_ACL_TX_SIZE.
 CONFIG_BT_USER_DATA_LEN_UPDATE=y
-CONFIG_BT_BUF_ACL_RX_SIZE=251
-CONFIG_BT_BUF_ACL_TX_SIZE=251
-CONFIG_BT_L2CAP_TX_MTU=247
+CONFIG_BT_BUF_ACL_RX_SIZE=135
+CONFIG_BT_BUF_ACL_TX_SIZE=135
+CONFIG_BT_L2CAP_TX_MTU=131
 
-# allow BLE buffering
+# inbound long-write (config transfer) queue depth
 CONFIG_BT_ATT_PREPARE_COUNT=4
 
 # store BLE link keys in flash

device/src/bt_conn.c

@@ -291,10 +291,13 @@ static void enableDataLengthExtension(struct bt_conn *conn) {
     data_len = BT_LE_DATA_LEN_PARAM_MAX;
 
     /**
-     * This configures actual transmission length.
+     * Configures actual transmission length. Cap the payload to our reduced ACL
+     * buffer (CONFIG_BT_BUF_ACL_TX_SIZE) rather than the DLE max of 251: we never
+     * send larger packets and shorter PDUs mean lower latency.
      *
-     * We don't want it too high in order to prevent scheduling conflicts between multiple links.
+     * tx_max_time kept low to avoid scheduling conflicts between multiple links.
      * */
+    data_len->tx_max_len = CONFIG_BT_BUF_ACL_TX_SIZE;
     data_len->tx_max_time = 2500;
 
     int err = bt_conn_le_data_len_update(conn, data_len);

device/src/link_protocol.h

@@ -8,12 +8,16 @@
 
 // Macros:
 
-// 251 = maximum BLE packet length with data length extension
+// Maximum deserialized message length (header + payload) that fits in a single
+// BLE notification / UART bridge frame. We deliberately keep this well below the
+// DLE maximum (244 with a 251-byte ACL buffer) to save RAM (messenger-queue
+// regions, nus/uart buffers) and to shorten BLE packets for lower latency.
 // 4 bytes reserved for L2CAP header
 // 3 bytes reserved for ATT header
 // https://devzone.nordicsemi.com/f/nordic-q-a/111900/maximum-nus-packet-payload-with-ble-data-length-extensio
-// Should equal `CONFIG_BT_BUF_ACL_RX_SIZE - L2CAP_HEADER_SIZE - ATT_HEADER_SIZE`, otherwise something is wrong
-#define MAX_LINK_PACKET_LENGTH 244
+// Should equal `CONFIG_BT_BUF_ACL_RX_SIZE - L2CAP_HEADER_SIZE - ATT_HEADER_SIZE`
+// (i.e. 135 - 4 - 3), otherwise something is wrong.
+#define MAX_LINK_PACKET_LENGTH 128
 
 // Typedefs:
 

device/src/state_sync.c

@@ -1,5 +1,6 @@
 #include "state_sync.h"
 #include "bt_conn.h"
+#include "link_protocol.h"
 #include "connections.h"
 #include "device.h"
 #include "device_state.h"
@@ -160,8 +161,27 @@ static state_sync_prop_t stateSyncProps[StateSyncPropertyId_Count] = {
     CUSTOM(KeyStatesDummy,          SyncDirection_LeftToRight,        DirtyState_Clean),
     CUSTOM(DongleProtocolVersion,   SyncDirection_DongleToRight,      DirtyState_Clean),
     SIMPLE(BatteryStationaryMode,   SyncDirection_RightToLeft,        DirtyState_Clean,    &Cfg.BatteryStationaryMode),
+    // Emitted alongside ModuleState*; never independently dirtied.
+    CUSTOM(ModuleGitRepo,           SyncDirection_LeftToRight,        DirtyState_Clean),
+    CUSTOM(ModuleGitTag,            SyncDirection_LeftToRight,        DirtyState_Clean),
 };
 
+// Bytes a state-sync message occupies on top of its payload struct: the 3-byte
+// link header (src, dst, watermark - MessageOffset_Payload) plus the two message
+// id bytes (MessageId_StateSync and the property id).
+#define STATE_SYNC_MESSAGE_HEADER_LENGTH (MessageOffset_Payload + 2)
+// Largest payload struct that still fits in a single link packet.
+#define STATE_SYNC_MAX_PAYLOAD_LENGTH (MAX_LINK_PACKET_LENGTH - STATE_SYNC_MESSAGE_HEADER_LENGTH)
+
+_Static_assert(sizeof(sync_command_layer_t) <= STATE_SYNC_MAX_PAYLOAD_LENGTH,
+    "sync_command_layer_t does not fit in a link packet; reduce KEY_COUNT_PER_UPDATE");
+_Static_assert(sizeof(sync_command_module_state_t) <= STATE_SYNC_MAX_PAYLOAD_LENGTH,
+    "sync_command_module_state_t does not fit in a link packet");
+_Static_assert(sizeof(sync_command_module_git_repo_t) <= STATE_SYNC_MAX_PAYLOAD_LENGTH,
+    "sync_command_module_git_repo_t does not fit in a link packet");
+_Static_assert(sizeof(sync_command_module_git_tag_t) <= STATE_SYNC_MAX_PAYLOAD_LENGTH,
+    "sync_command_module_git_tag_t does not fit in a link packet");
+
 static void invalidateProperty(state_sync_prop_id_t propId) {
     STATE_SYNC_LOG("<<< Invalidating property %s\n", stateSyncProps[propId].name);
     if (StateSyncPropertyId_LayerActionFirst <= propId &&
@@ -339,8 +359,6 @@ static void receiveModuleStateData(sync_command_module_state_t *buffer) {
     moduleState->firmwareVersion = buffer->firmwareVersion;
     moduleState->keyCount = buffer->keyCount;
     moduleState->pointerCount = buffer->pointerCount;
-    Utils_SafeStrCopy(moduleState->gitRepo, buffer->gitRepo, MAX_STRING_PROPERTY_LENGTH);
-    Utils_SafeStrCopy(moduleState->gitTag, buffer->gitTag, MAX_STRING_PROPERTY_LENGTH);
     memcpy(moduleState->firmwareChecksum, buffer->firmwareChecksum, MD5_CHECKSUM_LENGTH);
 
     if (DEVICE_IS_UHK80_RIGHT && leftModuleChanged) {
@@ -446,6 +464,20 @@ static void receiveProperty(device_id_t src, state_sync_prop_id_t propId, const
             receiveModuleStateData((sync_command_module_state_t *)data);
         }
         break;
+    case StateSyncPropertyId_ModuleGitRepo:
+        if (!isLocalUpdate) {
+            sync_command_module_git_repo_t *buffer = (sync_command_module_git_repo_t *)data;
+            uint8_t driverId = UhkModuleSlaveDriver_SlotIdToDriverId(buffer->slotId);
+            Utils_SafeStrCopy(UhkModuleStates[driverId].gitRepo, buffer->gitRepo, MAX_STRING_PROPERTY_LENGTH);
+        }
+        break;
+    case StateSyncPropertyId_ModuleGitTag:
+        if (!isLocalUpdate) {
+            sync_command_module_git_tag_t *buffer = (sync_command_module_git_tag_t *)data;
+            uint8_t driverId = UhkModuleSlaveDriver_SlotIdToDriverId(buffer->slotId);
+            Utils_SafeStrCopy(UhkModuleStates[driverId].gitTag, buffer->gitTag, MAX_STRING_PROPERTY_LENGTH);
+        }
+        break;
     case StateSyncPropertyId_FunctionalColors:
         if (!isLocalUpdate) {
             EventVector_Set(EventVector_LedMapUpdateNeeded);
@@ -565,27 +597,37 @@ static void prepareLayerActions(layer_id_t layerId, uint8_t slotId, uint8_t buff
 
 static void prepareAndSubmitLayer(device_id_t dst, state_sync_prop_id_t propId, layer_id_t layerId) {
     sync_command_layer_t buffer;
+    buffer.layerId = layerId;
 
-    const uint8_t firstPacketLeftHalfActionCount = KEY_COUNT_PER_UPDATE;
-    const uint8_t secondPacketLeftHalfActionCount = MAX_KEY_COUNT_PER_MODULE - KEY_COUNT_PER_UPDATE;
-    const uint8_t secondPacketLeftModuleActionCount = MAX_BACKLIT_KEY_COUNT_PER_LEFT_MODULE;
+    // Chunk the half-keyboard actions into KEY_COUNT_PER_UPDATE-sized packets. The
+    // module actions ride in the last chunk if they fit, else in a trailing packet.
+    bool moduleActionsSent = false;
 
-    buffer.layerId = layerId;
-    buffer.startOffset = 0;
-    buffer.actionCount = firstPacketLeftHalfActionCount;
-    buffer.moduleActionCount = 0;
+    for (uint8_t halfOffset = 0; halfOffset < MAX_KEY_COUNT_PER_MODULE; halfOffset += KEY_COUNT_PER_UPDATE) {
+        uint8_t halfCount = MIN(KEY_COUNT_PER_UPDATE, MAX_KEY_COUNT_PER_MODULE - halfOffset);
+        bool isLastHalfChunk = halfOffset + halfCount >= MAX_KEY_COUNT_PER_MODULE;
 
-    prepareLayerActions(layerId, SlotId_LeftKeyboardHalf, 0, 0, firstPacketLeftHalfActionCount, &buffer);
-    submitPreparedData(dst, propId, (const uint8_t *)&buffer, sizeof(buffer));
+        buffer.startOffset = halfOffset;
+        buffer.actionCount = halfCount;
+        buffer.moduleActionCount = 0;
+        prepareLayerActions(layerId, SlotId_LeftKeyboardHalf, 0, halfOffset, halfCount, &buffer);
 
-    buffer.layerId = layerId;
-    buffer.startOffset = firstPacketLeftHalfActionCount;
-    buffer.actionCount = secondPacketLeftHalfActionCount;
-    buffer.moduleActionCount = secondPacketLeftModuleActionCount;
+        if (isLastHalfChunk && halfCount + MAX_BACKLIT_KEY_COUNT_PER_LEFT_MODULE <= KEY_COUNT_PER_UPDATE) {
+            buffer.moduleActionCount = MAX_BACKLIT_KEY_COUNT_PER_LEFT_MODULE;
+            prepareLayerActions(layerId, SlotId_LeftModule, halfCount, 0, MAX_BACKLIT_KEY_COUNT_PER_LEFT_MODULE, &buffer);
+            moduleActionsSent = true;
+        }
+
+        submitPreparedData(dst, propId, (const uint8_t *)&buffer, sizeof(buffer));
+    }
 
-    prepareLayerActions(layerId, SlotId_LeftKeyboardHalf, 0, firstPacketLeftHalfActionCount, secondPacketLeftHalfActionCount, &buffer);
-    prepareLayerActions(layerId, SlotId_LeftModule, secondPacketLeftHalfActionCount, 0, secondPacketLeftModuleActionCount, &buffer);
-    submitPreparedData(dst, propId, (const uint8_t *)&buffer, sizeof(buffer));
+    if (!moduleActionsSent) {
+        buffer.startOffset = 0;
+        buffer.actionCount = 0;
+        buffer.moduleActionCount = MAX_BACKLIT_KEY_COUNT_PER_LEFT_MODULE;
+        prepareLayerActions(layerId, SlotId_LeftModule, 0, 0, MAX_BACKLIT_KEY_COUNT_PER_LEFT_MODULE, &buffer);
+        submitPreparedData(dst, propId, (const uint8_t *)&buffer, sizeof(buffer));
+    }
 }
 
 static void prepareBacklight(sync_command_backlight_t *buffer) {
@@ -604,8 +646,6 @@ static void prepareLeftHalfStateData(sync_command_module_state_t *buffer) {
     buffer->firmwareVersion = firmwareVersion;
     buffer->keyCount = MODULE_KEY_COUNT;
     buffer->pointerCount = MODULE_POINTER_COUNT;
-    Utils_SafeStrCopy(buffer->gitRepo, gitRepo, MAX_STRING_PROPERTY_LENGTH);
-    Utils_SafeStrCopy(buffer->gitTag, gitTag, MAX_STRING_PROPERTY_LENGTH);
     memcpy(buffer->firmwareChecksum, DeviceMD5Checksums[DEVICE_ID], MD5_CHECKSUM_LENGTH);
 #endif
 }
@@ -619,11 +659,35 @@ static void prepareLeftModuleStateData(sync_command_module_state_t *buffer) {
     buffer->firmwareVersion = moduleState->firmwareVersion;
     buffer->keyCount = moduleState->keyCount;
     buffer->pointerCount = moduleState->pointerCount;
-    memcpy(buffer->gitRepo, moduleState->gitRepo, MAX_STRING_PROPERTY_LENGTH);
-    memcpy(buffer->gitTag, moduleState->gitTag, MAX_STRING_PROPERTY_LENGTH);
     memcpy(buffer->firmwareChecksum, moduleState->firmwareChecksum, MD5_CHECKSUM_LENGTH);
 }
 
+static void prepareModuleGitRepo(device_id_t dst) {
+    sync_command_module_git_repo_t buffer = {};
+
+    buffer.slotId = SlotId_LeftKeyboardHalf;
+    Utils_SafeStrCopy(buffer.gitRepo, gitRepo, MAX_STRING_PROPERTY_LENGTH);
+    submitPreparedData(dst, StateSyncPropertyId_ModuleGitRepo, (const uint8_t *)&buffer, sizeof(buffer));
+
+    uhk_module_state_t *moduleState = UhkModuleStates + UhkModuleSlaveDriver_SlotIdToDriverId(SlotId_LeftModule);
+    buffer.slotId = SlotId_LeftModule;
+    Utils_SafeStrCopy(buffer.gitRepo, moduleState->gitRepo, MAX_STRING_PROPERTY_LENGTH);
+    submitPreparedData(dst, StateSyncPropertyId_ModuleGitRepo, (const uint8_t *)&buffer, sizeof(buffer));
+}
+
+static void prepareModuleGitTag(device_id_t dst) {
+    sync_command_module_git_tag_t buffer = {};
+
+    buffer.slotId = SlotId_LeftKeyboardHalf;
+    Utils_SafeStrCopy(buffer.gitTag, gitTag, MAX_STRING_PROPERTY_LENGTH);
+    submitPreparedData(dst, StateSyncPropertyId_ModuleGitTag, (const uint8_t *)&buffer, sizeof(buffer));
+
+    uhk_module_state_t *moduleState = UhkModuleStates + UhkModuleSlaveDriver_SlotIdToDriverId(SlotId_LeftModule);
+    buffer.slotId = SlotId_LeftModule;
+    Utils_SafeStrCopy(buffer.gitTag, moduleState->gitTag, MAX_STRING_PROPERTY_LENGTH);
+    submitPreparedData(dst, StateSyncPropertyId_ModuleGitTag, (const uint8_t *)&buffer, sizeof(buffer));
+}
+
 static void prepareData(device_id_t dst, const uint8_t *propDataPtr, state_sync_prop_id_t propId) {
     state_sync_prop_t *prop = &stateSyncProps[propId];
     const uint8_t *data = propDataPtr;
@@ -636,13 +700,13 @@ static void prepareData(device_id_t dst, const uint8_t *propDataPtr, state_sync_
 
     switch (propId) {
     case StateSyncPropertyId_LayerActionFirst ... StateSyncPropertyId_LayerActionLast: {
-        layer_id_t layerId = propId - StateSyncPropertyId_LayerActionFirst + LayerId_First;
+        uint8_t layerId = propId - StateSyncPropertyId_LayerActionFirst + LayerId_First;
 
         if (prop->dirtyState == DirtyState_NeedsClearing) {
             submitPreparedData(dst, StateSyncPropertyId_LayerActionsClear, &layerId, sizeof(layerId));
             return;
         } else {
-            // 2 packets!
+            // split across multiple packets
             prepareAndSubmitLayer(dst, propId, layerId);
             return;
         }
@@ -659,6 +723,14 @@ static void prepareData(device_id_t dst, const uint8_t *propDataPtr, state_sync_
         submitPreparedData(dst, propId, (const uint8_t *)&buffer, sizeof(buffer));
         return;
     } break;
+    case StateSyncPropertyId_ModuleGitRepo: {
+        prepareModuleGitRepo(dst);
+        return;
+    } break;
+    case StateSyncPropertyId_ModuleGitTag: {
+        prepareModuleGitTag(dst);
+        return;
+    } break;
     case StateSyncPropertyId_Backlight: {
         sync_command_backlight_t buffer;
         prepareBacklight(&buffer);
@@ -772,6 +844,8 @@ static bool handlePropertyUpdateLeftToRight() {
 
     UPDATE_AND_RETURN_IF_DIRTY(StateSyncPropertyId_ModuleStateLeftHalf, UpdateResult_UpdatedHighPrio);
     UPDATE_AND_RETURN_IF_DIRTY(StateSyncPropertyId_ModuleStateLeftModule, UpdateResult_UpdatedHighPrio);
+    UPDATE_AND_RETURN_IF_DIRTY(StateSyncPropertyId_ModuleGitRepo, UpdateResult_UpdatedLowPrio);
+    UPDATE_AND_RETURN_IF_DIRTY(StateSyncPropertyId_ModuleGitTag, UpdateResult_UpdatedLowPrio);
     UPDATE_AND_RETURN_IF_DIRTY(StateSyncPropertyId_KeyStatesDummy, UpdateResult_UpdatedHighPrio);
     UPDATE_AND_RETURN_IF_DIRTY(StateSyncPropertyId_LeftModuleDisconnected, UpdateResult_UpdatedHighPrio);
     UPDATE_AND_RETURN_IF_DIRTY(StateSyncPropertyId_MergeSensor, UpdateResult_UpdatedHighPrio);
@@ -915,6 +989,8 @@ void StateSync_ResetRightLeftLink(bool bidirectional) {
         invalidateProperty(StateSyncPropertyId_Battery);
         invalidateProperty(StateSyncPropertyId_ModuleStateLeftHalf);
         invalidateProperty(StateSyncPropertyId_ModuleStateLeftModule);
+        invalidateProperty(StateSyncPropertyId_ModuleGitRepo);
+        invalidateProperty(StateSyncPropertyId_ModuleGitTag);
         invalidateProperty(StateSyncPropertyId_KeyStatesDummy);
         invalidateProperty(StateSyncPropertyId_MergeSensor);
     }

device/src/state_sync.h

@@ -15,7 +15,9 @@
 
 // Macros:
 
-    #define KEY_COUNT_PER_UPDATE ((MAX_KEY_COUNT_PER_MODULE+MAX_BACKLIT_KEY_COUNT_PER_LEFT_MODULE)/2+1)
+    // Max key actions per layer-sync packet; sized so sync_command_layer_t fits a
+    // link packet (see static assert in state_sync.c). Split into 3 packets.
+    #define KEY_COUNT_PER_UPDATE 23
 
 // Typedefs:
 
@@ -32,12 +34,12 @@
     } ATTR_PACKED sync_command_action_t;
 
     typedef struct {
-        layer_id_t layerId;
+        uint8_t layerId;
         uint8_t startOffset;
         uint8_t actionCount;
         uint8_t moduleActionCount;
         sync_command_action_t actions[KEY_COUNT_PER_UPDATE];
-    } sync_command_layer_t;
+    } ATTR_PACKED sync_command_layer_t;
 
     typedef struct {
         uint16_t vertical;
@@ -59,11 +61,20 @@
         version_t firmwareVersion;
         uint8_t keyCount;
         uint8_t pointerCount;
-        char gitRepo[MAX_STRING_PROPERTY_LENGTH];
-        char gitTag[MAX_STRING_PROPERTY_LENGTH];
         char firmwareChecksum[MD5_CHECKSUM_LENGTH];
     } ATTR_PACKED sync_command_module_state_t;
 
+    // gitRepo/gitTag are synced separately: together they exceed one link packet.
+    typedef struct {
+        uint8_t slotId;
+        char gitRepo[MAX_STRING_PROPERTY_LENGTH];
+    } ATTR_PACKED sync_command_module_git_repo_t;
+
+    typedef struct {
+        uint8_t slotId;
+        char gitTag[MAX_STRING_PROPERTY_LENGTH];
+    } ATTR_PACKED sync_command_module_git_tag_t;
+
     typedef struct {
         version_t dataModelVersion;
     } sync_command_config_t;
@@ -106,6 +117,8 @@
         StateSyncPropertyId_KeyStatesDummy = 31,
         StateSyncPropertyId_DongleProtocolVersion = 32,
         StateSyncPropertyId_BatteryStationaryMode = 33,
+        StateSyncPropertyId_ModuleGitRepo = 34,
+        StateSyncPropertyId_ModuleGitTag = 35,
         StateSyncPropertyId_Count,
     } state_sync_prop_id_t;
 

right/src/slave_drivers/uhk_module_driver.c

@@ -458,6 +458,8 @@ slave_result_t UhkModuleSlaveDriver_Update(uint8_t uhkModuleDriverId)
 #ifdef __ZEPHYR__
             if (DEVICE_ID == DeviceId_Uhk80_Left && uhkModuleDriverId == UhkModuleDriverId_LeftModule) {
                 StateSync_UpdateProperty(StateSyncPropertyId_ModuleStateLeftModule, NULL);
+                StateSync_UpdateProperty(StateSyncPropertyId_ModuleGitRepo, NULL);
+                StateSync_UpdateProperty(StateSyncPropertyId_ModuleGitTag, NULL);
             }
 #endif
             LogU("Module %d initialized: protocol version %d.%d.%d, firmware version %d.%d.%d, git tag %s, git repo %s, firmware checksum %s\n",

shared/uart_defs.h

@@ -13,8 +13,11 @@
 
     // modules have very limited RAM, so keep low; Also, keep it
     #define UART_MAX_MODULE_PAYLOAD_LENGTH SLAVE_PROTOCOL_MAX_PAYLOAD_LENGTH
-    // 244 = max BLE payload length
-    #define UART_MAX_BRIDGE_PAYLOAD_LENGTH 244
+    // Max deserialized bridge message length. Must equal MAX_LINK_PACKET_LENGTH
+    // (link_protocol.h) - the bridge rx buffer is a messenger-queue region of
+    // that size, and messages travel interchangeably over UART and BLE. Kept
+    // small (rather than the BLE-max 244) to save RAM and cut BLE latency.
+    #define UART_MAX_BRIDGE_PAYLOAD_LENGTH 128
 
     #define UART_CRC_LEN 2