Add Extended Wi-Fi Timeout toggle for TRMNL sleep-screen fetch#3
Open
mcmphd wants to merge 4 commits into
Open
Add Extended Wi-Fi Timeout toggle for TRMNL sleep-screen fetch#3mcmphd wants to merge 4 commits into
mcmphd wants to merge 4 commits into
Conversation
TrmnlSleepClient::connectWifi() allows only 20 retries x 500ms = 10s for the radio to associate and get a DHCP lease after waking from deep sleep. On some networks -- WPA2/WPA3 mixed-mode APs in particular, where the WPA handshake can intermittently time out -- 10s isn't always enough, and the TRMNL fetch silently falls back to the default sleep screen. Rather than raising the timeout unconditionally for everyone, add a per-device toggle: Settings > System > TRMNL Settings > Extended Wi-Fi Timeout. Off by default (unchanged 10s/20-retry behavior); when on, widens the window to 20s/40 retries. - CrossPointSettings: new trmnlExtendedWifiTimeout field, registered in getSettingsList() so it persists through the existing JSON settings loop (same pattern as trmnlOrientation). - TrmnlSettingsActivity: new list item, toggled and saved the same way as the other TRMNL settings. - TrmnlSleepClient: Config gains extendedWifiTimeout; connectWifi() picks between WIFI_RETRIES_DEFAULT (20) and WIFI_RETRIES_EXTENDED (40) based on it. connectWifi() returns as soon as the link is up, so this only costs extra time on wakes that would otherwise fail. Verified: builds clean in the `simulator` env (macOS/arm64), including the auto-generated StrId string via gen_i18n.py. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewer's GuideAdds an opt-in 'Extended Wi-Fi Timeout' TRMNL setting and plumbs it through settings, config, and the TRMNL sleep client so TRMNL fetches can use a longer Wi-Fi connection window on problematic networks, while preserving existing behavior by default. Sequence diagram for TRMNL fetch using Extended Wi-Fi TimeoutsequenceDiagram
participant SA as SleepActivity
participant SC as TrmnlSleepClient
participant WiFi as WiFi
SA->>SC: fetchLatest(config)
SC->>SC: connectWifi(config.extendedWifiTimeout)
alt extendedWifiTimeout is true
SC->>WiFi: begin(ssid, password)
SC->>WiFi: retry up to WIFI_RETRIES_EXTENDED
else extendedWifiTimeout is false
SC->>WiFi: begin(ssid, password)
SC->>WiFi: retry up to WIFI_RETRIES_DEFAULT
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The TRMNL settings UI now relies on the magic index
4in multiple places (MENU_ITEMS,handleSelection, andrender); consider introducing a small enum or named constants for the menu indices so future additions/reordering don’t silently break the toggle wiring.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The TRMNL settings UI now relies on the magic index `4` in multiple places (`MENU_ITEMS`, `handleSelection`, and `render`); consider introducing a small enum or named constants for the menu indices so future additions/reordering don’t silently break the toggle wiring.
## Individual Comments
### Comment 1
<location path="src/activities/settings/TrmnlSettingsActivity.cpp" line_range="14-20" />
<code_context>
namespace {
-constexpr int MENU_ITEMS = 4;
+constexpr int MENU_ITEMS = 5;
const StrId menuNames[MENU_ITEMS] = {StrId::STR_TRMNL_SERVER_URL, StrId::STR_TRMNL_API_KEY,
- StrId::STR_TRMNL_DEVICE_ID, StrId::STR_TRMNL_ORIENTATION};
</code_context>
<issue_to_address>
**suggestion:** Avoid hardcoding menu indices now that MENU_ITEMS has increased.
The new item uses the literal `4` in both `handleSelection` and `render`, tying behavior to the current ordering and count. Please introduce a named constant or enum (e.g. `constexpr int MENU_INDEX_EXTENDED_WIFI_TIMEOUT = 4;`) and reuse it in both places, or derive the behavior from `menuNames` instead of raw indices.
Suggested implementation:
```cpp
namespace {
constexpr int MENU_ITEMS = 5;
constexpr int MENU_INDEX_EXTENDED_WIFI_TIMEOUT = 4;
const StrId menuNames[MENU_ITEMS] = {StrId::STR_TRMNL_SERVER_URL, StrId::STR_TRMNL_API_KEY,
StrId::STR_TRMNL_DEVICE_ID, StrId::STR_TRMNL_ORIENTATION,
StrId::STR_TRMNL_EXTENDED_WIFI_TIMEOUT};
```
```cpp
} else if (selectedIndex == MENU_INDEX_EXTENDED_WIFI_TIMEOUT) {
SETTINGS.trmnlExtendedWifiTimeout = !SETTINGS.trmnlExtendedWifiTimeout;
```
There is a corresponding use of the hardcoded index in the `render` logic for this menu item (e.g., something like `if (i == 4)` or similar). That code should be updated to compare against `MENU_INDEX_EXTENDED_WIFI_TIMEOUT` instead of the literal `4`, for example:
- Replace any `i == 4` (or similar loop-index comparison) that is specifically for the extended Wi-Fi timeout item with `i == MENU_INDEX_EXTENDED_WIFI_TIMEOUT`.
- Likewise, any other conditions or accessors for `menuNames[4]` that are conceptually about the extended Wi-Fi timeout should use `MENU_INDEX_EXTENDED_WIFI_TIMEOUT` instead of a raw index.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
TrmnlSettingsActivity used the literal 4 in both handleSelection() and render() for the new Extended Wi-Fi Timeout item, tying its wiring to the current position/count with no compiler-enforced link between the two call sites. Introduce MENU_INDEX_EXTENDED_WIFI_TIMEOUT and use it in both places instead, so reordering or inserting a new item before it can't silently desync selection handling from rendering. Left the pre-existing 0/1/2/3 literals for Server URL/API Key/Device ID/Orientation as-is -- out of scope for this change, and not something this feature touched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
TrmnlSleepClient::connectWifi()allows only 20 retries × 500ms = 10sfor the radio to associate and get a DHCP lease after waking from deep
sleep. On some networks — WPA2/WPA3 mixed-mode APs in particular, where
the WPA handshake can intermittently time out, or marginal signal — 10s
isn't always enough. When it isn't, the TRMNL fetch fails silently for
that cycle and the device falls back to a stale cached image or the
default sleep screen, indistinguishable on-device from any other failure.
Evidence (on-device serial, debug build)
[TRM] Connecting to WiFi:
[TRM] Wi-Fi connect failed after 20 attempts (~10s)
[WIFI] STA event: disconnected reason=204(HANDSHAKE_TIMEOUT)
A warm manual connect to the same AP typically succeeds in under 2s — the
failure is specific to the 10s cold-wake budget, not the network itself.
Fix
Rather than raising the timeout unconditionally for everyone, add a
per-device, opt-in toggle: Settings → System → TRMNL Settings →
Extended Wi-Fi Timeout. Off by default (unchanged 10s/20-retry
behavior); when enabled, widens the window to 20s/40 retries.
CrossPointSettings: newtrmnlExtendedWifiTimeoutfield, registeredin
getSettingsList()so it persists through the existing JSONsettings loop (same pattern as
trmnlOrientation).TrmnlSettingsActivity: new list item, toggled and saved the same wayas the other TRMNL settings.
TrmnlSleepClient:ConfiggainsextendedWifiTimeout;connectWifi()picks betweenWIFI_RETRIES_DEFAULT(20) andWIFI_RETRIES_EXTENDED(40) based on it.connectWifi()returns assoon as the link is up, so this only costs extra time on wakes that
would otherwise fail — no cost on the happy path.
Testing
Builds clean in the
simulatorenv (macOS/arm64). Verified on an XTeinkX4: with the toggle off, wakes on a WPA2/WPA3 mixed-mode AP intermittently
hit the 10s
HANDSHAKE_TIMEOUTfailure above; with the toggle on, thewider window resolves the connection reliably on the same network.
Summary by Sourcery
Add an optional extended Wi‑Fi timeout for TRMNL sleep-screen fetches, wired through device settings into the TRMNL client configuration and Wi‑Fi connection logic.
New Features:
Enhancements: