Skip to content

Commit 1d6bcc0

Browse files
committed
feat: Implement DisplayServiceClient to address panel-blank bug
- Added a minimal DisplayServiceClient in harness.cpp to handle backlight control. - Updated harness.h to expose the current backlight level for testing. - Created scenarios in scenarios.cpp to ensure that the screen blanking correctly interacts with the DisplayService. - Modified main.cpp to ensure the panel is darkened correctly by calling display_backlight. - Bumped cinder-installer version to 0.1.8 in Cargo.toml and Cargo.lock. - Added .gitattributes to prevent line-ending issues in payload files. - Introduced check_payload_attrs.sh to verify that payload files are exempt from Git's EOL rewriting. - Enhanced verify_payload_manifest.sh to provide clearer error messages for line-ending issues.
1 parent ea49257 commit 1d6bcc0

17 files changed

Lines changed: 603 additions & 29 deletions

File tree

.gitattributes

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
# ── The release payload is shipped BYTE-FOR-BYTE. Git must not rewrite any of it. ──────────────
2+
#
3+
# WHAT WENT WRONG WITHOUT THIS (v0.1.7, run 33643552973). `release.yml` builds the installer on
4+
# `windows-latest`, where Git's default `core.autocrlf=true` converts LF→CRLF at checkout for every
5+
# file it AUTO-DETECTS as text. The payload binaries have NUL bytes, so Git leaves them alone and
6+
# they verified fine. The two text members did not:
7+
#
8+
# cinder-home/dist/stable/cinder-signature.sh: FAILED
9+
# cinder-home/dist/stable/cinder_components.conf: FAILED
10+
# sha256sum: WARNING: 2 computed checksums did NOT match
11+
#
12+
# `tools/release.sh` hashes them on Linux (LF); the runner checked out CRLF; the manifest gate
13+
# rejected the release. Nothing was actually stale — the bytes were rewritten in transit. This was
14+
# latent from the day the manifest gate was added: v0.1.6 was never tagged, so v0.1.7 was the first
15+
# tag the gate ever ran on, and it failed on its first real use.
16+
#
17+
# AND IT IS NOT JUST CI. `cinder-signature.sh` is copied to an ARM device and executed there. A
18+
# CRLF shebang is `#!/bin/sh\r`, which the device's kernel looks up as an interpreter literally
19+
# named "sh\r" — the script fails with a confusing "not found" naming a binary that plainly exists.
20+
# So the checkout that fails the hash is also a checkout that ships a broken script; the gate was
21+
# right to stop it, it just could not say why.
22+
#
23+
# `-text` (rather than `eol=lf`) is deliberate: it disables the conversion in BOTH directions and
24+
# on every platform, rather than trusting each contributor's `core.autocrlf`. These files are
25+
# build OUTPUT whose bytes are the artefact.
26+
cinder-home/dist/** -text
27+
28+
# The payload manifest itself, for the same reason: it is the record of those bytes, and a
29+
# checkout that rewrote it would be checking a rewritten record against rewritten files.
30+
cinder-home/dist/PAYLOAD.sha256 -text
31+
32+
# The scripts that are installed onto the device and executed by its shell. Same shebang hazard as
33+
# cinder-signature.sh above; these are simply the rest of that set.
34+
cinder-home/deploy/*.sh -text
35+
player/deploy/*.sh -text

.github/workflows/ci.yml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,21 @@ jobs:
137137
- name: Shell syntax + lint
138138
run: tools/shell_check.sh
139139

140+
# The release payload must be exempt from Git's EOL rewriting — and this has to be checked
141+
# HERE, on every push, not at release time.
142+
#
143+
# v0.1.7 is why. release.yml verifies the payload against a sha256 manifest, and that gate ran
144+
# for the FIRST TIME on the v0.1.7 tag (v0.1.6 was prepared but never tagged). It failed
145+
# immediately: `windows-latest` checks out with core.autocrlf=true, which rewrote the two text
146+
# members of the payload, so their hashes no longer matched. Nothing was stale — the bytes were
147+
# changed in transit — but the release was already cut before anything noticed.
148+
#
149+
# That is the same shape as the shellcheck lesson recorded in tools/shell_check.sh: a check
150+
# that only ever runs during a release is a check that fails during a release. This one is
151+
# cheap, deterministic, and runs on every push.
152+
- name: Payload is safe from line-ending rewrites
153+
run: tools/check_payload_attrs.sh
154+
140155
# The app itself, BOOTED. Everything above checks pieces in isolation; this links the real
141156
# main.cpp against fake Sony services and a virtual clock, runs the easel lifecycle, and
142157
# asserts on the call trace — that the paired list and the notification listener are set up

CHANGELOG.md

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,47 @@ level the commit history supports; from `v0.1.6` onward, entries are written as
1616

1717
## [Unreleased]
1818

19+
## [0.1.8] — 2026-09-02
20+
21+
### Fixed
22+
23+
- **The screen never actually went dark.** `set_backlight` has recorded since 2026-08-19 that the
24+
sysfs node alone does not darken this panel — measured with the node at 0, DisplayService at 2 and
25+
the panel still lit — and handles it. But neither path that blanks the screen went through it:
26+
`screen_auto_off` (idle timeout) and `screen_toggle`'s off-branch each carried their own copy of a
27+
raw `fputc('0')` to the node and stopped there. So the idle timer and the Power button both
28+
"turned the screen off" by writing a number the service overrides. Both now call one `panel_dark()`
29+
that caches the service's level and then zeroes both halves — the duplication is why the defect
30+
existed twice. *This is a standing bug in its own right and NOT the `g_ipc_dead` wedge; it was
31+
briefly attributed to that latch while diagnosing v0.1.7, which was wrong.* Two harness scenarios
32+
(`blank-idle`, `blank-order`) pin it, and both fail against the old code.
33+
- **Losing Sony IPC was completely silent.** When `run_guarded` unwinds a Sony call it latches
34+
`g_ipc_dead`, and from that moment the device cannot play, pause, skip, drive Bluetooth volume or
35+
sleep its panel until it is restarted — with the only trace a line in `cinderhome.log`, which
36+
cannot be read from the device. The status strip now carries a persistent `AUDIO STOPPED —
37+
RESTART` banner in the codec badge's place, on every screen, set from the guard's recovery path
38+
through a lock-free atomic (`cinder_set_ipc_dead`) because that path must not take a mutex. The
39+
banner replaces the badge and the NIGHT label and nothing else — an early version returned before
40+
the right-hand block and dropped the battery readout, which is the one indicator a degraded device
41+
most needs. Four host tests.
42+
- **New music copied over USB never appeared.** Nothing asked MediaStore to scan when a transfer
43+
finished — the DB reload on USB-MSC exit re-reads a store that has never heard of the new album,
44+
and the only trigger was a Settings row you had to know about. Exiting mass storage now arms the
45+
same bounded rescan campaign. *Device-unverified.*
46+
47+
### Changed
48+
49+
- **`.gitattributes` pins the release payload to byte-exact.** `release.yml` builds on
50+
`windows-latest`, where Git's `core.autocrlf` rewrote LF→CRLF at checkout for the two *text*
51+
members of the payload — so their hashes stopped matching and the v0.1.7 release failed its own
52+
manifest gate (run 33643552973) with every binary passing and only the text files failing. Nothing
53+
was stale; the bytes were changed in transit. This was latent from the day the gate was added:
54+
v0.1.6 was never tagged, so v0.1.7 was the first tag it ever ran on. `cinder-home/dist/**` is now
55+
`-text`, a new `tools/check_payload_attrs.sh` runs in CI on every push so the gate can no longer
56+
make its debut during a release, and `verify_payload_manifest.sh` names line endings as the cause
57+
when only text members fail. It also mattered beyond CI: `cinder-signature.sh` is executed on the
58+
device, and a CRLF shebang is `#!/bin/sh\r`.
59+
1960
## [0.1.7] — 2026-09-02
2061

2162
### Fixed
@@ -263,7 +304,8 @@ First tagged release.
263304
- The wired-headphone volume-change pop: 26 pops below volume 100 against 1 above, and it is not
264305
the shell or any mixer control ([`docs/`](docs/)).
265306

266-
[Unreleased]: https://github.com/superwilso/Cinder/compare/v0.1.7...HEAD
307+
[Unreleased]: https://github.com/superwilso/Cinder/compare/v0.1.8...HEAD
308+
[0.1.8]: https://github.com/superwilso/Cinder/compare/v0.1.7...v0.1.8
267309
[0.1.7]: https://github.com/superwilso/Cinder/compare/v0.1.5...v0.1.7
268310
[0.1.5]: https://github.com/superwilso/Cinder/compare/v0.1.4...v0.1.5
269311
[0.1.4]: https://github.com/superwilso/Cinder/compare/v0.1.3...v0.1.4

cinder-home/dist/PAYLOAD.sha256

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22
# Written by tools/release.sh AFTER a byte-for-byte rebuild check; verified by
33
# .github/workflows/release.yml before anything is published. Do not edit by hand —
44
# a mismatch here is the runner telling you the payload is not what was verified.
5-
tag v0.1.7
6-
21152a45d61339147b4b04deb8a6d0f18f652b423b3dabb549c57785c6d00327 cinder-home/dist/stable/cinder-home
7-
826bdaa90335f24704771adf003a20a4c40cc45c77fbdcdd0a43ca21e2f7cabc cinder-home/dist/stable/cinder-probe
5+
tag v0.1.8
6+
d46871e115543607d5c99acfb6a964daeed2b14a2077680c90f906b9feec70e8 cinder-home/dist/stable/cinder-home
7+
ff4945ca8d1c2e40b0e5d7f7924d7a8772c45d74b8f1e5d07ae7729a3674ebf3 cinder-home/dist/stable/cinder-probe
88
7788d381d609eb80dcb4db6305106dbfad4f8f0d01738022bd5c65d6f8cff7ba cinder-home/dist/stable/cinder-umount
99
42dd6d1f20d7602bf66ef2ff67174623cfb037fdc9564158e46b21ed55d20b26 cinder-home/dist/stable/cinder-power
1010
b58cc1d84c61d27df20fd0cdf01a031c49d146ec0d5725745fb7ca996c99b7c1 cinder-home/dist/stable/cinder-msc
0 Bytes
Binary file not shown.
0 Bytes
Binary file not shown.

cinder-home/harness/harness.cpp

Lines changed: 73 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -556,15 +556,85 @@ FILE* popen(const char* cmd, const char* /*mode*/) {
556556

557557
int pclose(FILE*) { return -1; }
558558

559+
// ── A minimal DisplayService, because the panel-blank bug was INVISIBLE without one ─────────────
560+
//
561+
// Everything else here degrades honestly when dlopen returns null: the app takes its "service not
562+
// available" path and the scenario still means something. The backlight is the exception, because
563+
// the service is not an optional extra there — it is half of the operation.
564+
//
565+
// `set_backlight`'s own comment has recorded since 2026-08-19 that writing 0 to the sysfs node does
566+
// NOT darken the glass (node at 0, service at 2, panel still lit). Both blank paths — the idle
567+
// timeout and the Power button — wrote only the node. With dlopen stubbed to null the harness saw
568+
// the node go to "0" and would have called that a pass, which is exactly the shape of the bug: the
569+
// observable half worked and the half that mattered was missing.
570+
//
571+
// So: enough of a DisplayServiceClient to be CALLED and COUNTED. Two vtable slots, matching
572+
// main.cpp's VIDX_SetLCDBacklightBrightness = 11 and VIDX_GetLCDBacklightBrightness = 12. The
573+
// remembered level starts at 2, which is the level the real service was measured reporting.
574+
static unsigned g_fake_bl_level = 2;
575+
576+
static void fake_bl_set(void*, const unsigned* level) {
577+
Lock l; ensure();
578+
g_fake_bl_level = level ? *level : 0;
579+
g_trace->push_back(Call{intern("display:SetLCDBacklightBrightness"),
580+
(long long)g_fake_bl_level, g_now_ms});
581+
}
582+
static void fake_bl_get(void*, unsigned* out) {
583+
Lock l; ensure();
584+
if (out) *out = g_fake_bl_level;
585+
g_trace->push_back(Call{intern("display:GetLCDBacklightBrightness"),
586+
(long long)g_fake_bl_level, g_now_ms});
587+
}
588+
589+
// EVERY slot gets a body, not just the two this fake models.
590+
//
591+
// The first version filled only 11 and 12 and left the rest null — and the very first run
592+
// segfaulted with PC=0x00000000, because ONE DisplayServiceClient is shared by the backlight AND
593+
// the touch-panel switch (main.cpp says so where it builds the client): `touch_set_sleep` calls
594+
// slot 13, SetTouchPanelValidate, on the same object. A partially-filled vtable is a worse fake
595+
// than no fake at all — it turns "this service is unavailable" into a crash in code that was
596+
// correct. So the table is filled with a recording no-op and the modelled slots overwrite it.
597+
static void* g_fake_display_vtbl[32];
598+
static void* g_fake_display_obj[2];
599+
600+
// Accepts and ignores its arguments. The real methods here are all `void(void*, const T*)`, and a
601+
// callee that never reads its arguments is safe to reach with any of those shapes.
602+
static void fake_display_noop(void*, const void*) {}
603+
604+
static void* fake_display_create(void) {
605+
for (unsigned i = 0; i < sizeof g_fake_display_vtbl / sizeof *g_fake_display_vtbl; ++i)
606+
g_fake_display_vtbl[i] = (void*)&fake_display_noop;
607+
g_fake_display_vtbl[11] = (void*)&fake_bl_set; // VIDX_SetLCDBacklightBrightness
608+
g_fake_display_vtbl[12] = (void*)&fake_bl_get; // VIDX_GetLCDBacklightBrightness
609+
// 13 = SetTouchPanelValidate — left as the no-op, but it must not be NULL.
610+
g_fake_display_obj[0] = (void*)g_fake_display_vtbl; // the vptr main.cpp reads
611+
return (void*)g_fake_display_obj;
612+
}
613+
614+
// A distinct non-null cookie so dlsym can tell which library it is being asked about.
615+
static int g_display_handle_cookie = 0;
616+
617+
/// Current fake backlight level, for scenarios to assert on.
618+
int cinder_harness_display_backlight(void) { return (int)g_fake_bl_level; }
619+
559620
void* dlopen(const char* path, int) {
560621
Lock l; ensure();
561-
g_trace->push_back(Call{intern((std::string("dlopen:") + (path ? path : "?")).c_str()), 0, g_now_ms});
622+
const std::string p = path ? path : "?";
623+
g_trace->push_back(Call{intern((std::string("dlopen:") + p).c_str()), 0, g_now_ms});
624+
// Only this one is faked; every other Sony .so still returns null so the optional-service
625+
// paths keep being exercised in their degraded form, which is what they are here to prove.
626+
if (p == "libDisplayService.so") return (void*)&g_display_handle_cookie;
562627
return nullptr; // no Sony .so on a build machine — the optional-service paths degrade
563628
}
564629

565-
void* dlsym(void*, const char* sym) {
630+
void* dlsym(void* h, const char* sym) {
566631
Lock l; ensure();
567-
g_trace->push_back(Call{intern((std::string("dlsym:") + (sym ? sym : "?")).c_str()), 0, g_now_ms});
632+
const std::string s = sym ? sym : "?";
633+
g_trace->push_back(Call{intern((std::string("dlsym:") + s).c_str()), 0, g_now_ms});
634+
if (h == (void*)&g_display_handle_cookie &&
635+
s == "_ZN3pst8services27DisplayServiceClientFactory14CreateInstanceEv") {
636+
return (void*)&fake_display_create;
637+
}
568638
return nullptr;
569639
}
570640

cinder-home/harness/harness.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,10 @@ void cinder_harness_fs_mkdir(const char* path);
104104
// What the app currently believes is in the file (after its own writes) — how a test checks that
105105
// something was persisted.
106106
int cinder_harness_fs_read(const char* path, char* buf, int cap);
107+
108+
// The fake DisplayService's current backlight level (see harness.cpp). 0 = the service was told to
109+
// turn the panel fully off; the sysfs node alone does NOT do that on this hardware.
110+
int cinder_harness_display_backlight(void);
107111
// Change a file PART WAY THROUGH the run, when the virtual clock reaches `at_ms`. The device's
108112
// world is not static — headphones get unplugged, a charger goes in, the battery falls — and
109113
// almost every interesting rule in the app is an EDGE rather than a level (bt_edge.h, jack_edge.h

cinder-home/harness/scenarios.cpp

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -814,8 +814,69 @@ static void s_library_changed(void) {
814814
"and it settles again rather than reloading for ever");
815815
}
816816

817+
// ── blanking the panel must reach the SERVICE, not just the sysfs node ───────────────────────
818+
//
819+
// THE BUG. `set_backlight` has carried this comment since 2026-08-19: "measured with the node at 0,
820+
// the service at 2 and the panel STILL LIT — the reported 'backlight off still powers the
821+
// backlight'". It handles that itself (level 0 -> display_backlight(0)). But the two paths that
822+
// actually blank the screen never went through it: `screen_auto_off` (idle timeout) and
823+
// `screen_toggle`'s off-branch both did a raw fputc('0') to the node and stopped. So both of them
824+
// "turned the screen off" by writing a number the service overrides, and the panel stayed lit.
825+
//
826+
// It went unnoticed for two weeks because the observable half worked. Anything watching the sysfs
827+
// node — including this harness before it grew a DisplayService fake — saw "0" and called it a
828+
// pass. These scenarios assert on the half that was missing.
829+
//
830+
// Both paths are covered separately on purpose: they are different callers of the same rule, and
831+
// fixing one and not the other is precisely the state the code was in.
832+
static void s_idle_blank_darkens_the_panel(void) {
833+
healthy_device();
834+
cinder_harness_script("cinder_get_screen_off_s", 30); // blank after 30s idle
835+
cinder_harness_set_budget_ms(120000);
836+
cinder_harness_run();
837+
838+
check(cinder_harness_count("log") >= 0, ""); // keep the trace alive for the dump below
839+
check_eq(cinder_harness_display_backlight(), 0,
840+
"the idle blank tells DisplayService to turn the panel off, not just the sysfs node");
841+
// …and the node too. Both halves, or the restore path has nothing to put back.
842+
char buf[32] = {0};
843+
if (cinder_harness_fs_read("/sys/class/leds/lcd-backlight/brightness", buf, sizeof buf) > 0)
844+
check(buf[0] == '0', "and still writes the brightness node");
845+
}
846+
847+
// The restore has to put back what was there, and that is an ORDERING rule:
848+
// `display_backlight_remember()` caches the service's own level, and it only works while that
849+
// level is still non-zero. Blank first and it caches 0 — the device then "restores" to 0 and wakes
850+
// to a black screen with the backlight node reading normal, which is indistinguishable from dead.
851+
// panel_dark() therefore remembers BEFORE it writes, and this is that rule stated as a test.
852+
//
853+
// Deliberately input-free. The obvious version of this drives the Power button and then a wake tap,
854+
// and two attempts at it asserted nothing: synthetic key events land before the app is consuming
855+
// its input nodes, so the scenario passed through a device that never saw the press. An ordering
856+
// assertion over the trace needs no input at all and cannot fail that way. The Power-button path is
857+
// not separately covered because it is the same function — screen_toggle() and screen_auto_off()
858+
// both call panel_dark(), which is precisely the change that fixed them; they had the defect twice
859+
// because they each carried their own copy of those four lines.
860+
static void s_blank_remembers_before_zeroing(void) {
861+
healthy_device();
862+
cinder_harness_script("cinder_get_screen_off_s", 30);
863+
cinder_harness_set_budget_ms(120000);
864+
cinder_harness_run();
865+
866+
check_eq(cinder_harness_before("display:GetLCDBacklightBrightness",
867+
"display:SetLCDBacklightBrightness"), 1,
868+
"the service level is READ before it is ever written (else the restore caches 0)");
869+
check(cinder_harness_count("display:SetLCDBacklightBrightness") >= 1,
870+
"and the service is actually driven");
871+
check_eq(cinder_harness_display_backlight(), 0, "ending dark, after the idle blank");
872+
}
873+
817874
struct Scenario { const char* name; void (*fn)(void); const char* what; };
818875
static const Scenario kScenarios[] = {
876+
{ "blank-idle", s_idle_blank_darkens_the_panel,
877+
"the idle blank reaches DisplayService, not just the sysfs node" },
878+
{ "blank-order", s_blank_remembers_before_zeroing,
879+
"the service level is read before it is zeroed, so the restore has something to restore" },
819880
{"boot", s_boot, "the app boots and brings Bluetooth up with it"},
820881
{"no-services", s_no_services, "no Sony service exists: degrade, never die"},
821882
{"bt-late-service", s_bt_late_service, "the BT service arrives after the app does"},

0 commit comments

Comments
 (0)