feat: add us debt screen #2
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "PMK/btclock_v4:feat/usdebt_screen"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
WIP: feat: add us debt screento feat: add us debt screenPR Reviewer Guide 🔍
(Review updated until commit
d5333a0a79)Here are some key observations to aid the review process:
Floating-point precision
In the "precise mode" fallback path (when
us_debt_exactis nullopt butus_debt_tis set), the code computes*us_debt_t * 1e12and casts touint64_t. Sinceus_debt_t(e.g. 39.2) is not exactly representable in IEEE 754 double, the multiplication may produce a value like 39199999999999.998… which truncates to 39199999999999 instead of the expected 39200000000000. In normal operation this fallback never triggers becauseus_debt_tandus_debt_exactare always populated together inPollOnce(), but if they ever diverge (e.g., partial snapshot merge edge case), the displayed precise number could be wrong by one in the last digit.PR Code Suggestions ✨
No code suggestions found for the PR.
PR Code Suggestions ✨
No code suggestions found for the PR.
Persistent review updated to latest commit
d5333a0a79🤖 Code Review — US Debt screen (
feat/usdebt_screen)Manual review by Claude (read the full head content of the new
us_debtcomponent + the integration points, not just the diff). Overall this is a clean, well-structured addition — solid task lifecycle, good RAII, and a sensible compact/precise display split. A few things worth addressing before merge, ordered by severity.🟠 Medium
1. Dead branch in
FormatUsDebtString—main/screens/panel_texts.cpp#L946-L949The
value_p < 10.0branch and the finalelseproduce the exact same format string, so the< 10.0test is dead. Given the< 1.0branch uses"$%.2fP", the intent was almost certainly"$%.2fP"here too (extra precision for single-digit quadrillions). Either restore the intended%.2for drop the redundant branch.2. Triplicated "dollars + 3-digit grouping" logic. The derivation
plus the
uint64 → 3-char groupsformatting is duplicated across three places that must stay in lockstep:panel_texts.cpp BuildUsDebtmain/screens/us_debt.cpp RenderUsDebtScreen+DebtGroupsscreen_manager.cpp ShouldRender(L708) andRender(L1019)A format tweak or bug-fix now needs editing in three spots — high divergence risk. Suggest two small shared helpers, e.g.
UsDebtDollars(const DataSnapshot&) -> uint64_tandGroupDigits(uint64_t) -> std::vector<std::string>, reused by source/screen/manager.🟡 Low
3. NVS read on every
ShouldRendercall —screen_manager.cpp#L700-L707This opens an NVS handle and reads from flash each time
ShouldRenderruns, whereas the other screen cases just compare cached values. Theus_debt_big_charflag is already read once intoRenderPrefs(rp.us_debt_big_char). Reuse the cached value here to avoid per-tick flash I/O and to guaranteeShouldRenderandRenderagree within the same cycle.4. Float fallback precision —
us_debt.cpp#L96-L100(and the two duplicates)The
*us_debt_t * 1e12fallback is doubly lossy:us_debt_tis already rounded to 0.1T (±~50 billion), and thedouble → uint64cast can truncate (e.g.39.2 * 1e12 → 39199999999999). It's effectively unreachable today becausePollOnce()always setsus_debt_exactalongsideus_debt_t, so this is defensive-only — but if the two ever diverge (e.g. a partial snapshot merge), the precise display would be wrong. When usingus_debt_exactthe path is exact (integer part < 2^53), so the fix is just to make the fallback's lossiness explicit or drop it.5. Task stack size — please verify.
us_debt_source.cpp#L127-L129creates the worker with a 4 KB stack while it runsesp_http_client_perform+ cJSON parsing. If TLS terminates on this task (rather than in the proxy transport), 4 KB is tight and a handshake could overflow it. Worth a quickuxTaskGetStackHighWaterMark()check under a real fetch.✅ Looks good / verified
Stop()setsstop_, joins on thedone_semaphore (≤12 s) and only then clearshub_, so there's no use-after-free / data race onhub_fromPollOnce().FetchContextfrees its buffer via RAII. Nicely done.truncatedflag prevents unbounded growth;body[size] = '\0'stays within thekMaxResponseBytes + 1allocation.static_assert(kAgnosticSlots == 11, …)bumped in lockstep with the slot map — good guard against the documented slot regression.heap_caps_malloc_prefer(kMaxResponseBytes + 1, 2, MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT, MALLOC_CAP_8BIT)call is correct — the2is the count of capability arguments (num), not a caps bitmask. It properly prefers SPIRAM and falls back to internal 8-bit RAM. (Flagging explicitly because an automated pass read this as a "wrong caps" bug — it's a false positive.)🧪 Tests
test_host/test_us_debt_panel_texts.cppcovers the panel-text formatting — good. Note the duplicated render/manager grouping logic (point 2) isn't exercised by those tests; folding it into a shared helper would also make it directly testable.Please check my comments.
@djuri Can you verify the dataSource if this is done correctly?
@ -168,3 +170,3 @@| `mempoolInstance` | string | `"mempool.space"` | Base URL of the mempool.space-compatible instance. | Reboot required. || `mempoolSecure` | bool | `true` | Use HTTPS/WSS when talking to `mempoolInstance`. | Reboot required. || `ceEndpoint` | string | `"ws-staging.btclock.dev"` | Custom-endpoint host when `dataSource=1`. | Reboot required. || `ceEndpoint` | string | `"ws.btclock.dev"` | Custom-endpoint host when `dataSource=2/3`. | Reboot required. |@djuri Can you verify the dataSource if this is done correctly?
@ -19,6 +19,8 @@// slot 12 + 3k : kMarketCap api_id 30 for currencies[k]// slot last : kBlockFeeRate api_id 6//// app_id 100 - 128 are for the debt screens (kDebtSlotBase)@djuri As there are 28 new screens, I have added only this line.
@ -73,3 +72,4 @@// agnostic block (after NwcBalance) so the screen's introduction// didn't shift bitaxe / NWC slot indices. New screens added later// should follow the same pattern."base slot") {@djuri Please test the "didn't shift bitaxe / NWC slot indices" part with this fix.
Review — US Debt screen over the v2 WS
metricchannelVerdict: Request changes. This is a well-structured feature — the rate-frame-ticked-locally model is the right design, the slot/catalog integration is clean, and host-test coverage is solid. The wire protocol aligns 100% with the new Go
metricfeed. But there are two must-fixes before merge, and the debt socket's connection target is wired wrong — confirmed both by static analysis and empirically on hardware (Rev B).Empirical verification (Rev B @ staging,
dataSource=3→ws-staging.btclock.dev)I built this branch, flashed a Rev B, pointed it at staging, and read the live panel texts via
GET /api/status(data[]= the current screen's panels), jumping straight to the debt screen withPOST /api/show/screen {"s":100}:data[]on screen 100['US/DEBT','','','','','','']['US/DEBT','$','3','9.','2','6','T']In both runs the control price screen showed
['BTC/USD','$','6','2','9','3','7'](≈ $62,937) — so the staging core feed was alive the whole time; only debt was silent in A. That isolates the failure precisely to the debt-metric routing.🔴 Blockers
1. The debt-metric socket can never reach the server that serves the feed
main/sources/sources.cppThe
metricfeed exists only onws-staging.btclock.devtoday (GET https://ws.btclock.dev/api/v2/metrics→404). But the debt subscription is hardwired to production via two paths:shares_debt_socket = (data_source == 0)(line ~199): with a custom endpoint (dataSource=3), the core socket that is connected to staging is handed an emptymetric_keyslist and never subscribes todebt:US.data_source != 0) is built withBuildBtclockSourceUri(0, "", false)=wss://ws.btclock.dev/api/v2/ws— hardcoded production, ignoringceEndpoint.So there is no settings/runtime path to point the debt socket at the configured custom endpoint. This is broader than the staging period: even after GA, a user running
dataSource=3against a self-hosted btclock endpoint that serves metrics would still not get debt. Hardware-verified fix (2 lines):Optional refinement: have that
data_source == 1socket honorceEndpointinstead of hardcoding production.2. Host tests fail on CI —
ceEndpointdefault mismatchtest_host/test_settings_api.cppasserts theceEndpointGET default is"ws.btclock.dev", butcomponents/settings/include/settings/schema.hppstill declareskCeEndpoint's default as"ws-staging.btclock.dev"(anddocs/SETTINGS.mdagrees with the schema).GET /api/settingsemits the schema default, so the assertion fails deterministically →btclock_host_testsexits non-zero → CI red. Looks like a rebase leftover (test updated, schema/doc not). Pick one source of truth.🟢 Wire-contract alignment — fully verified
A live probe against
wss://ws-staging.btclock.dev/api/v2/wsreplaying the firmware's exact subscribe frame, plus feeding the real server bytes through the vendored ArduinoJson:Subscribed to debt:USbase/rate/ref/dpencode as float64 / float64 / int64 / int →is<double>()/is<double>()/is<int64_t>()/is<uint8_t>()all accept it.Because the Go encoder statically types
base/rateasfloat64, msgpack always emits0xcbeven for whole values, so theis<double>()guards are safe against integer-encoding drift by construction. No legitimate frame is ever dropped. The protocol side is correct; only the endpoint wiring was wrong.🟠 Medium
main/screens/panel_texts.cpp(FormatCompactDebt). The divisor table floors at1e6('M'), so any debt-in-BTC below 1M renders as a misleading₿0.0XM(e.g. Malta ≈₿0.11M), and the non-big-char path rounds*valuetouint64so a sub-1-BTC value collapses to0. Add a sub-million tier and a host test for a small-country BTC value.debtCurrencytoggle bounces the WebSocket —components/webserver/control_server.cpp. It's in theon_screens_changedtrigger, which postskRebuildScreens→SetSubscriptions→ unconditionalStop()+Start()on the live socket. ButdebtCurrencyonly changes rendering; the subscription set is invariant. Route it through the live re-render /MarkDirtypath instead of a TLS reconnect.data_source != 0user, even with zero debt screens enabled. Largely resolved by the Blocker-1 fix.main/CMakeLists.txtlinksdebt.cpp+ the 29-entry catalog + ~170 lines of formatting unconditionally into the 4 MB image. Rev A sits at ~19 KiB free, so please measure the Rev A build before merge. Feature parity across all variants is the goal — if it regresses past the headroom, reclaim space by trimming the footprint (e.g. a leaner catalog representation, folding the duplicated formatting), not by excluding Rev A from the build.main/sources/sources.cpptruly conflicts on a 3-way merge.mainmoved currency-fetch into a deferredRefreshUpstreamCurrencies(); a naive resolution drops the USD/EUR price subscription (which debt-in-BTC depends on) after the first connect. Rebase manually and add a test that USD/EUR survive the post-connect prune.DataSnapshot::Mergedebt loop has no test —components/data_core/hub.cpp.⚪ Low / nit (selection)
Per-tick
ReadRenderPrefs()(~18 NVS reads) in the debtShouldRenderbranch; the debt screen repaints everyminSecPriceUpds while displayed (the counter ticks continuously — EPD wear);BuildDebtPanelTextsis built 3× per paint;dpis decoded but never consumed; deadstarts_with("screen")branch inIsScreenVisibilityKey;ShouldRender/Renderdebt-key diverge only on the first frame (V8, N=8); untested edges inCurrentDebtValue(now<ref), theFormatCompactDebtunit-carry, and the 8-panel layout. Note: the "replace the US-only setting" comment is inaccurate — the prefs are purely additive, no migration involved.Suggested order before merge
ceEndpointfor thedata_source == 1socket too.ceEndpointdefault mismatch; get host tests green.main(semanticsources.cppconflict) + a test that the debt USD/EUR subscription survives the prune.debtCurrencyfrom the WS bounce.Reviewed by reading the full PR-branch files (not just the diff), an adversarially-verified multi-dimension pass, a live wire probe, and an on-device build/flash confirmation on Rev B.
View command line instructions
Manual merge helper
Use this merge commit message when completing the merge manually.
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.