Skip to content

feat(switch2_pro): Nintendo Switch 2 Pro Controller BLE emulation - #765

Open
finger563 wants to merge 29 commits into
mainfrom
feat/switch2-pro
Open

feat(switch2_pro): Nintendo Switch 2 Pro Controller BLE emulation#765
finger563 wants to merge 29 commits into
mainfrom
feat/switch2-pro

Conversation

@finger563

@finger563 finger563 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Warning

Do not merge until h2zero/esp-nimble-cpp#443 is merged and released.
This PR bumps the esp-nimble-cpp submodule to a branch commit that carries the new
NimBLEServer::registerServicesFirst() API (needed for the exact GATT handle layout).
Once #443 is released, the submodule pin moves to the released commit and the manifest
dependency is bumped. Opening now for review.

Summary

Adds a new switch2_pro component that emulates a Nintendo Switch 2 Pro Controller
over BLE
so a real Switch 2 console accepts it as a native controller — pairing,
encrypted link, continuous input streaming, reconnect, and wake-from-sleep. Built on
espp::BleGattServer (NimBLE); implements the reverse-engineered Nintendo custom GATT
interface (not HID-over-GATT) and the console's custom pairing handshake (not BLE SMP).

Status

  • ESP32-C6 (and the other open-NimBLE-controller chips — C61/C2/H2): fully working,
    verified against a real console. Pairs (full battery + correct icon), streams input
    lag-free at ~62 Hz, buttons/sticks register on "Test Input Devices", and reconnect +
    wake-from-sleep work without re-pairing.
  • ESP32-S3: also fully working on ESP-IDF ≥ v6.1 — verified end-to-end against a real
    console (pairing, continuous lag-free input, reconnect, wake). The S3 needs the
    console's sub-spec 5 ms interval, which is now official in ESP-IDF via
    CONFIG_BT_CTRL_BLE_MIN_CONN_INTERVAL_ENABLE (default on; ESP32-S3 Bluedroid Controller Support for 5ms Connection Interval (Nintendo Switch Pro2 Controller Compatibility) (IDFGH-17532) espressif/esp-idf#18467,
    backported to v6.0/v5.5/v5.4/v5.3) — no binary patch required on a current IDF. The
    earlier "closed BTDM controller degrades the encrypted stream" finding was on the
    pre-fix (v6.0.1) controller lib; v6.1 ships an updated lib and the stream now holds
    (see Testing below).

What's included

  • The switch2_pro component: pairing crypto (known-answer verified) + LTK injection,
    the exact GATT handle layout, the full console init/command sequence, NVS bond
    persistence, continuous input streaming with real (mbuf-pool-based) backpressure, and
    a wake_console() API.
  • A small, additive ble_gatt_server enhancement: conn_params_update_callback.
  • esp-nimble-cpp submodule bump for registerServicesFirst() (see the warning above).
  • Two paths for the console's sub-spec 5 ms connection interval (required for
    sustained input in every mode — the console drops even the fresh session to 5 ms
    mid-stream — plus reconnect/wake):
    • S3/C3: the official, default-on CONFIG_BT_CTRL_BLE_MIN_CONN_INTERVAL_ENABLE
      (ESP-IDF ≥ v6.1). A configure-time CMake check warns if it (or the legacy patch) is
      missing on an older IDF.
    • C6/C61/C2/H2: the opt-in SWITCH2_PRO_PATCH_NIMBLE_5MS controller patch (off by
      default — the open NimBLE controller has no equivalent config option yet) + a
      hardware-free smoke_test_5ms.py verifier.
  • Docs (doc/en/ble/switch2_pro*, Doxyfile), CI matrix entries (C6 + S3), example, and
    DESIGN/README.

Testing

  • On-hardware against a real Switch 2 on ESP32-C6: pairing, input, reconnect, wake —
    all working; input stays lag-free at the console's cadence.
  • On-hardware against a real Switch 2 on ESP32-S3 (ESP-IDF v6.1): a full session —
    fresh pair → input testing → power the console off and back on → reconnect +
    input still working — with the official option (no patch). Annotated log below.
  • The example builds clean for both esp32c6 and esp32s3. The 5 ms patch is off by
    default so CI builds do not mutate the IDF install.

The S3 log also revised a protocol assumption: the console does renegotiate the
fresh session down to 5 ms (CONN PARAMS UPDATE: itvl=5.00ms, ~1.5 s after
subscription), rather than staying at 15 ms. That timed switch — which the pre-v6.1 S3
controller couldn't apply — is the unified root cause of the old "~3 s tx-stall", and it
is the same sub-spec-interval requirement as reconnect/wake, just arriving mid-session.

ESP32-S3 / v6.1 session log (repetitive heartbeat lines elided)
[Switch2Pro/I][0.223]: pairing crypto self-test passed
I (226) BLE_INIT: Bluetooth MAC: f4:12:fa:5a:85:92
[Switch2Pro/I][0.289]: BLE address: using PUBLIC
[Switch2Pro/I][0.290]:   common_input  = 0x000a (0x000a)
[Switch2Pro/I][0.291]:   pro2_input    = 0x000e (0x000e)
[Switch2Pro/I][0.291]:   command       = 0x0014 (0x0014)
[Switch2Pro/I][0.291]:   vib_command   = 0x0016 (0x0016)
[Switch2Pro/I][0.292]:   resp1         = 0x001a (0x001a)
[Switch2Pro/I][0.292]:   resp2         = 0x001e (0x001e)
[Switch2Pro/I][0.296]: Switch2Pro advertising as 'Pro Controller'

# --- fresh pairing, connects at 15 ms ---
[Switch2Pro/I][5.331]: connected: peer=C8:48:05:64:A0:BB interval=15.00ms supervision=2000ms latency=0
[Switch2Pro/I][6.503]: pairing: exchange addresses -> replied with our address 92 85 5a fa 12 f4 (little-endian)
[Switch2Pro/I][6.532]: pairing: exchange keys -> LTK derived, replied B1
[Switch2Pro/I][6.578]: pairing: confirm -> replied B2
[Switch2Pro/I][6.607]: pairing: finalised — bonded
[Switch2Pro/I][6.611]: injected LTK into NimBLE store (rc=0) — ready for LL encryption
[Switch2Pro/I][6.613]: saved bond to NVS (console addr + LTK)
[Switch2Pro/I][6.710]: AUTH complete: encrypted=true bonded=true authenticated=true
[Switch2Pro/I][7.176]: SUBSCRIBE pro2_input(0x000e) value=0x0001 (on)
[Switch2Pro/I][7.176]: input-report streaming ENABLED (0x000e)

# --- console drops the FRESH session to 5 ms ~1.5 s after streaming begins ---
[Switch2Pro/I][7.699]: LINK CHANGE: itvl=15.00ms latency=0 timeout=2000ms tx_phy=2 rx_phy=2
[Switch2Pro/I][8.666]: CONN PARAMS UPDATE: itvl=5.00ms latency=0 timeout=2000ms
[Switch2Pro/I][8.717]: LINK CHANGE: itvl=5.00ms latency=0 timeout=2000ms tx_phy=2 rx_phy=2

# --- streams fine at 5 ms; BOOT press registers as A ---
[switch2_pro example/I][32.944]: connected=true streaming=true A(boot)=true L+R(auto)=false
   ... (continuous streaming at 5 ms) ...

# --- console powered off ---
[Switch2Pro/W][40.620]: disconnected: peer=C8:48:05:64:A0:BB reason=REMOTE_USER_TERMINATED (paired=true)
[Switch2Pro/I][40.623]: advertising (reconnect): flags+mfr(26 B) in adv, name in scan response

