Allow display mode "None" on basic 7-segment wheels - #97
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughDisplay output now tracks successful ownership. ChangesDisplay ownership and disabled-mode handling
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant FanatecWheelDeviceInstance
participant DisplayEncoder
participant WheelbaseTransport
FanatecWheelDeviceInstance->>DisplayEncoder: SetDisplay for active mode
DisplayEncoder->>WheelbaseTransport: SendCol01(frame)
WheelbaseTransport-->>DisplayEncoder: accepted or rejected
FanatecWheelDeviceInstance->>DisplayEncoder: SetDisplay(blank frame) for ModeNone
DisplayEncoder->>WheelbaseTransport: Retry blank frame until accepted
FanatecWheelDeviceInstance->>DisplayEncoder: Release after successful handoff
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
"None" previously existed only for the legacy page on ITM wheels. Offering it on basic 7-segment wheels lets another application own the wheel display while FanaBridge keeps driving the LEDs. With mode "None" FanaBridge never writes a display report, except a single blank when switching into "None" (retried until the wheel accepts it). The blank also releases display ownership, so the disconnect, End, and plugin shutdown cleanups no longer blank content that is no longer ours.
ad405bd to
4080fc5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/FanaBridge/Adapters/FanatecWheelDeviceInstance.cs`:
- Around line 495-501: Update the display-test cleanup flow around
_displayManager.Clear() to store its result in _legacyBlanked, and call
plugin.Display?.Release() only when the clear is accepted; preserve ownership
when clearing fails so UpdateSegmentDisplay() can retry the blank. Add a
regression test covering switching DisplayMode to None during an active display
test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6d7dc73-1569-48dd-bcdc-39deecc96762
📒 Files selected for processing (9)
src/FanaBridge.Core/Protocol/DisplayEncoder.cssrc/FanaBridge/Adapters/DisplaySettings.cssrc/FanaBridge/Adapters/FanatecDisplayDriver.cssrc/FanaBridge/Adapters/FanatecWheelDeviceInstance.cssrc/FanaBridge/FanatecPlugin.cssrc/FanaBridge/UI/ScreenSettingsPanel.xamlsrc/FanaBridge/UI/ScreenSettingsPanel.xaml.cstests/FanaBridge.Tests/FanatecDisplayDriverTests.cstests/FanaBridge.Tests/FanatecWheelDeviceInstanceTests.cs
💤 Files with no reviewable changes (1)
- src/FanaBridge/UI/ScreenSettingsPanel.xaml.cs
The display-test handback and the blank-once path both ran in the frame the test was released, so mode "None" sent two blanks instead of one. The handback also released display ownership before its clear was accepted, dropping it while our own test residue was still on screen. The handback now leaves mode "None" alone: the blank-once path already clears the residue, latches only on an accepted write, and releases ownership there.
Summary
A user asked whether FanaBridge can leave the wheel display alone so another application (e.g. the vendor software) can drive it, while FanaBridge keeps the LEDs. On ITM wheels that already works (Legacy Display Mode "None" + the ITM checkbox), but basic 7-segment wheels had no off switch: the "None" option was hidden in the UI and the drive path had no gate, so FanaBridge rewrote the display on every gear/speed change.
This offers "None" on basic wheels and makes it mean what it should everywhere: with mode "None", FanaBridge never writes a display report — except a single blank when switching into "None", retried until the wheel accepts it.
Changes
UpdateSegmentDisplayused by both the ITM and basic branches.StopDrivingHardware), deviceEnd(), and the driver itself (belt-and-braces guard so "None" can never fall through to the unknown-mode → Gear default) all skip their exit blank when the display isn't ours.DisplayEncodertracks whether FanaBridge owns the current display content (HasWritten, released by the accepted handoff blank). Plugin finalize only blanks the display when the content is actually ours — previously it blanked unconditionally at exit, which would stomp another application's content even in "None".Testing
End()staying silent in "None" (and still blanking in active modes), and ownership release after the handoff blank.HasWrittenlatch/release.An independent model review flagged the shutdown-blank ownership gap (fixed here); it also noted the reconnect re-blank one-shot, which is kept deliberately — the ITM legacy page relies on it after reconnects, and on basic wheels a rim swap power-cycles the display anyway.