feat(net): adaptive throttle toggle in Connect panel (all modes)#3175
Conversation
95d3f74 to
01fc69c
Compare
|
@AetherClaude can I get your thoughts on this? Also refer to the previous PR mentioned earlier. |
|
Read the diff against 1. Default should probably be
|
There was a problem hiding this comment.
Thanks @chibondking — clean, focused PR. Conventions match the rest of ConnectionPanel (AppSettings "True"/"False" strings, same lambda-save pattern as AutoConnectToLastRadio and LowBandwidthConnect), and routing the gate through startNetworkMonitor() is the right hook since all three connect paths funnel there.
A couple of observations, none blocking:
Connect-time semantics aren't surfaced to the user. m_adaptiveThrottleEnabled is cached in startNetworkMonitor() (RadioModel.cpp:2553), so toggling the checkbox mid-session has no effect until reconnect. Worse, if the throttle is currently engaged and the user unchecks the box hoping to lift it, nothing happens — applyAdaptiveFrameRate() early-returns on subsequent state changes but doesn't undo the in-flight cap, and the user has no signal that their toggle was ignored. This mirrors how LowBandwidthConnect works, so it's defensible, but the tooltip currently reads like a live switch. Consider either:
- Tightening the tooltip to make the connect-time semantics explicit (e.g. "Applied at connect time"), or
- Reading
AppSettingsdirectly insideapplyAdaptiveFrameRate()so the toggle is live (and lifting the cap when it flips off mid-throttle).
The cached-flag approach is simpler and probably fine for v1 — just worth a tooltip nudge.
Unrelated rename. The visible → slowLinksVisible rename in updateLowBandwidthVisibility() is out of scope for this PR. Harmless but adds review noise; better split or dropped.
Otherwise LGTM — no AppSettings/RAII issues, no null-pointer or leak concerns, and the early-return path means zero behavioral change when the box is unchecked (the documented default).
🤖 aethersdr-agent · cost: $5.5376 · model: claude-opus-4-7
…l (all modes) PR aethersdr#2829 introduced adaptive frame-rate throttling — when the network quality monitor downgrades from Good → Fair → Poor it progressively reduces FFT/waterfall fps (15 → 8 → 4) to shed UDP load and avoid spurious disconnects. The feature shipped always-on with no way to turn it off at connect time. Issue aethersdr#3173 is a direct report of this: a user saw the panadapter rate drop unexpectedly a few seconds after connecting. The toggle is available in all three connection modes: On This Network, Remote with SmartLink, and Connect by IP. Default is off (opt-in). Changes ─────── src/gui/ConnectionPanel.h • Add QCheckBox* m_adaptiveThrottleCheck{nullptr} member. src/gui/ConnectionPanel.cpp • Construct "Enable adaptive frame-rate throttle" as a standalone checkbox in the root layout, between the SmartLink/ManualMode slower-links callout and "Connect to last radio on start up". Always visible regardless of selected connection mode. • Reads AdaptiveThrottleEnabled setting on construction (default "False") and saves on toggle, matching the pattern of m_autoConnectCheck and m_lowBwCheck. • Tooltip explains that toggling while connected takes effect at the next quality state change, and that reconnect is required to immediately lift an already-applied cap. src/models/RadioModel.h • Remove m_adaptiveThrottleEnabled cached member. Live-reading AppSettings at the call site is cheaper than caching a bool that silently ignores mid-session toggle changes. src/models/RadioModel.cpp (applyAdaptiveFrameRate) • Read AdaptiveThrottleEnabled directly from AppSettings inside applyAdaptiveFrameRate() instead of caching it at connect time. This means the toggle takes effect at the next quality state transition without requiring a reconnect — no footgun where the user unchecks the box and nothing visibly changes. • The startNetworkMonitor() read and cached-flag assignment are removed; no other callers depended on m_adaptiveThrottleEnabled. Setting key ─────────── AdaptiveThrottleEnabled (AppSettings, persisted) Values: "False" (default) | "True" Scope: checked on every network quality state transition; changes take effect at the next transition. To immediately lift an in-flight cap, disconnect and reconnect. All three connection paths call onConnected() → startNetworkMonitor() → evaluateNetworkQuality() → applyAdaptiveFrameRate(), so no path bypasses the live check: Local → connectToRadio() → onConnected() WAN → connectViaWan() → onConnected() (direct call, TLS already up) Manual → connectToRadio() → onConnected() Testing notes ───────────── • Checkbox appears in all three connection modes, unchecked by default. • Check the box, connect; confirm adaptive fps commands appear in the protocol debug log (lcProtocol) as network quality degrades. • Leave unchecked, connect; confirm no fps throttle commands are sent. • Toggle mid-session; confirm the change takes effect at the next quality state transition without reconnecting. • Setting persists across app restart. Fixes aethersdr#3173 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
01fc69c to
b963c9d
Compare
|
Claude here — merged, and welcome to AetherSDR, CJ! 🎉 First PR from a brand-new Tier 3 reviewer, and a really nice one — clear bug, surgical fix, design-decisions table showing iteration, and the live-read-vs-cached choice is the more elegant pattern (avoids a whole class of member-state-sync bugs). Issue #3173 closes automatically. Looking forward to more. 73, Jeremy KK7GWY & Claude (AI dev partner) |
…nabled check (#3201) ## Summary Closes a gap in the adaptive frame-rate throttle toggle introduced in #3175. When a panadapter was **opened mid-session** while the network was already in a degraded state, `ensureOwnedPanadapter()` applied the active fps cap directly via `sendAdaptiveCapToPan()` without consulting `AdaptiveThrottleEnabled`. The toggle was bypassed for newly-registered pans, including the deferred `waterfallIdChanged` re-apply lambda. ## Approach: gate at the source Rather than adding inline checks at each call site (which multiplies the guard and creates a latent risk for future callers), the fix moves the enable check into `currentAdaptiveFpsCap()`: ```cpp int RadioModel::currentAdaptiveFpsCap() const { if (AppSettings::instance().value("AdaptiveThrottleEnabled", "False").toString() != "True") return 0; return fpsCapForState(m_netState); } ``` `ensureOwnedPanadapter()` already gates on `activeCap > 0` — it gets correct behaviour for free with no other changes. `applyAdaptiveFrameRate()` keeps its own early-return as a short-circuit (it calls `fpsCapForState` directly, not `currentAdaptiveFpsCap`). ## All `sendAdaptiveCapToPan` call sites now covered | Location | How gated | |---|---| | `applyAdaptiveFrameRate()` | explicit early-return (short-circuit; calls `fpsCapForState` directly) | | `ensureOwnedPanadapter()` immediate cap | `activeCap > 0` — `currentAdaptiveFpsCap()` returns 0 when off | | `ensureOwnedPanadapter()` `waterfallIdChanged` lambda | same — `cap > 0` check calls `currentAdaptiveFpsCap()` | ## Additional nits addressed - `Qt::UniqueConnection` added to the `waterfallIdChanged` lambda to prevent duplicate cap commands if `waterfallId` changes more than once on a pan - Comment added documenting the narrow ordering edge case: pan created healthy → network degrades → `waterfallId` arrives before next tier transition. `applyAdaptiveFrameRate()` catches it on the next transition; noted, not fixed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…hersdr#3175) ## Summary - Adds an **\"Enable adaptive frame-rate throttle\"** checkbox to all three connection modes, wired to a new `AdaptiveThrottleEnabled` AppSettings key (default `False` — opt-in) - **Live-reads** `AppSettings` inside `applyAdaptiveFrameRate()` on every quality state transition instead of caching at connect time — toggle is responsive mid-session, not just at next connect - Tooltip is explicit: toggling mid-session takes effect at the next quality update; reconnect lifts an already-applied cap immediately - Fixes aethersdr#3173 (user seeing automatic fps reduction they didn't expect — caused by aethersdr#2829 shipping always-on) ## Design decisions addressed from review | Concern | Resolution | |---|---| | Cached flag ignored mid-session toggles | Dropped `m_adaptiveThrottleEnabled` member; `applyAdaptiveFrameRate()` reads AppSettings live | | Tooltip implied a live switch | Updated to explain next-quality-update semantics and reconnect-to-lift | | Default `True` vs `False` | Keeping `False` (opt-in) — issue aethersdr#3173 confirms the always-on behaviour surprises users | ## All three connect paths covered All paths funnel through `onConnected()` → `startNetworkMonitor()` → `evaluateNetworkQuality()` → `applyAdaptiveFrameRate()`, so the live check is reached regardless of connection type. ## Test plan - [ ] Checkbox appears in all three connection modes, unchecked by default - [ ] Check the box, connect; confirm adaptive fps commands appear in `lcProtocol` as quality degrades - [ ] Leave unchecked, connect; confirm no fps throttle commands are sent - [ ] Toggle mid-session; confirm change takes effect at next quality state transition - [ ] Setting persists across app restart 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…nabled check (aethersdr#3201) ## Summary Closes a gap in the adaptive frame-rate throttle toggle introduced in aethersdr#3175. When a panadapter was **opened mid-session** while the network was already in a degraded state, `ensureOwnedPanadapter()` applied the active fps cap directly via `sendAdaptiveCapToPan()` without consulting `AdaptiveThrottleEnabled`. The toggle was bypassed for newly-registered pans, including the deferred `waterfallIdChanged` re-apply lambda. ## Approach: gate at the source Rather than adding inline checks at each call site (which multiplies the guard and creates a latent risk for future callers), the fix moves the enable check into `currentAdaptiveFpsCap()`: ```cpp int RadioModel::currentAdaptiveFpsCap() const { if (AppSettings::instance().value("AdaptiveThrottleEnabled", "False").toString() != "True") return 0; return fpsCapForState(m_netState); } ``` `ensureOwnedPanadapter()` already gates on `activeCap > 0` — it gets correct behaviour for free with no other changes. `applyAdaptiveFrameRate()` keeps its own early-return as a short-circuit (it calls `fpsCapForState` directly, not `currentAdaptiveFpsCap`). ## All `sendAdaptiveCapToPan` call sites now covered | Location | How gated | |---|---| | `applyAdaptiveFrameRate()` | explicit early-return (short-circuit; calls `fpsCapForState` directly) | | `ensureOwnedPanadapter()` immediate cap | `activeCap > 0` — `currentAdaptiveFpsCap()` returns 0 when off | | `ensureOwnedPanadapter()` `waterfallIdChanged` lambda | same — `cap > 0` check calls `currentAdaptiveFpsCap()` | ## Additional nits addressed - `Qt::UniqueConnection` added to the `waterfallIdChanged` lambda to prevent duplicate cap commands if `waterfallId` changes more than once on a pan - Comment added documenting the narrow ordering edge case: pan created healthy → network degrades → `waterfallId` arrives before next tier transition. `applyAdaptiveFrameRate()` catches it on the next transition; noted, not fixed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Summary
AdaptiveThrottleEnabledAppSettings key (defaultFalse— opt-in)AppSettingsinsideapplyAdaptiveFrameRate()on every quality state transition instead of caching at connect time — toggle is responsive mid-session, not just at next connectDesign decisions addressed from review
m_adaptiveThrottleEnabledmember;applyAdaptiveFrameRate()reads AppSettings liveTruevsFalseFalse(opt-in) — issue #3173 confirms the always-on behaviour surprises usersAll three connect paths covered
All paths funnel through
onConnected()→startNetworkMonitor()→evaluateNetworkQuality()→applyAdaptiveFrameRate(), so the live check is reached regardless of connection type.Test plan
lcProtocolas quality degrades🤖 Generated with Claude Code