# --- BOOT press broadcasts wake; console powers back on and reconnects straight at 5 ms ---
[Switch2Pro/I][54.114]: wake: broadcasting wake advertisement (user-requested)
[switch2_pro example/I][54.118]: BOOT pressed while disconnected -> sent wake advertisement
[Switch2Pro/I][56.801]: connected: peer=C8:48:05:64:A0:BB interval=5.00ms supervision=2000ms latency=0
[Switch2Pro/I][57.225]: AUTH complete: encrypted=true bonded=true authenticated=true
[Switch2Pro/I][57.227]: input-report streaming ENABLED (0x000e)
[Switch2Pro/I][57.731]: LINK CHANGE: itvl=5.00ms latency=0 timeout=2000ms tx_phy=2 rx_phy=2

# --- input works again after wake/reconnect; BOOT press registers as A ---
[switch2_pro example/I][63.454]: connected=true streaming=true A(boot)=true L+R(auto)=false

Attribution / scope

Interoperability only — no Nintendo or Espressif binaries are included. The pairing
"authentication" relies on a published fixed key (a possession check, not per-device
attestation). Protocol reverse-engineering credit: the community, principally
ndeadly/switch2_controller_research; the NimBLE 5 ms patch technique is adapted (MIT)
from zhantss/ESP32-BLE5-NSController-Emulator.

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings September 3, 2026 15:46
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

✅Static analysis result - no issues found! ✅

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

There are several correctness/documentation mismatches and missing error-handling paths (paired-state semantics, pthread config error checking, PSA crypto status handling, and dependency/version alignment) that should be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new switch2_pro component to ESPP that emulates a Nintendo Switch 2 Pro Controller over BLE (custom GATT + custom pairing/crypto + continuous input streaming), plus a small ble_gatt_server enhancement to support the required GATT/connection-parameter behavior, along with docs and CI coverage for the new example.

Changes:

  • Introduces the components/switch2_pro component (protocol constants, pairing crypto/self-test, GATT layout, advertising variants, bond persistence, and an input streaming task with backpressure).
  • Extends espp::BleGattServer with a connection-parameters-update callback and an option to disable built-in DIS/BAS services (needed for strict handle layout emulation).
  • Adds documentation pages, Doxygen inputs, and CI build matrix entries for the new example (ESP32-C6 + ESP32-S3).
File summaries
File Description
doc/en/ble/switch2_pro.rst New Sphinx page documenting the Switch 2 Pro BLE emulation component and 5 ms interval patch.
doc/en/ble/switch2_pro_example.md Includes the example README into the docs site.
doc/en/ble/index.rst Adds switch2_pro docs pages to the BLE docs toctree.
doc/Doxyfile Adds Switch2Pro headers and example to Doxygen INPUT/EXAMPLE_PATH.
components/switch2_pro/tools/smoke_test_5ms.py Hardware-free verifier for the 5 ms controller patch (disassembly-based).
components/switch2_pro/tools/patch_nimble_5ms.py Opt-in patcher that edits ESP-IDF controller archives to accept 5 ms intervals.
components/switch2_pro/src/switch2_pro.cpp Core implementation: GATT layout, pairing handling, bond persistence, advertising, and input streaming/backpressure.
components/switch2_pro/src/switch2_pro_pairing.cpp Pairing crypto implementation using PSA Crypto API + self-test.
components/switch2_pro/README.md Component README covering usage, 5 ms patch rationale, and known issues.
components/switch2_pro/Kconfig Adds SWITCH2_PRO_PATCH_NIMBLE_5MS opt-in configuration.
components/switch2_pro/include/switch2_pro.hpp Public API for espp::Switch2Pro and its configuration/streaming behavior.
components/switch2_pro/include/switch2_pro_report.hpp Defines the packed Pro Controller 2 input report helper.
components/switch2_pro/include/switch2_pro_protocol.hpp Protocol constants: UUIDs, command IDs, feature bits, manufacturer data, golden vectors.
components/switch2_pro/include/switch2_pro_pairing.hpp Pairing crypto interface and self-test API.
components/switch2_pro/include/switch2_pro_motion.hpp Embedded captured IMU motion sequence for optional replay.
components/switch2_pro/include/switch2_pro_flash.hpp Embedded captured flash blocks and a simulated flash reader.
components/switch2_pro/idf_component.yml Component Manager manifest for switch2_pro.
components/switch2_pro/example/sdkconfig.defaults.esp32c6 ESP32-C6-specific sdkconfig defaults (libc/toolchain workaround notes).
components/switch2_pro/example/sdkconfig.defaults Example defaults for BLE/NimBLE tuning and patch guidance.
components/switch2_pro/example/README.md Example walkthrough (pairing, streaming, wake/reconnect instructions).
components/switch2_pro/example/partitions.csv Partition table enabling NVS for bond persistence.
components/switch2_pro/example/main/switch2_pro_example.cpp Example app implementing BOOT-as-A and wake-on-press logic.
components/switch2_pro/example/main/CMakeLists.txt Registers the example main component.
components/switch2_pro/example/CMakeLists.txt Example project wiring for ESP-IDF build and components list.
components/switch2_pro/DESIGN.md Design notes, protocol sources/attribution, and patch rationale.
components/switch2_pro/CMakeLists.txt Component build registration + opt-in patch invocation at configure time.
components/switch2_pro/.gitignore Ignores example build artifacts and generated sdkconfig files.
components/ble_gatt_server/src/ble_gatt_server_callbacks.cpp Plumbs NimBLE conn-param-update event into server callbacks.
components/ble_gatt_server/include/ble_gatt_server.hpp Adds conn-param-update callback typedef + togglable DIS/BAS creation/start/deinit.
components/ble_gatt_server/include/ble_gatt_server_callbacks.hpp Declares the new onConnParamsUpdate callback override.
.github/workflows/build.yml Adds CI build matrix entries for switch2_pro/example (C6 + S3).
Review details
  • Files reviewed: 32/32 changed files
  • Comments generated: 6
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread components/switch2_pro/idf_component.yml
Comment thread components/switch2_pro/src/switch2_pro.cpp
Comment thread components/switch2_pro/src/switch2_pro.cpp Outdated
Comment thread components/switch2_pro/src/switch2_pro_pairing.cpp
Comment thread components/switch2_pro/include/switch2_pro.hpp Outdated
Comment thread components/switch2_pro/include/switch2_pro.hpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical synchronization, lifecycle, handshake, and configuration issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (13)

Previously missed (12) — in code that hasn't changed since the last review.

