Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions crates/stackchan-firmware/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,12 @@ esp-alloc = { version = "0.9.0", features = ["defmt"] }
# esp-hal ~1.1.0-rc.0 which would force a wider toolchain bump.
# `unstable` is required because esp-hal itself runs with `unstable`
# enabled here. `ble` enables the on-chip BLE controller (HCI behind
# the `BleConnector` Transport); `coex` lets Wi-Fi STA and BLE share
# the radio at the cost of ~10–15% Wi-Fi throughput. The bt-hci
# version pin (^0.6) cascades into the trouble-host 0.5.x line.
esp-radio = { version = "~0.17", features = ["defmt", "esp32s3", "wifi", "ble", "coex", "esp-now", "unstable"] }
# the `BleConnector` Transport). No `coex`: BLE and Wi-Fi are brought up
# mutually exclusively in firmware (BLE only when no Wi-Fi SSID is set),
# so the radio is never shared — coexistence would only cost the internal
# RAM the Wi-Fi MAC needs to start. The bt-hci version pin (^0.6) cascades
Comment on lines +112 to +113
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 The comment says coex would "cost the internal RAM the Wi-Fi MAC needs to start," but the PR description explicitly notes that heap at Wi-Fi start is unchanged (~37 KiB) and the benefit is "binary size + removed complexity, not RAM headroom." The current phrasing would mislead future readers into thinking dropping coex freed Wi-Fi MAC RAM. The same inaccuracy appears in the parallel sentence in ble/mod.rs (line 42–43).

Suggested change
# so the radio is never shared — coexistence would only cost the internal
# RAM the Wi-Fi MAC needs to start. The bt-hci version pin (^0.6) cascades
# so the radio is never shared — `coex` is dead weight here (binary size
# plus the linked `coexist` lib) with no runtime scheduling benefit. The bt-hci version pin (^0.6) cascades
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/stackchan-firmware/Cargo.toml
Line: 112-113

Comment:
The comment says coex would "cost the internal RAM the Wi-Fi MAC needs to start," but the PR description explicitly notes that heap at Wi-Fi start is unchanged (~37 KiB) and the benefit is "binary size + removed complexity, not RAM headroom." The current phrasing would mislead future readers into thinking dropping `coex` freed Wi-Fi MAC RAM. The same inaccuracy appears in the parallel sentence in `ble/mod.rs` (line 42–43).

```suggestion
# so the radio is never shared — `coex` is dead weight here (binary size
# plus the linked `coexist` lib) with no runtime scheduling benefit. The bt-hci version pin (^0.6) cascades
```

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code

# into the trouble-host 0.5.x line.
esp-radio = { version = "~0.17", features = ["defmt", "esp32s3", "wifi", "ble", "esp-now", "unstable"] }
# BLE host stack. 0.5.1 is the trouble-host release pinned to
# `bt-hci ^0.6` (matches esp-radio 0.17). 0.6.0 jumps to `bt-hci 0.8`
# and pairs with esp-radio 0.18 — bump together when we move the
Expand Down
8 changes: 4 additions & 4 deletions crates/stackchan-firmware/src/ble/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@
//! LCD. Bonds are persisted to the SD card via [`bonds`] so a
//! re-paired peer skips the passkey dance.
//!
//! Coexistence: BLE rides the same radio as Wi-Fi via esp-radio's
//! `coex` feature. Expect a single-digit-percent Wi-Fi throughput
//! cost — invisible for the dashboard / SSE / settings round-trips
//! the firmware does today.
//! Mutually exclusive with Wi-Fi: BLE is brought up only when no Wi-Fi
//! SSID is configured (see `main.rs`). The two never share the radio, so
//! esp-radio's `coex` feature is left off — enabling it would only reserve
//! internal RAM the Wi-Fi MAC needs to start.
Comment on lines +42 to +43
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Same comment inaccuracy as in Cargo.toml: coex's measured runtime RAM cost was negligible (heap unchanged at Wi-Fi start per the PR description); the real benefit is binary size and removed complexity.

Suggested change
//! esp-radio's `coex` feature is left off — enabling it would only reserve
//! internal RAM the Wi-Fi MAC needs to start.
//! esp-radio's `coex` feature is left off — enabling it adds binary size
//! (links the `coexist` lib) with no runtime scheduling benefit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/stackchan-firmware/src/ble/mod.rs
Line: 42-43

Comment:
Same comment inaccuracy as in `Cargo.toml`: coex's measured runtime RAM cost was negligible (heap unchanged at Wi-Fi start per the PR description); the real benefit is binary size and removed complexity.

```suggestion
//! esp-radio's `coex` feature is left off — enabling it adds binary size
//! (links the `coexist` lib) with no runtime scheduling benefit.
```

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code

//!
//! Sync-version note: trouble-host 0.5.x pulls
//! `embassy-sync = 0.6` while the rest of the firmware uses 0.7.
Expand Down
Loading