Skip to content

fix: Use MAC address for BLE disconnect decision after climb changes#653

Open
marcodejongh wants to merge 7 commits intomainfrom
lilygo_next
Open

fix: Use MAC address for BLE disconnect decision after climb changes#653
marcodejongh wants to merge 7 commits intomainfrom
lilygo_next

Conversation

@marcodejongh
Copy link
Owner

@marcodejongh marcodejongh commented Feb 4, 2026

Summary

  • Fixes ESP32 display not receiving metadata updates when climbs are loaded via Bluetooth from the official Kilter app
  • Uses device MAC address (auto-detected) instead of manual controller ID configuration for identifying self-initiated changes
  • Handles unknown/unsynced climbs gracefully with "Unknown Climb" display

Problem

When a climb was loaded via Bluetooth, the backend was skipping LedUpdate events to the controller that initiated them. This prevented the ESP32 display from showing climb metadata (name, grade, navigation context) for BLE-loaded climbs.

Solution

  • Add clientId field to LedUpdate GraphQL type
  • Backend always sends LedUpdate with clientId set to the controller's MAC address
  • ESP32 compares incoming clientId with its own MAC address:
    • Same MAC: Self-initiated change → keep BLE client connected
    • Different MAC: Web user changed climb → disconnect BLE client
  • MAC address is automatically detected from WiFi - no configuration needed

Test plan

  • Load a climb via official Kilter app (phone → ESP32 via BLE)
  • Verify ESP32 display shows climb name, grade, navigation
  • Verify phone stays connected via BLE
  • Change climb from web interface while phone is connected
  • Verify phone gets disconnected from BLE
  • Verify display shows new climb from web

🤖 Generated with Claude Code

marcodejongh and others added 2 commits February 4, 2026 12:32
@marcodejongh @claude
feat: Add queue state sync for reliable ESP32 navigation
fcdaf1f 
Implements local queue storage on ESP32 with optimistic UI updates
for responsive button navigation. Key changes:

Backend:
- Add ControllerQueueItem and ControllerQueueSync GraphQL types
- Add queueItemUuid field to LedUpdate for reconciliation
- Update navigateQueue mutation to accept queueItemUuid (direct nav)
- Send ControllerQueueSync on initial connection and queue changes

ESP32 Firmware:
- Add LocalQueueItem struct and queue storage (up to 150 items)
- Implement optimistic navigation with immediate display updates
- Handle ControllerQueueSync events to maintain local queue state
- Navigate using queueItemUuid instead of direction for reliability
- Reconcile optimistic updates with backend LedUpdate responses

This fixes erratic navigation when the same climb appears multiple
times in the queue, and enables fast-skipping through the queue.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@marcodejongh @claude
fix: Use MAC address for BLE disconnect decision after climb changes
02674b5 
When a climb was loaded via Bluetooth from the official Kilter app, the
ESP32 display wasn't receiving metadata updates because the backend was
skipping LedUpdate events sent to the same controller that initiated them.

This fix:
- Adds clientId field to LedUpdate GraphQL type
- Backend now always sends LedUpdate with clientId (MAC address of controller)
- ESP32 compares incoming clientId with its own MAC address
- If clientId matches own MAC: self-initiated change, keep BLE client connected
- If clientId differs: web user took over, disconnect BLE client

Also handles unknown climbs (drafts/unsynced) gracefully by displaying
"Unknown Climb" with navigation context so users can navigate back.

The MAC address is automatically detected - no manual configuration needed.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@vercel
Copy link

vercel bot commented Feb 4, 2026 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
boardsesh Building Building Preview, Comment Feb 5, 2026 1:00am

Request Review

@claude
Copy link

claude bot commented Feb 4, 2026

Claude Review

❌ Do not merge - Missing required files will cause build failure.

Critical Issues

  1. Missing files cause build failure - The PR imports from ./navigation-helpers and ./grade-colors in packages/backend/src/graphql/resolvers/controller/ but these files are not included in the PR:

    • packages/backend/src/graphql/resolvers/controller/mutations.ts:9 imports findClimbIndex from ./navigation-helpers
    • packages/backend/src/graphql/resolvers/controller/subscriptions.ts:8 imports getGradeColor from ./grade-colors and buildNavigationContext, findClimbIndex from ./navigation-helpers
    • Neither file exists in the repository

  2. Stack overflow risk in onQueueSync (embedded/projects/climb-queue-display/src/main.cpp:379) - Allocates LocalQueueItem items[ControllerQueueSyncData::MAX_ITEMS] (150 items × ~88 bytes = ~13KB) on stack, risking overflow on ESP32's limited stack

Minor Issues

  1. Unused setter method - graphql_ws_client.h:88 adds setControllerId() but this is never called; the code uses auto-detected MAC address instead

Documentation

  1. docs/websocket-implementation.md needs update - The PR adds new GraphQL types (ControllerQueueSync, QueueNavigationContext, navigateQueue mutation) and significantly changes controller event behavior, but the Controller Events section doesn't document these changes

marcodejongh and others added 2 commits February 4, 2026 15:04
@marcodejongh @claude
fix: Prevent BLE GATT server duplicate start crash (rc=15)
dc551c7 
- Use pServer->start() instead of ble_gatts_start() directly to set
  NimBLE's internal m_gattsStarted flag
- Move UUID/advertising config to begin() to avoid duplicate registration
- Use BLE.startAdvertising() in proxy mode for consistent state management

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@marcodejongh @claude
feat: Forward websocket LED updates to board in proxy mode
b9c60f4 
- Add AuroraProtocol::encodeLedCommands() to encode LED data into
  Aurora protocol packets for transmission to the board