components/switch2_pro/example/main/switch2_pro_example.cpp:31

  • Initialization failures are ignored, including failure of the erase/retry path, so the example proceeds even though this component's advertised reconnect/wake behavior cannot persist a bond. Check the final NVS status before constructing the controller.
    components/switch2_pro/include/switch2_pro_report.hpp:82
  • At the documented neutral input v == 0, this expression truncates 4095 / 2 to 2047, despite STICK_CENTER being 2048. Consequently every default/reset report encodes both sticks one count off center. Scale each half around STICK_CENTER so the endpoints remain 0/4095 and zero maps exactly to 2048.
    components/switch2_pro/src/switch2_pro.cpp:425
  • The underlying API returns false when advertising cannot start, but that result is discarded and success is logged; wake_console() likewise returns true unconditionally. This makes callers believe the wake/discovery advertisement was issued when it was not. Return and propagate the start result through start_advertising(), advertise(), wake_console(), and initialization.
    components/switch2_pro/src/switch2_pro.cpp:445
  • wake_console_on_boot_ is immutable and remains true after the first successful connection. With the default configuration, every later disconnect therefore immediately advertises the wake variant, and the persistent timer resumes doing so every five seconds, potentially waking a console the user just put to sleep. Use a one-shot wake-pending latch initialized at boot and clear/cancel it on successful connection, as the public configuration contract states.
    components/switch2_pro/src/switch2_pro.cpp:788
  • Byte 0x0b is forced to the rumble-enabled value even when FEATURE_SELECT has not enabled rumble or has explicitly disabled it. This contradicts Pro2InputReport::reset() and the feature-state contract in the header, and sends a feature state different from the negotiated mask.
    components/switch2_pro/src/switch2_pro.cpp:853
  • A failed ble_store_write_our_sec is logged at INFO as “ready,” and callers continue to mark/save the pairing even though the controller cannot answer the upcoming encryption request. Propagate this failure from inject_ltk() so finalization/reconnect can fail explicitly rather than reporting a usable bond.
    components/switch2_pro/src/switch2_pro.cpp:885
  • The return values from nvs_set_blob and nvs_commit are ignored, yet the method logs success and updates the in-memory bond even when persistence failed. This makes pairing appear durable while reconnect/wake fails after reboot. Only publish success after both operations complete successfully.
    components/switch2_pro/tools/patch_nimble_5ms.py:205
  • Targets with two controller archives are modified one at a time. If the first archive is patched and validation of the second archive fails, the tool exits after leaving the global ESP-IDF installation partially patched. Preflight every archive and create all backups before writing any archive, or roll back already-written archives on failure.
    components/switch2_pro/tools/smoke_test_5ms.py:99
  • The smoke test documents exit code 2 for tool errors, but failures from ar raise CalledProcessError, missing tools raise OSError, and objdump failures are silently treated as empty output; the first two terminate with Python's exit code 1, which is documented as “unpatched.” Catch tool-execution failures and consistently return the indeterminate/error status 2.
    components/switch2_pro/DESIGN.md:43
  • This states that every link is driven at 5 ms and cannot stream without the patch, contradicting the verified behavior documented elsewhere in this PR: fresh pairing and first-session streaming use 15 ms, while only bonded reconnect/wake starts at 5 ms. Clarify the distinction here because it changes whether the invasive patch is required.
    components/switch2_pro/include/switch2_pro.hpp:195
  • This says on-change mode is the default, but Config::continuous_streaming defaults to true. Update the method documentation so generated API docs describe the actual default behavior.
    components/switch2_pro/include/switch2_pro_protocol.hpp:40
  • The protocol comment declares this characteristic as WRITE, but build_gatt() intentionally creates it with NIMBLE_PROPERTY::WRITE_NR to match the real controller. Correct the public protocol documentation to avoid sending users toward the wrong GATT layout.

components/switch2_pro/DESIGN.md:85

  • This says absolute handles are not strict and “we discover by UUID,” whereas the implementation and required registerServicesFirst() API are explicitly based on the console using fixed handles without discovery. Resolve this contradiction; it changes whether the exact GATT ordering is a protocol requirement or merely emulation fidelity.
Two proprietary primary services; contiguous handles matter for some console
firmwares (FW 2.0.0+ shifts them +8 for headset audio, so absolute-handle dependence
is not strict — we reproduce the map but discover by UUID).
  • Files reviewed: 32/32 changed files
  • Comments generated: 9
  • Review effort level: Balanced

Comment thread components/switch2_pro/include/switch2_pro.hpp Outdated
Comment thread components/switch2_pro/src/switch2_pro.cpp
Comment thread components/switch2_pro/src/switch2_pro.cpp Outdated
Comment thread components/switch2_pro/src/switch2_pro.cpp
Comment thread components/switch2_pro/src/switch2_pro.cpp
Comment thread components/switch2_pro/CMakeLists.txt
Comment thread .github/workflows/build.yml Outdated
Comment thread doc/Doxyfile Outdated
Comment thread doc/Doxyfile Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Multiple critical and moderate issues remain, and merging is gated on an unreleased upstream NimBLE API.

Review details

Suppressed comments (11)

Previously missed (8) — in code that hasn't changed since the last review.

components/switch2_pro/example/main/switch2_pro_example.cpp:31

  • This example proceeds even if NVS initialization or the erase/retry fails. The controller will then appear to pair but cannot persist its bond, so the documented reboot reconnect/wake flow breaks. Check both operations and the final initialization result, as the existing NVS initialization pattern does in components/st25dv/example/main/st25dv_example.cpp:53-59.
    components/switch2_pro/example/sdkconfig.defaults:21
  • The default target is ESP32-C6, whose CPU supports at most 160 MHz, so these 240 MHz assignments are not valid for the default build and are ignored rather than providing the claimed headroom. Keep 240 MHz in an S3-specific defaults file and configure 160 MHz in sdkconfig.defaults.esp32c6.
    components/switch2_pro/include/switch2_pro_report.hpp:82
  • The conversion truncates the normalized midpoint: axis_to_u12(0.0f) produces 2047 (0x7ff) even though STICK_CENTER and the public API define center as 2048 (0x800). Round the scaled value so a default/centered report encodes the declared midpoint.
    components/switch2_pro/src/switch2_pro.cpp:911
  • The return values from both NVS writes are ignored, so a full/corrupt flash can make this function log “saved bond” even though reconnect-after-reboot will fail. Check nvs_set_blob and nvs_commit before updating the cached peer fields or reporting success.
    components/switch2_pro/tools/patch_nimble_5ms.py:225
  • An existing .original is reused forever. If ESP-IDF is upgraded in place, the new unpatched archive is patched while the old-version backup remains; a later --restore then installs an archive from the previous IDF version. Because this branch has already verified that the current archive contains the unpatched pattern, refresh the backup from it before each new patch.
    components/switch2_pro/tools/smoke_test_5ms.py:125
  • The module documents exit code 2 for tool/extraction errors, but an unavailable ar/objdump or failed extraction raises an uncaught subprocess/OSError and Python exits with 1—the same code documented for a valid unpatched controller. This makes automation misclassify verifier failures as an unpatched result; normalize those exceptions to exit 2.
    components/switch2_pro/include/switch2_pro.hpp:195
  • This documentation says on-change mode is the default, but Config::continuous_streaming defaults to true. Generated API docs therefore describe the opposite runtime behavior.
    components/switch2_pro/include/switch2_pro_protocol.hpp:41
  • The protocol header documents this characteristic as WRITE, but build_gatt() intentionally creates it as WRITE_NR to match the real controller. Update the public wire-protocol documentation so consumers do not implement the wrong property.

components/switch2_pro/DESIGN.md:94

  • This table also says the firmware-update characteristic is WRITE, while the implementation and its source comment use WRITE_NR. Keep the design reference aligned with the actual GATT table.
      4147423d-…       WRITE         firmware update (large)

components/switch2_pro/DESIGN.md:43

  • This design statement contradicts the implemented and documented behavior elsewhere: fresh pairing and first-session input run at 15 ms without the patch; only bonded reconnect/wake begins at 5 ms. As written, it incorrectly tells readers that all input streaming requires the controller patch.
The console drives the link at a **5 ms** connection interval — below the 7.5 ms BLE
spec minimum. The controller stack must accept it or the console won't stream input.

components/switch2_pro/DESIGN.md:85

  • This says absolute handles are not strict and that UUID discovery is used, but the component and its required registerServicesFirst() dependency are explicitly built around the console addressing fixed handles without discovery. That contradiction obscures the core GATT-layout requirement.
