Repository navigation
fix(panel): Galahad II LCD brightness fix - #3
Conversation
2aef443 to
63b84ce
Compare
|
Squashed down to the minimal diff - dropped every comment/formatting-only change, kept only what's strictly required: Also closed #1 per your note - I'll resubmit the pump RGB as a fresh, independent PR off |
geretti
left a comment
There was a problem hiding this comment.
Reviewed at head 63b84ce3. The production side is right and I have no changes to ask for there; the test half and the description still carry pieces of #1. Items 1-3 are required, 4-5 optional. Line numbers are against 63b84ce3.
What I checked
- Protocol:
sgtaziz/lian-li-linuxhydroshift_lcd/controller.rsset_brightness(line 972) sendsLcdControlMode::LcdSetting = 4, andset_rotationdoes the same, for the whole family. The "settings mode hands the glass back" comment onmainwas never hardware-checked (commit9b8bafefsays so in its message), so 0x04 for both Galahad II and HydroShift is the correct reading. - Cross-thread HID access:
ApplyBrightness()now runs from the HTTP thread.JpegPanelHub.SetBrightnessandSendFrameboth take_lock, so the control write and its ack drain are serialized against the frame path. Fine. - Branch is 6 commits behind
origin/main, merges clean. TheJpegPanel*,LianLiAio*andStreamedPanelCoordinator*suites (87 tests) pass on the head; I did not run the full suite or an AOT publish.
1. (required) Remove the #1 leftovers from the tests
The comment on this PR says the comment/formatting-only changes were dropped; 63b84ce3 still has them.
tests/Nexus.Service.Tests/JpegPanels/LianLiAioHandshakeTests.cs
- Lines 59, 73, 115, 128:
new LianLiAioHandshake(\n "…", 24)wrapped over two lines. Onmainthese are one line, and the wrap is what the removed #1 constructor parameter needed. Put them back on one line. - Line 57
Galahad_attach_keeps_the_glass_in_application_mode: assertsOnAttachbehaviourmainalready has and this PR does not change. Delete. - Line 71
Galahad_brightness_packet_uses_the_hardware_verified_settings_mode: keep. It is the one new test that pins the fix byte-for-byte.
tests/Nexus.Service.Tests/JpegPanels/JpegPanelHubTests.cs
- Line 76: the existing frame test was switched from
GalahadIiLcdtoHydroShiftLcdwith nothing in this PR motivating it. RestoreGalahadIiLcd. - Line 99
Galahad_frames_use_the_1024_byte_b_jpeg_reports, line 121Galahad_discovery_asks_the_overlay_for_raw_bgra_and_encodes_jpeg, line 186Galahad_uses_the_b_jpeg_frame_channel_and_control_stays_on_b: all three assert model-table and frame-path behaviour that is unchanged onmain. Delete them. (If any of them exists to guard something #1 will change, it belongs in that PR.) - Line 195
Dimmable(): reflowed from one expression to a multi-linewith { ... }block for no change in content. Restore themainform.
After this the test diff should be: the 0x04 assertion change at JpegPanelHubTests.cs:214, the renamed/reworded HydroShift test in LianLiAioHandshakeTests.cs (A_brightness_change_uses_the_family_wide_settings_mode), the new Galahad packet test, and the coordinator test.
2. (required) Fix the PR description
First Summary bullet, "Puts the Galahad II LCD glass into application mode so pushed frames actually render", describes no change in this diff: LianLiAioHandshake.OnAttach already sends ModeApplication on main. Drop the bullet. The other two bullets are accurate.
Per CONTRIBUTING.md, the validation section is your statement of what ran on your machine, so please also confirm it still matches after the test cleanup (test-only changes do not need a hardware re-run).
3. (required) Rename a now-wrong test name
JpegPanelHubTests.cs:203 SetBrightness_reaches_an_attached_panel_in_application_mode now asserts LcdSettingMode, so "in_application_mode" is false. Something like SetBrightness_reaches_an_attached_panel_in_lcd_setting_mode.
4. (optional) Make ApplyBrightness void
IStreamedPanelTransport.cs:48, JpegPanelStreamTransport.cs:68, StreamedPanelCoordinator.cs:198. The only production caller, PanelRoutes.cs:339, discards the result, and the bool has mixed meaning inside TryApplyBrightnessLocked (true for "record has no value / unchanged" at lines 111 and 117, false for "TTL not expired" at line 94 and for a rejected write at line 119). A void keeps the interface honest about what callers use. If you take this, the coordinator test asserts on the fake's ApplyCalls / LastApplied already, so nothing else moves.
5. (optional) Inline TryApplyBrightness
JpegPanelStreamTransport.cs:77 is a one-line lock wrapper with a single caller at line 159. lock (_brightnessLock) { TryApplyBrightnessLocked(); } inside PushFrame reads the same and saves a method.
63b84ce to
11a1e8d
Compare
|
All fixed at 11a1e8d (squashed to one commit, same as before). 1. #1 leftovers in tests - removed. 2. PR description - dropped the inaccurate bullet, updated the #1 reference to point at #5 (the pump RGB resubmit). Test-only cleanup, so the validation section still matches what ran on hardware. 3. Test rename - 4/5 (optional, took both) - |
|
Checked One thing to fix before this can go in: the branch is no longer one commit on
Please make it one commit whose parent is |
11a1e8d to
7d93b68
Compare
|
Rebased onto
Force-pushed to this branch. |
|
Everything else stays as is. Amend into the one commit and force-push; comment/formatting only, so no hardware re-run. I'll approve the CI run again when it lands. |
…ately - LianLiAioHandshake.ApplyBrightness now sends the reference driver's LcdSetting mode (0x04) instead of application mode (0x01), which is what the panel actually expects for a brightness-only control write. - A brightness change now applies immediately (IBrightnessPanelTransport gets ApplyBrightness(), wired through StreamedPanelCoordinator and the panel-device PATCH route) instead of waiting for the frame-path's periodic recheck. Tested on real Galahad II LCD hardware: brightness applies instantly, panel renders pushed frames correctly with Nexus Control on.
7d93b68 to
ac34b9c
Compare
|
Restored the three leftover comment/structure diffs from the old #1 branch:
Comment/formatting only, fix bytes unchanged. Force-pushed as `ac34b9c2`. |
geretti
left a comment
There was a problem hiding this comment.
All rounds addressed at ac34b9c2: single commit on main, diff is the fix plus its tests only, CI green.
Split out from #1 per review feedback there - just the brightness fix, so it can merge on its own while the pump RGB work (now #5) takes its time.
Summary
LcdSetting(0x04) is the reference driver's family-wide brightness-only mode (not Galahad-specific - HydroShift takes it too), applied unconditionally instead of gated behind a per-model constructor parameter.Test plan
dotnet buildsucceedsI've read and agree to the CLA (CONTRIBUTING.md / CLA.md).