feat: add Beep On Touch to device settings - #928
Conversation
Expose the firmware touch_beep setting (Tidbyt Gen2) the same way as disable_touch: store the value reported in client_info, send it through update_firmware_settings (web form and API), return it as touchBeep in the device API, and add a checkbox under Live Firmware Settings. The checkbox is only shown once the device has reported touch_beep, so older firmware that does not know the setting does not get a control that does nothing.
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughChangesTouch beep firmware setting
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Firmware
participant WebSocket
participant Server
participant ManagementUI
Firmware->>WebSocket: Report touch_beep
WebSocket->>Server: Store TouchBeep in device info
Server->>ManagementUI: Render supported setting
ManagementUI->>Server: Submit touch_beep update
Server->>Firmware: Send firmware settings command
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 7 files. (4 skipped: 4 unsupported.)
Comment |
What
Adds the new firmware setting
touch_beep("Beep On Touch", Tidbyt Gen2 only) to the server, so it can be toggled from the device edit page and the API instead of only through the WiFi config portal.Firmware side: tronbyt/firmware-esp32#159. The firmware reports
"touch_beep"inclient_infoand accepts{"touch_beep": true|false}as a WebSocket setting, exactly likedisable_touch. Unlikedisable_touchit takes effect immediately, no reboot needed.How
This mirrors #903 (
disable_touch) field for field:ClientInfo/data.DeviceInfo: newTouchBeep *bool, stored when the device reports it (Infois JSON, so no migration)update_firmware_settings(web form):touch_beepadded to the boolean fieldstouchBeepin the device payload and inFirmwareSettingsUpdate, forwarded to the device astouch_beeptidbyt_gen2blockAPI.mdOne difference from
disable_touch: the checkbox is only rendered once the device has reportedtouch_beep. Firmware that predates the setting never reports it, so those devices do not get a control that does nothing. I used the reported value rather than aSupportsColorOrder-style version gate because the firmware PR is not released yet; happy to switch to a version check once there is a version to compare against.Testing
go build ./...,go vet ./...,go test ./...pass;gofmtanddjlint --profile=golang(check + lint) are cleanclient_infowithtouch_beepis persisted;touchBeepis returned byGET /v0/devices/{id}and forwarded by the API settings update; the web form sendstouch_beeptrue and false to the device; the checkbox is shown for a Gen2 only after the field was reported (and is checked accordingly), and never for other device typesNot tested: end to end against a device (I run the firmware PR on a Gen2, but against the released server), and
golangci-lintwas not run locally. The German wording is mine, corrections welcome.This only makes sense once tronbyt/firmware-esp32#159 is merged, so feel free to leave it open until then.
Summary by CodeRabbit
New Features
Documentation