Two proprietary primary services; contiguous handles matter for some console
firmwares (FW 2.0.0+ shifts them +8 for headset audio, so absolute-handle dependence
is not strict — we reproduce the map but discover by UUID).
  • Files reviewed: 33/33 changed files
  • Comments generated: 7
  • Review effort level: Balanced

Comment thread components/switch2_pro/idf_component.yml
Comment thread components/switch2_pro/src/switch2_pro.cpp Outdated
Comment thread components/switch2_pro/tools/patch_nimble_5ms.py Outdated
Comment thread components/switch2_pro/tools/smoke_test_5ms.py Outdated
Comment thread components/switch2_pro/src/switch2_pro.cpp Outdated
Comment thread components/switch2_pro/src/switch2_pro.cpp Outdated
Comment thread components/switch2_pro/src/switch2_pro.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Critical patching defects and unresolved concurrency, persistence, wake-state, encoding, and configuration issues require changes and human review.

Review details

Suppressed comments (7)

Previously missed (7) — in code that hasn't changed since the last review.

components/switch2_pro/example/sdkconfig.defaults:21

  • The default target is ESP32-C6, whose CPU tops out at 160 MHz, but these common defaults request 240 MHz. The C6 build ignores/coerces this unsupported setting, so the documented performance configuration is not actually applied. Move the 240 MHz settings to S3-specific defaults and configure 160 MHz for C6.
    components/switch2_pro/include/switch2_pro.hpp:101
  • is_paired() can run in the application task while the NimBLE command callback assigns paired_ during FINALISE. Because paired_ is a plain bool, that is a C++ data race; make it atomic (and load it explicitly at logging/getter sites) or protect it with the connection-state lock.
    components/switch2_pro/include/switch2_pro_report.hpp:82
  • The conversion truncates neutral 0.0 to 0x7ff, even though this class defines STICK_CENTER as 0x800 and reset() uses 0.0 for centered sticks. Round to the nearest 12-bit value so default reports encode the declared center.
    components/switch2_pro/src/switch2_pro.cpp:261
  • These diagnostics are owned and updated by the streaming thread, but the NimBLE disconnect callback reads stream_start_us_, enomem_count_, and wedge_reported_ concurrently before stopping the stream. The remaining unsynchronized reads are undefined behavior; take a synchronized snapshot or make every cross-thread diagnostic atomic.
    components/switch2_pro/src/switch2_pro.cpp:932
  • Both persistence operations are ignored, so a full/read-only NVS partition is still logged as a saved bond even though reconnect after reboot will fail. Check nvs_set_blob and nvs_commit before updating the cached peer or reporting success.
    components/switch2_pro/include/switch2_pro.hpp:196
  • The API documentation labels on-change mode as the default, but Config::continuous_streaming defaults to true. Update this description so users understand that the component continuously sends every interval unless they opt out.
    components/switch2_pro/include/switch2_pro_protocol.hpp:41
  • This public protocol comment says the firmware-update characteristic supports WRITE, but build_gatt() creates it with NIMBLE_PROPERTY::WRITE_NR. Documenting the wrong property can produce an incompatible GATT table for consumers reusing these constants.
  • Files reviewed: 33/33 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread components/switch2_pro/tools/patch_nimble_5ms.py Outdated
Comment thread components/switch2_pro/tools/patch_nimble_5ms.py Outdated
Comment thread components/switch2_pro/src/switch2_pro.cpp Outdated
finger563 and others added 15 commits September 3, 2026 23:11
…r BLE emulation

New component that emulates a Nintendo Switch 2 Pro Controller as a BLE
peripheral so a real Switch 2 accepts it as a native controller (and can be
woken from sleep). Unlike the original Switch (BT Classic HID), the Switch 2
uses a proprietary BLE GATT interface (not HID-over-GATT) and a custom pairing
handshake (not SMP), so this builds custom Nintendo GATT services directly on
espp::BleGattServer rather than hid_service/hid-rp.

This first milestone:
- custom GATT service tree (two proprietary services + input/command/response
  characteristics) with no-SMP security config
- Nintendo manufacturer-data advertising (+ wake-flag variant scaffolding)
- reverse-engineered pairing crypto (LTK = A1 ^ B1, B2 = AES-128-ECB via PSA
  Crypto), self-tested against a host-verified known-answer vector at init
- Pro Controller 2 input report (0x09) struct incl. the C / GL / GR buttons
- opt-in, target-gated NimBLE 5 ms connection-interval patch (tools/ + Kconfig),
  off by default and never mutating $IDF_PATH silently

Builds and links for ESP32-C6. Protocol facts from
ndeadly/switch2_controller_research; approach + NimBLE patch adapted (MIT) from
zhantss/ESP32-BLE5-NSController-Emulator. See DESIGN.md for milestones 2-4.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…estone 2)

The command channel now replies, completing the pairing handshake on the wire
and answering the console's init sequence — the part a real console needs to
accept the controller.

- send device->host responses on the matching notify characteristic (writes on
  0x0016 reply on 0x001e, writes on 0x0014 reply on 0x001a)
- full 0x15 pairing handshake replies with exact captured framing: exchange
  addresses (our BT address), exchange keys (fixed B1), confirm (B2 = AES-ECB),
  finalise ACK — bytes verified against ndeadly's captures
- command dispatch: flash/calibration reads (0x02) from a simulated flash,
  feature-select (0x0c) mask capture, firmware-info (0x10) canned reply,
  firmware-update (0x0d) ACK to suppress the update prompt, and header-only
  ACKs for init/LEDs/vibration/battery so the console's state machine advances
- simulated flash (switch2_pro_flash.hpp) with neutral stick calibration
  (placeholder; refine against a capture for exact stick behavior)

Builds and links for ESP32-C6. Uncertain-on-hardware bits (exchange-address
byte order, calibration contents, firmware-update suppression) are marked in
code for refinement against a real console. Next: input report streaming
(milestone 3, needs the 5 ms NimBLE patch) and wake-from-sleep (milestone 4).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…iring tests