- Add GraphQL LED update callback to notify when websocket updates arrive
- In proxy mode, encode and forward LED updates to the real board via BLE

This enables the web interface to control the physical board when
running through the ESP32 proxy.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Feb 4, 2026

Claude Review

❌ Do not merge - Missing files will cause build failures: navigation-helpers.ts and grade-colors.ts are imported but not included in the PR.

Critical Issues

  1. Missing helper files (packages/backend/src/graphql/resolvers/controller/):
    • mutations.ts:12 imports findClimbIndex from ./navigation-helpers - file doesn't exist
    • subscriptions.ts:10-11 imports getGradeColor from ./grade-colors and buildNavigationContext, findClimbIndex from ./navigation-helpers - files don't exist
    • This will cause TypeScript/build failures

Other Issues

  1. Stack allocation risk (embedded/projects/climb-queue-display/src/main.cpp:370-377): onQueueSync allocates LocalQueueItem items[150] on the stack (~13KB) before copying to heap. ESP32 has limited stack space - consider allocating on heap from the start like in board-controller/src/main.cpp.

  2. Verbose console logging in production (packages/backend/src/graphql/resolvers/controller/subscriptions.ts): Added many console.log statements (lines 150, 151, 152, 165, 166, 167, 228, 230, etc.) that could create noisy logs in production.

  3. Missing tests: No tests added for the new navigateQueue mutation, ControllerQueueSync event handling, or navigation context building logic.

Documentation

  • This PR adds navigation and queue sync features to the WebSocket/controller architecture. Consider updating docs/websocket-implementation.md to document the new ControllerQueueSync event type and navigateQueue mutation.

marcodejongh and others added 2 commits February 4, 2026 16:22
@marcodejongh @claude
fix: BLE proxy connection race condition and write chunking
6abec56 
- Fix race condition where handleScanComplete and handleBoardFound
  both tried to connect, causing state to flip back to CONNECTING
- Set state to CONNECTING before calling stopScan() to prevent
  handleScanComplete from initiating duplicate connection
- Split BLE writes into 20-byte chunks to match board's MTU and
  the TypeScript client implementation (MAX_BLUETOOTH_MESSAGE_SIZE)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@marcodejongh
Missing files
23bcfa5
@claude
Copy link

claude bot commented Feb 4, 2026

Claude Review

✅ Ready to merge - No significant issues found. The PR implements MAC address-based client identification for ESP32 BLE disconnect decisions, which is a cleaner approach than the previous controller ID system. The code is well-structured with good separation between navigation helpers, grade colors, and subscriptions.

Minor observations (non-blocking):

  • Memory allocation on heap in onQueueSync and handleQueueSync is appropriately handled with proper cleanup, avoiding stack overflow risks with the large queue data structures.

  • Removed climb-queue-display project in favor of consolidated board-controller with display support - this simplifies the codebase.

  • The GraphQL subscription now properly sends clientId (MAC address) with all LedUpdate events, allowing ESP32 to make intelligent BLE disconnect decisions.

@claude
Copy link

claude bot commented Feb 4, 2026 •
edited
Loading

Claude Review

✅ Ready to merge - Minor issues noted below, but nothing blocking.

Issues

  1. Blocking delay() calls in BLE proxy code - embedded/libs/ble-proxy/src/ble_proxy.cpp:213-214,265-267,309-311 and embedded/libs/ble-proxy/src/ble_scanner.cpp:133-136: Multiple blocking delay() calls (100ms, 200ms, 500ms) could freeze the main loop. Consider using non-blocking timers if responsiveness is critical.

  2. Unused variable BYTES_PER_LED - embedded/libs/aurora-protocol/src/aurora_protocol.cpp:21: Variable is declared but never used.

  3. Large stack allocation risk - embedded/projects/board-controller/src/main.cpp:543: LocalQueueItem* items = new LocalQueueItem[itemCount] with MAX_QUEUE_SIZE=150 items of ~88 bytes each (~13KB heap allocation) is appropriate, but the corresponding ControllerQueueSyncData struct in graphql_ws_client.h:23-32 is ~19KB per instance. The code handles this correctly by heap allocating at line 1681-1689 in graphql_ws_client.cpp, but worth noting for memory-constrained ESP32.

  4. Potential null dereference - embedded/libs/lilygo-display/src/lilygo_display.cpp:699-704: navigateToPrevious() and navigateToNext() check canNavigate*() before decrementing/incrementing, but then call getCurrentQueueItem() which could return nullptr if queue state changed. The code does null-check newCurrent before use, so this is just a note.

Test Coverage

The new tests (grade-colors.test.ts, navigate-queue.test.ts, navigation-helpers.test.ts) provide good coverage for the backend changes including edge cases.

Documentation

No documentation updates needed - this PR adds embedded/backend features without changing documented systems in docs/.

@marcodejongh @claude
fix: Address review feedback - BLE race condition and add tests
56cf821 
- Add atomic flag to prevent BLE proxy scan/connect race condition
  between handleBoardFound and handleScanComplete callbacks
- Document hardcoded BLE delays (100ms, 200ms, 500ms) with explanations
- Add grade-colors.test.ts (32 tests for grade color utilities)
- Add navigation-helpers.test.ts (17 tests for navigation helpers)
- Add navigate-queue.test.ts (15 tests for navigateQueue mutation)
- Update test setup to include esp32_controllers table
- Fix controller.test.ts to create test users for FK constraints

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@marcodejongh