Make the example flashable on ESP32-S3 (what's commonly on hand) and observable
enough to see how far pairing with a real Switch 2 gets, since S3 can't accept
the console's 5 ms interval and may drop mid/post-pairing.

- example defaults to esp32s3 (builds clean; the C6 GCC 15.2 attribute issue is
  RISC-V-only, so S3 is unaffected) with USB-Serial-JTAG console
- connect/disconnect callbacks log the negotiated connection interval,
  supervision timeout, and disconnect reason — the interval is the 5 ms
  diagnostic, the reason shows why a link dropped
- DEBUG-level hex trace of every command write (0x0014/0x0016) and response
  (0x001a/0x001e), so the 0x15 pairing exchange is visible on the monitor
- re-advertise on disconnect
- example README: step-by-step pairing test and what to expect on S3 vs C6

Note: S3 pairing may not complete/hold (5 ms limit); the trace shows how far it
gets, which is the useful signal. Full path remains C6/C61 + the NimBLE patch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rtisement

The controller wasn't visible to the console because the advertisement
overflowed the 31-byte legacy limit: flags (3) + name "Pro Controller" (16) +
manufacturer data (22) = 41 bytes, so NimBLE dropped/truncated it — and the
console filters discovery on the Nintendo manufacturer data, which was the part
getting lost.

Put flags + manufacturer data (25 bytes, fits) in the primary advertisement and
move the device name to the scan response. Flags now 0x06 (LE General Disc +
BR/EDR unsupported), matching the captured discovery advertisement. Log the
manufacturer-data fit and warn if it still doesn't.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The NS2 now discovers and connects (stable at a 15ms interval — so the 5ms
wall isn't hit during pairing on S3), but doesn't start the pairing exchange.
We were blind to what it does post-connect (only the two command chars were
logged).

Attach a tracing callback to every custom characteristic that logs reads,
writes (with hex), and notification subscribes at INFO. This shows whether the
console is subscribing to our response/input characteristics and what, if
anything, it writes — the data needed to find why pairing doesn't start.

Hex dump is INFO during bring-up (dial back later).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The console connects but does no value reads/writes/subscribes on our
characteristics — consistent with it doing ATT service discovery (invisible to
characteristic callbacks) and then declining to proceed.

- log the actual handle each service/characteristic landed on vs the real
  controller's handles (0x000a/0x000e/0x0014/0x0016/0x001a/0x001e). If
  BleGattServer's GAP/GATT/DeviceInfo/Battery services shifted ours off those
  handles, and the console keys off them, that's the cause.
- enable NimBLE host INFO logging (+ compile-in debug) so GAP/ATT activity
  (MTU, discovery, connection-parameter updates, subscribes) is visible at the
  stack level.

Diagnostic only; revert the log levels once understood.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Prior round's handle dump read 0x0000 because NimBLE assigns handles only after
the server starts; move it to after ble_gatt_server_.start(). Turn the NimBLE
host log to DEBUG for its tags only (esp_log_level_set, so ATT discovery/reads/
writes show without flooding other components). Log authentication_complete so
we can tell if the console is (unexpectedly) running BLE SMP.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The prior config set CONFIG_BT_NIMBLE_LOG_LEVEL to INFO, which compiles out
NimBLE's ATT/GATT-server DEBUG logs entirely — so the console's service
discovery / reads / writes never printed regardless of runtime level. Set it to
DEBUG and raise the runtime default level. Verified the generated sdkconfig now
has CONFIG_BT_NIMBLE_LOG_LEVEL=0 and CONFIG_LOG_DEFAULT_LEVEL=4.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The NS2 does app-level pairing (the 0x15 command exchange), not BLE SMP. Our
bonding=true config created BLE bonds; the console then used GATT caching over
the bond and skipped service discovery on reconnect, getting stuck after the
MTU exchange. Set bonding=false and clear any stored bonds at startup so the
console re-discovers our GATT cleanly on each connection.

Diagnostics from this round also confirmed our characteristics are shifted off
the real controller's handles (command at 0x0033 vs 0x0014) because
BleGattServer registers GAP/GATT/DeviceInfo/Battery first — tracked for the
likely raw-NimBLE GATT rework if the console keys off fixed handles.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…chive)

The C6 controller archives are GNU-format (long-name symbol/string tables);
macOS's BSD ar fails to extract them ("File exists" on the / and // members),
which silently broke the patcher on macOS. Prefer the RISC-V toolchain's
riscv32-esp-elf-ar (on PATH after the IDF export script), then llvm-ar, then ar;
add an --ar override.

Verified end-to-end on IDF 6.0.1: ble_ll_conn.c.o has exactly one 7.5ms-floor
pattern; patch -> 5ms -> restore round-trips cleanly. Confirms the 5 ms patch
applies on IDF 6.0.1, not just the 5.5.3 the reference used.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cesFirst

Points the esp-nimble-cpp submodule at esp-cpp's feat/register-services-first
(also fast-forwards it to upstream h2zero 2.5.0). This pulls in the new opt-in
NimBLEServer::registerServicesFirst() API that switch2_pro needs to place its
Nintendo services at the low attribute handles a real console addresses by
fixed handle.

Upstream PR: h2zero/esp-nimble-cpp#443. Do not merge this espp change until that
PR is merged and released; the pin will then move to a released commit.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Surface NimBLE's BLE_GAP_EVENT_CONN_UPDATE via a new optional
conn_params_update_callback on BleGattServer::Callbacks, forwarded from a
BleGattServerCallbacks::onConnParamsUpdate override. Lets applications observe
connection-parameter updates (interval/latency/timeout) as they complete.
Additive and optional; existing users are unaffected.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, docs, CI

Complete the switch2_pro component so a real Nintendo Switch 2 accepts it as a
native Pro Controller. Verified end-to-end on ESP32-C6: pairing, encrypted
link, continuous input streaming (~62 Hz, matching a real controller), reconnect,
and wake-from-sleep.

Highlights:
- Streaming model: set_input_report() stores latest state; a driver-owned task
  streams it (continuous by default, on-change fallback) with real backpressure
  keyed on the host mbuf pool (NimBLE NOTIFY_TX fires at handoff, not over-air,
  so it can't gate a backlog — the pool level can).
- Correct report/init: firmware-info identity, always-0x38 byte 0x0b, all-zero
  IMU motion block, feature-mask restore on reconnect, exact GATT handle layout
  via NimBLEServer::registerServicesFirst().
- Reconnect + wake: bonded reconnect and 0x81 wake advertisement (public
  wake_console() API); needs the opt-in 5 ms controller patch (off by default).
- Tooling: patch_nimble_5ms.py (C6/C61/C2/H2 NimBLE + S3/C3 BTDM) and
  hardware-free smoke_test_5ms.py verifier.
- Docs (doc/en/ble/switch2_pro*, Doxyfile), CI matrix (C6 + S3), example, and
  README/DESIGN updated. C6 is the supported target; ESP32-S3 builds and pairs
  but does not yet stream reliably (closed BTDM controller) — documented as a
  known issue / fast-follow.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- confirm(): check psa_crypto_init()/psa_cipher_encrypt() status and out_len;
  log and return zeroed output on failure instead of a silent partial block.
- init(): check esp_pthread_set_cfg() and warn if the streaming thread can't get
  its configured stack/prio/core.
- disconnect: stop clearing paired_ — is_paired() reports bond/handshake existence
  which survives a disconnect (use is_connected()/is_input_streaming() for session
  state).
- flash read: use std::find_if instead of a raw block-search loop (cppcheck).
- docs: refresh the class-level status comment (no longer a "skeleton"); fix the
  send_ack() doc to match the on-air bytes (0x10/0x78, no payload).
- manifest: note the unreleased esp-nimble-cpp registerServicesFirst() dependency.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- data races: make the cross-thread session state atomic (input_subscribed_,
  active_conn_handle_, enabled_features_) and move the grouped per-session
  counter/link-baseline resets out of on_subscribe into the streaming thread's
  session-start, so they stay single-writer.
- lifecycle: in ~Switch2Pro() stop/join the wake timer and detach the
  this-capturing GAP/GATT callbacks (+ stop advertising) before members are
  destroyed, so a late callback can't touch freed state.
- pairing: gate FINALISE on an in-order handshake (pairing_stage_ must reach 3:
  address-exchange -> key-exchange -> confirm) so a malformed/out-of-order peer
  can't persist an all-zero/partial bond.
- portability: #error if CONFIG_BT_NIMBLE_EXT_ADV is enabled (this component uses
  the legacy advertising path).
- register the component in upload_components.yml; fix alphabetical ordering in
  build.yml and Doxyfile (switch2_pro before sx126x; headers main/protocol/report).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- init(): guard against a second call (assigning to a joinable std::thread
  would std::terminate) — treat an already-initialized instance as a no-op.
- Disconnect telemetry: make stream_start_us_, enomem_count_, wedge_reported_
  atomic (read on the disconnect/host thread while the stream thread writes).
- inject_ltk()/inject_pairing_ltk(): return success; on a nonzero
  ble_store_write_our_sec result, log an error and let callers know the link
  cannot encrypt instead of logging "ready".
- save_bond(): check nvs_set_blob/nvs_commit; on failure log an error and keep
  the in-memory bond for this boot (reconnect after reboot won't work).
- example: fail fast on nvs_flash_init()/erase errors (reconnect depends on NVS).
- Stick packing: round instead of truncate so a neutral axis maps to the
  declared midpoint 2048 (not 2047); +/-1 still map to 0/4095.
- CPU frequency: move out of the base defaults (240 MHz is invalid for the
  default C6 target) into per-target files — esp32c6 = 160 MHz, esp32s3 = 240.
- patch_nimble_5ms.py: require exactly one old / zero new pattern to patch (and
  exactly one new / zero old for the already-patched case) — no ambiguous or
  mixed-object patching.
- smoke_test_5ms.py: map string sys.exit() and unexpected errors to exit code 2
  so "error" is distinct from the "unpatched" (1) result, per the documented
  contract.
- Docs: continuous_streaming is the default (fix inverted comment); firmware-
  update characteristic is WRITE_NR, not WRITE; refresh the class-level S3 status.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unresolved critical and moderate issues affect 5 ms support, streaming reliability, and address handling.

Review details

Suppressed comments (6)

Previously missed (3) — in code that hasn't changed since the last review.

components/switch2_pro/src/switch2_pro.cpp:136

  • The static-random fallback ignores both NimBLE return values and then logs success. If either call fails, pairing proceeds with an address that may not match the persisted identity, defeating reconnect/wake; fail init() instead of claiming the stable address was installed.

This issue also appears on line 467 of the same file.
components/switch2_pro/src/switch2_pro.cpp:807

  • interval_tick_ is never reset when a stream stops, so with a divisor greater than 1 a new subscription may wait up to N−1 intervals for its first report despite the idle path promising a fresh initial send. Reset the phase on the first tick of each streaming run.
    components/switch2_pro/src/switch2_pro.cpp:820
  • This pacing relies on the application's FreeRTOS tick rate, but component consumers do not inherit the example's CONFIG_FREERTOS_HZ=1000. At the normal 100 Hz setting, 5 ms and 15 ms sleeps round to roughly 10 ms and 20 ms, so the published component cannot provide its documented per-connection-interval stream cadence. Either enforce/document the required tick configuration for consumers or use a high-resolution pacing mechanism.

components/switch2_pro/src/switch2_pro.cpp:254

  • This comment still says the console remains at 15 ms and only reconnect obtains 5 ms in CONNECT_IND, directly contradicting the newly documented fresh-session LL_CONNECTION_UPDATE to 5 ms. Keeping this beside the connection callback will misdirect future protocol work; describe both the fresh-session update and reconnect paths.
    // NOTE: we deliberately do NOT initiate a connection-parameter update here.
    // It cannot lower us below 7.5 ms anyway (NimBLE floors ble_gap_update_params
    // at the spec minimum), the console keeps 15 ms regardless, and the real 5 ms
    // arrives in the console's CONNECT_IND on reconnect (needs the controller
    // patch), not via an update. On the ESP32-S3 BTDM controller, kicking off an

components/switch2_pro/src/switch2_pro.cpp:295

  • The PR's own successful log shows this callback firing after encryption with the manually injected LTK, without BLE SMP. Thus its occurrence does not imply that the console ran SMP; update the comment so this diagnostic is interpreted correctly.
  callbacks.authentication_complete_callback = [this](const NimBLEConnInfo &info) {
    // If this fires, the console ran BLE SMP (which the research says it should
    // NOT do). Encrypted={}, bonded={} tells us what security state it reached.

components/switch2_pro/src/switch2_pro.cpp:467

  • The underlying API reports whether advertising actually started, but this result is discarded. Consequently wake_console() always returns true and leaves wake_pending_ latched even when no wake advertisement is on air, contradicting its public contract. Return and propagate this status (and clear the latch on failure).
  ble_gatt_server_.start_advertising(params);
  • Files reviewed: 34/35 changed files
  • Comments generated: 8
  • Review effort level: Balanced

Comment thread components/switch2_pro/tools/patch_nimble_5ms.py Outdated
Comment thread components/switch2_pro/CMakeLists.txt Outdated
Comment thread components/switch2_pro/src/switch2_pro.cpp Outdated
Comment thread components/switch2_pro/Kconfig Outdated
Comment thread components/switch2_pro/README.md Outdated
Comment thread components/switch2_pro/example/README.md Outdated
Comment thread components/switch2_pro/include/switch2_pro.hpp Outdated
Comment thread doc/en/ble/switch2_pro.rst Outdated
…S3/C3 patch

Follow-up review round.

- Disconnect telemetry: snapshot stream_start_us_ once (the stream thread can
  reset it to 0 between the condition and the calculation, e.g. when unsubscribe
  precedes disconnect, yielding an uptime-sized "streamed" value).
- Correct the "5 ms is only for reconnect/wake" framing everywhere: the console
  drops even a FRESH session to 5 ms ~1.5 s after subscription, so sustained
  input needs sub-spec support too; only the initial pairing handshake runs at
  15 ms. Updated CMakeLists warning, Kconfig help, README status/rate wording,
  example README, the continuous_stream_divisor doc (rate depends on the live
  interval — ~62/200 Hz), and the .rst 5 ms section (split by chip family).
- Remove the unverified S3/C3 binary-patch fallback (reviewer + esp-idf#18467 +
  our own testing: patching r_llc_con_upd_param_in_range alone doesn't enable
  5 ms on the pre-fix S3). patch_nimble_5ms.py / smoke_test_5ms.py now support
  only the open NimBLE chips (C6/C61/C2/H2); CMake errors if the patch is enabled
  on S3/C3, pointing to the official CONFIG_BT_CTRL_BLE_MIN_CONN_INTERVAL_ENABLE.
  Docs updated accordingly; RE notes remain in git history.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@finger563
finger563 requested a balanced review from Copilot September 4, 2026 18:16
- Static-random address fallback: check NimBLE setOwnAddr/setOwnAddrType return
  values and fail init() if the stable address can't be installed (proceeding
  with a mismatched/unstable address silently breaks pairing and reconnect/wake).
- wake_console(): start_advertising() now returns whether advertising actually
  started; wake_console() propagates that and clears the wake_pending_ latch on
  failure instead of always returning true.
- interval_tick_ is now reset at the start of each streaming run, so with a
  continuous_stream_divisor > 1 a new subscription always sends on its first tick
  (was carrying phase across runs, delaying the first report up to N-1 intervals).
- FreeRTOS tick rate: warn at init (and document in the README) when
  CONFIG_FREERTOS_HZ < 1000, since sub-15 ms / 5 ms stream pacing is tick-
  quantised and coarsens on a consumer build that didn't set 1000 Hz.
- Comments: the connect-callback note no longer claims the console stays at 15 ms
  / only gets 5 ms via CONNECT_IND — it also sends a fresh-session
  LL_CONNECTION_UPDATE to 5 ms; the auth-complete comment no longer implies the
  callback firing means the console ran SMP (it fires on our injected-LTK
  encryption in the normal non-SMP flow).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@finger563

Copy link
Copy Markdown
Contributor Author

Addressed the 6 suppressed comments from the latest Copilot review in 7405d33 (these were flagged in the review body rather than as resolvable threads):

  • switch2_pro.cpp:136 (address fallback) — the static-random fallback now checks setOwnAddr/setOwnAddrType return values and fails init() if the stable address cannot be installed, instead of logging success with a possibly-mismatched address (which would silently break reconnect/wake).
  • switch2_pro.cpp:467 (wake_console contract)start_advertising() now returns whether advertising actually started; wake_console() propagates that and clears the wake_pending_ latch on failure instead of unconditionally returning true.
  • switch2_pro.cpp:807 (interval_tick_)interval_tick_ is now reset at the start of each streaming run, so with continuous_stream_divisor > 1 a new subscription always sends on its first tick (it was carrying phase across runs, delaying the first report up to N-1 intervals).
  • switch2_pro.cpp:820 (FreeRTOS tick rate) — the component now warns at init when CONFIG_FREERTOS_HZ < 1000 and the README documents it as a consumer requirement, since the sub-15 ms / 5 ms stream pacing is tick-quantised and coarsens otherwise.
  • switch2_pro.cpp:254 (connect-callback comment) — corrected: it no longer claims the console stays at 15 ms and only reaches 5 ms via CONNECT_IND on reconnect; it now describes the fresh-session LL_CONNECTION_UPDATE to 5 ms as well.
  • switch2_pro.cpp:295 (auth-complete comment) — corrected: the callback firing does not imply the console ran BLE SMP; in the normal non-SMP flow it fires on the injected-LTK encryption completing (encrypted=true).

S3 build is clean. All inline review threads remain resolved.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 34 out of 35 changed files in this pull request and generated 7 comments.

Suppressed comments (1)

components/switch2_pro/tools/smoke_test_5ms.py:1

  • The objdump invocation ignores the return code. If objdump fails (wrong binary for arch, corrupted object, permission issues), stdout may be empty and the script could report an incorrect/indeterminate verdict without surfacing the actual failure cause. Consider using check=True and/or explicitly checking returncode and raising a clear error so failures reliably map to exit code 2.

Comment thread components/switch2_pro/idf_component.yml
Comment thread components/switch2_pro/tools/patch_nimble_5ms.py Outdated
Comment thread components/ble_gatt_server/include/ble_gatt_server.hpp Outdated
Comment thread components/ble_gatt_server/include/ble_gatt_server.hpp Outdated
Comment thread components/switch2_pro/Kconfig Outdated
Comment thread components/switch2_pro/idf_component.yml Outdated
Comment thread components/switch2_pro/src/switch2_pro_pairing.cpp
- patch_nimble_5ms.py: run `ar r` from the temp dir with the bare object
  basename so the archive member name stays exactly `ble_ll_conn.c.o` (passing
  a full path could store it as the member name on some ar variants, breaking
  later extraction/builds).
- smoke_test_5ms.py: check objdump's return code and raise (→ exit 2) instead
  of treating empty output as an indeterminate verdict.
- Kconfig: the 5 ms patch option is C6/C61/C2/H2 only; the help no longer claims
  an S3/C3 fallback (CMake already errors there), pointing S3/C3 at the official
  CONFIG_BT_CTRL_BLE_MIN_CONN_INTERVAL_ENABLE.
- ble_gatt_server: conn_params_update_callback now takes `const NimBLEConnInfo&`
  (consistent with the other callbacks); set_builtin_info_services_enabled()
  ignores post-init() calls with a warning instead of silently leaving services
  created-but-never-started.
- switch2_pro_pairing: initialise PSA once via std::call_once instead of on every
  confirm() call.
- idf_component.yml: use https:// (not git://) for the repository URL.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The manifest permits an incompatible dependency, and initialization can succeed despite advertising failure.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

components/switch2_pro/idf_component.yml:35

  • This dependency range can resolve to 2.3.0, but switch2_pro.cpp unconditionally calls NimBLEServer::registerServicesFirst(), which is only in the still-open upstream PR #443 and is not in that release. A registry build using the manifest will fail to compile. Before publishing this component, constrain the dependency to the first released version containing that API.
  h2zero/esp-nimble-cpp:
    version: '>=2.3.0'

components/switch2_pro/include/switch2_pro.hpp:64

  • The documentation tells users to feed real IMU data “via the report,” but Pro2InputReport exposes no motion setter or mutable byte access (data() is const), so this is not currently possible. Either add a bounded API for the 40-byte motion block or remove this guidance.
    /// Replay captured IMU motion frames in the input reports' motion block. Once
    /// the console enables the IMU feature (it does during standard init), every
    /// report carries a 40-byte motion block. **Off (default): the block is sent
    /// all-zero**, which the console accepts (verified on hardware, and what the
    /// zhantss emulator ships). On: replay a captured 128-frame resting sequence —
    /// but it loops (~2 s at 62 Hz) so its embedded timestamps jump backwards at
    /// the wrap; prefer feeding real IMU data via the report instead.

components/switch2_pro/include/switch2_pro.hpp:72

  • This public configuration documentation still presents the ESP32-S3 transmit stall as current, although the component README and PR status say it is resolved in ESP-IDF v6.1. That can incorrectly steer supported S3 users toward on-change mode; describe it as an optional reduced-traffic mode instead.
    /// a real controller — verified stable and lag-free on the C6-class chips
    /// (open NimBLE controller) at the console's 15 ms / 62 Hz. **Off =
    /// on-change:** notify only when the app's button/stick state changes, plus a
    /// low-rate keepalive — a reduced-traffic fallback that partially masks the
    /// ESP32-S3 BTDM controller's tx-servicing bug (see README "Known issues").

doc/Doxyfile:451

  • PairingCrypto is a documented public type in switch2_pro_pairing.hpp and is re-exported by switch2_pro.hpp, but this header is omitted from Doxygen input and the Switch2Pro API page. As a result, its generated API reference is missing. Add the header here and include inc/switch2_pro_pairing.inc from doc/en/ble/switch2_pro.rst.
  • Files reviewed: 34/35 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread components/switch2_pro/src/switch2_pro.cpp Outdated
- init() now fails if advertising doesn't start: advertise() returns bool
  (propagating start_advertising()'s result through all three variants) and
  init() returns false instead of logging success while undiscoverable.
- Config docs: continuous_streaming no longer presents the S3 tx-stall as
  current (fixed in v6.1) — on-change is described as an optional reduced-traffic
  mode; stream_imu_motion no longer claims a per-report motion input path that
  doesn't exist (the driver owns the motion block; a live-IMU setter is future
  work).
- Docs: add switch2_pro_pairing.hpp to the Doxygen input and include
  switch2_pro_pairing.inc in the API reference so PairingCrypto is documented.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@finger563

Copy link
Copy Markdown
Contributor Author

Addressed the suppressed comments from the latest Copilot review in 69b8c21:

  • switch2_pro.hpp:64 (IMU motion doc) — the stream_imu_motion doc no longer promises a per-report motion-input path that does not exist; it now states the driver owns the motion block and a live-IMU setter is future work.
  • switch2_pro.hpp:72 (continuous_streaming doc) — no longer presents the ESP32-S3 tx-stall as current (it is fixed in ESP-IDF v6.1); on-change is now described as an optional reduced-traffic mode, not an S3 necessity.
  • Doxyfile:451 / switch2_pro.rst (PairingCrypto docs) — added switch2_pro_pairing.hpp to the Doxygen input and inc/switch2_pro_pairing.inc to the API reference, so PairingCrypto is documented.
  • idf_component.yml:35 (esp-nimble-cpp range) — same item tracked by the merge gate: the "do not merge until feat(NimBLEServer): add registerServicesFirst() to place app services at low handles h2zero/esp-nimble-cpp#443 is released" banner blocks publishing this component, and the manifest floor is bumped to the first release containing registerServicesFirst() at that point (espp builds via the in-tree submodule meanwhile). No change this round.

S3 build is clean; all inline review threads are resolved.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unresolved dependency, notification accounting, and initialization lifecycle issues require correction and human validation.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

components/switch2_pro/src/switch2_pro.cpp:75

  • NimBLE emits BLE_GAP_EVENT_NOTIFY_TX for every notification attempt, including immediate failures, and passes that failure in code. Ignoring code makes ENOMEM attempts increment tx_completions_ and refresh last_tx_complete_us_, so the wedge telemetry reports failed sends as successful completions and can never show a stale completion while retries continue. Pass the status through and update completion state only when code == 0.

components/switch2_pro/idf_component.yml:35

  • This constraint currently allows the released 2.3–2.5 packages, but none contains NimBLEServer::registerServicesFirst(); upstream PR #443 is still open. A registry/Component Manager build will therefore select a valid dependency version and then fail at switch2_pro.cpp:363. Before publishing this component, either pin the fork commit that provides the API or wait for its release and raise this version floor to that release.
  h2zero/esp-nimble-cpp:
    version: '>=2.3.0'

components/switch2_pro/include/switch2_pro_report.hpp:22

  • These offsets are described as byte0..byte2, but data() includes the counter and power bytes and every setter below writes the button fields at offsets 2–4. Calling out the actual wire/report offsets avoids consumers interpreting or modifying the wrong bytes.
/// Button bits (matching the reverse-engineered layout):
///   byte0: 0x80 RStick 0x40 Plus 0x20 ZR 0x10 R 0x08 X 0x04 Y 0x02 A 0x01 B
///   byte1: 0x80 LStick 0x40 Minus 0x20 ZL 0x10 L 0x08 Up 0x04 Left 0x02 Right 0x01 Down
///   byte2: 0x10 C 0x08 GL 0x04 GR 0x02 Capture 0x01 Home
  • Files reviewed: 34/35 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread components/switch2_pro/src/switch2_pro.cpp
- init(): start the periodic wake re-advertiser only AFTER the initial
  advertisement succeeds. Previously start_wake_timer() ran before advertise();
  if advertise() then failed and init() returned false, the timer was left
  running and could later call advertise() on an object the caller believes
  failed to initialize. (boot_wake_pending_ is still latched before the first
  advertisement so it uses the wake variant.)
- on_notify_tx(): take the NimBLE NOTIFY_TX status. NimBLE fires this event for
  every notification attempt including immediate failures (e.g. ENOMEM) with the
  outcome in `code`; only advance the completion telemetry (tx_completions_,
  last_tx_complete_us_) when status == 0, so failed sends no longer look like
  successful over-air completions in the wedge diagnostics. The flow-control slot
  is freed either way.
- Clarify the input-report button-bits comment to reference the actual report
  offsets (2/3/4), matching set_bit() — data()[0]/[1] are counter/power.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@finger563

Copy link
Copy Markdown
Contributor Author

Addressed the suppressed comments from the latest Copilot review in bb9b74d:

  • switch2_pro.cpp:75 (NOTIFY_TX status)on_notify_tx() now takes the NimBLE code. Since NimBLE fires BLE_GAP_EVENT_NOTIFY_TX for every attempt including immediate failures (e.g. ENOMEM), the completion telemetry (tx_completions_, last_tx_complete_us_) now only advances when status == 0; the flow-control slot is freed either way. Failed sends no longer look like successful over-air completions in the wedge diagnostics.
  • switch2_pro_report.hpp:22 (button-bits comment) — reworded to reference the actual report offsets (2/3/4), matching set_bit(), and noting data()[0]/[1] are the counter/power bytes.
  • idf_component.yml:35 (esp-nimble-cpp range) — same item tracked by the merge gate (the "do not merge until feat(NimBLEServer): add registerServicesFirst() to place app services at low handles h2zero/esp-nimble-cpp#443 releases" banner blocks publishing; the floor bumps to the first release with registerServicesFirst() at that point; espp builds via the in-tree submodule meanwhile). No change this round.

S3 build is clean; all inline review threads are resolved.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Two moderate dependency and wake/reconnect issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

components/switch2_pro/idf_component.yml:35

  • The declared minimum still allows released esp-nimble-cpp 2.3.0, but that release does not provide NimBLEServer::registerServicesFirst(), so Component Manager consumers will fail to compile at the call in switch2_pro.cpp. Upstream PR #443 is still open; before publishing this component, require the first released version containing that API and update the submodule pin accordingly.
    version: '>=2.3.0'

components/switch2_pro/include/switch2_pro_motion.hpp:13

  • This describes replay as unconditional, but Config::stream_imu_motion defaults to false and send_input_report() leaves the motion block zeroed in that mode. Clarify that this sequence is replayed only when the option is enabled so the public header matches runtime behavior.
/// when replayed in sequence. The console enables IMU during init and every real
/// report carries this 40-byte block; we replay this sequence (looping) so our
/// stream matches the device rather than sending an empty or non-monotonic block.

components/switch2_pro/src/switch2_pro.cpp:379

  • These dynamically allocated callbacks are not owned by NimBLE: NimBLECharacteristic::setCallbacks() only stores the raw pointer, and the characteristic destructor deletes descriptors but not its callback. Every controller teardown therefore leaks one ChannelCallbacks per attached characteristic. Keep the callback objects in an owning container on Switch2Pro and pass their raw pointers to NimBLE.
  auto attach = [this](NimBLECharacteristic *c, const char *name, int role) {
    c->setCallbacks(new ChannelCallbacks(this, name, role));
  };
  • Files reviewed: 34/35 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread components/switch2_pro/src/switch2_pro.cpp Outdated
finger563 and others added 2 commits September 4, 2026 22:31
- Clear the wake latches (wake_pending_ / boot_wake_pending_) only after an
  encrypted session is confirmed (authentication_complete with encrypted=true),
  not on the raw GAP connect event. A console we just woke can connect and then
  drop before encryption/init completes; clearing on connect made the ensuing
  disconnect fall back to the passive reconnect advertisement instead of keeping
  the wake variant on the air. The wake timer only advertises while disconnected
  and self-cancels once boot_wake_pending_ clears, so this is safe.
- Own the per-characteristic ChannelCallbacks objects in a member vector instead
  of leaking a `new` on every setCallbacks(). NimBLECharacteristic::setCallbacks()
  only stores the raw pointer and never deletes it, so each teardown previously
  leaked one callback per characteristic. Deleted via the virtual base after
  ble_gatt_server_.deinit() tears the characteristics down.
- switch2_pro_motion.hpp: note the sequence is replayed only when
  Config::stream_imu_motion is enabled (default false sends a zeroed block).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@finger563

Copy link
Copy Markdown
Contributor Author

Addressed the suppressed comments from the latest Copilot review in 88b264a:

  • switch2_pro.cpp:379 (ChannelCallbacks leak)NimBLECharacteristic::setCallbacks() only stores the raw pointer and never frees it, so each teardown leaked one ChannelCallbacks per characteristic. They are now owned by a std::vector<std::unique_ptr<NimBLECharacteristicCallbacks>> member (deleted via the virtual base after ble_gatt_server_.deinit() tears the characteristics down).
  • switch2_pro_motion.hpp:13 (replay doc) — clarified that the captured sequence is replayed only when Config::stream_imu_motion is enabled; the default (false) sends a zeroed motion block.
  • idf_component.yml:35 (esp-nimble-cpp range) — the standing merge-gated item (blocked by the "do not merge until h2zero#443 releases" banner; floor bumps to the first release with registerServicesFirst() then). No change.

(Also rebased onto your 8c2ca5e autofix and consolidated the now-duplicate start_wake_timer() calls into one.) S3 build is clean; all inline threads resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants