Add lights support for Nook Glowlight 4 Plus (bnrv1300)

[backcountrymountains]

Split bnrv1300 out of the NOOK_GL4 device ID into a new NOOK_GL4PLUS ID with a dedicated NookGL4plusController.

Brightness is set via Settings.System.SCREEN_BRIGHTNESS — consistent with other Android e-reader controllers (e.g. TolinoNtxController, which uses the same lm3630a hardware). On e-ink devices the frontlight is driven by dedicated hardware that only responds to the system brightness setting, not window.attributes.screenBrightness. Requires the "Modify system settings" special app permission — grant via Settings → Apps → KOReader → Modify system settings, or adb shell appops set org.koreader.launcher WRITE_SETTINGS allow. This permission is reset on fresh install.

Warmth is set via the Nook's own com.nook.partner.service.GlowLightService — no root required. The controller sends a startService intent with action action_set_color_temperature and extra extra_color_temperature (0–100 scale). The service (a priv-app holding DEVICE_POWER) internally rescales by ÷10 for the BNRV1300 hardware and calls PowerManager.setFrontlightBrightnessColor() under its own privilege, bypassing both the SELinux restriction on sysfs writes and the DEVICE_POWER requirement for direct binder calls. Hardware range is 0–10. Note: the Nook's native display settings show 0=warm and 10=cool — KOReader's warmth slider uses the opposite convention (0=cool, 10=warm), so the controller passes the value through without inversion and lets KOReader's UI labeling handle user-facing direction. The controller falls back to su -c "echo $warmth > /sys/class/backlight/lm3630a_led/color" if the service is unavailable (e.g. com.nook.partner disabled).

Note: com.nook.partner may be disabled on some devices. Enable with adb shell pm enable com.nook.partner if warmth does not respond.

Fixes koreader/koreader#14574

Test plan

• Brightness slider changes screen brightness on Nook Glowlight 4 Plus
• Warmth slider changes color temperature (0–10 hardware range)
• Warmth works without Magisk root (via GlowLightService)
• Rapid slider movement does not cause freezes

---

This change is 

[Frenzie]

Looks fine to me in principle; as for the whole root stuff @hugleo knows more about that.

[Frenzie]

That does after 4, not before. ;-)

[hugleo]

Looks fine to me in principle; as for the whole root stuff @hugleo knows more about that.

Starting a new process every time setWarmth is set using su -c could cause freezes in the slider that makes several rapid calls. One should test.

[backcountrymountains]

I asked Claude about issues using "su -c" and it decided to change the code such that:

The persistent root shell is now in place — su is started once on the first setWarmth call and reused for all subsequent ones, eliminating the per-call process spawn and
  waitFor() block that would cause slider lag. If the shell dies for any reason, isShellAlive() detects it via exitValue() throwing IllegalThreadStateException, and runRoot()
  transparently restarts it.

I don't know if that's an improvement.

[Frenzie]

I'm not sure if you meant to force push this branch, but those last two commits about the wifi seem to have stowawayed themselves.

[backcountrymountains]

Oh man, I've definitely given Claude too much power! I'm sorry I'm messing this up. Do you think we can get the basic Glowlight 4 Plus changes added in? I'll have to ask Claude how to separate the wifi stuff from the basic Glowlight 4 Plus color and warmth functionality.

[Frenzie]

You can simply revert the last two commits (git revert 94e0ddc and git revert 53f50b8).

[hugleo]

I asked Claude about issues using "su -c" and it decided to change the code such that:

The persistent root shell is now in place — su is started once on the first setWarmth call and reused for all subsequent ones, eliminating the per-call process spawn and
  waitFor() block that would cause slider lag. If the shell dies for any reason, isShellAlive() detects it via exitValue() throwing IllegalThreadStateException, and runRoot()
  transparently restarts it.

I don't know if that's an improvement.

I don’t think it’s an improvement. I asked GemMindGPT to analyze it from your perspective/worldview:

---

Claude’s proposal solves the wrong bottleneck by introducing a much more fragile IPC architecture into a latency-sensitive path.

The original concern — spawning su -c on every slider update — is valid. Process creation plus waitFor() inside a UI-driven brightness callback can absolutely cause visible stutter. But replacing that with a permanently attached interactive root shell is not a clean optimization; it fundamentally changes the failure model of the component.

The issue is not simply “persistent shell bad.” The issue is that the implementation turns a deterministic one-shot command execution into a long-lived stateful IPC channel without implementing the machinery required to safely manage that channel.

1. The implementation is not actually managing the shell

The code opens an interactive su shell but treats it as if it were stateless fire-and-forget IPC.

That is incorrect.

Once you keep a shell open, you inherit all the responsibilities of stream lifecycle management:

• stdout consumption
• stderr consumption
• synchronization
• shell state validation
• command framing
• backpressure handling
• teardown semantics
• lifecycle cleanup

None of that exists in the proposed implementation.

The shell is effectively being used as an unmanaged daemon with a write-only pipe attached to it.

That is a serious architectural smell, especially inside Android where process lifetime is controlled externally by the framework.

2. The deadlock risk is real — even if rare

Your point about pipe buffering is correct and important.

The implementation never drains:

• process.inputStream
• process.errorStream

Even if the current echo command itself produces no output, the shell environment is not guaranteed to remain permanently silent. SELinux denials, su policy messages, shell startup warnings, vendor-specific logging, or even unexpected syntax errors can write to stderr.

Pipe buffers are finite.

Once full, the child process blocks on write. When the shell blocks internally, subsequent writes from the app can also block. Since this code is likely triggered from UI-adjacent execution paths, this creates the possibility of UI stalls or ANRs.

The important nuance is:

• this may not happen immediately,
• may not happen on all devices,
• may not happen during light testing,
• but the architecture permits it.

That is exactly the kind of latent bug that survives development and appears later as “random freezes.”

3. The error handling is objectively broken

This is probably the strongest criticism.

Using PrintWriter here is dangerous because PrintWriter intentionally suppresses IOException.

That means:

suWriter!!.println(cmd)

does not reliably indicate success.

If the shell dies, the pipe breaks, or writes fail, println() may silently fail while the code still returns true.

So the application can enter a false-success state where:

• UI updates,
• currentWarmth changes,
• but the hardware never changed.

This is worse than a visible failure because it destroys state consistency.

The implementation effectively assumes:
“write succeeded because no exception was thrown,”

but PrintWriter explicitly violates that assumption by design.

That alone is enough reason to reject the implementation in production code.

4. isShellAlive() is not meaningful correctness

Using:

p.exitValue()

inside exception-based control flow is already questionable, but the deeper issue is that it provides no real safety.

Even if isShellAlive() returns true, the shell can still die immediately afterward before the write occurs.

So the code still has:

• TOCTOU race conditions,
• silent write failures,
• undefined shell state.

The liveness check creates the illusion of robustness without actually guaranteeing correctness.

5. Long-lived root shells are especially questionable in KOReader’s architecture

This part is particularly important given KOReader’s NativeActivity/LuaJIT model.

KOReader is not a conventional Android app continuously operating inside a typical managed Activity lifecycle. It intentionally bypasses much of the Java-side orchestration and runs a Lua-driven event loop through NativeActivity.

That makes introducing a persistent Java-managed root shell even more questionable because:

• the shell lifetime is now disconnected from the actual rendering/event architecture,
• there is no explicit lifecycle ownership,
• no deterministic cleanup path,
• no guarantee the shell state remains synchronized with Lua-side state,
• no integration with KOReader’s existing event scheduling/throttling model.

In other words:
the proposed optimization injects a stateful Android-side IPC subsystem into an architecture specifically designed to minimize dependence on long-lived Java-side state.

That is the opposite direction of KOReader’s design philosophy.

6. The optimization is premature and probably unnecessary

Most importantly: nobody demonstrated that su -c is actually the bottleneck.

The entire redesign appears speculative.

Before introducing:

• persistent privileged processes,
• IPC state management,
• shell synchronization complexity,
• silent failure modes,
• lifecycle hazards,

the obvious first step should be measuring:

• slider callback frequency,
• actual su latency,
• UI thread blocking,
• whether warmth writes are even perceptible to the user.

There is a very good chance the real solution is simply:

• debouncing,
• throttling,
• coalescing slider events,
• or applying warmth only on drag release.

Those approaches preserve the simplicity and correctness of the original one-shot execution model while removing the pathological rapid-fire process spawning.

That is a much safer tradeoff than introducing a permanent root IPC channel.

Final verdict

Claude correctly identified a potential performance concern, but the proposed fix dramatically increases architectural complexity while weakening correctness guarantees.

The original su -c implementation may be inefficient, but it is:

• stateless,
• deterministic,
• self-cleaning,
• lifecycle-safe,
• and failure-isolated.

The persistent shell version is:

• stateful,
• unsupervised,
• partially unmanaged,
• race-prone,
• and capable of silent desynchronization.

For a small hardware control path inside KOReader, that is a very poor tradeoff.

[backcountrymountains]

Dang! Your AI just schooled my AI! Claude was impressed (I was, too, but I only understood maybe 50% of it). Claude decided to:

▎ Replaced the persistent root shell with a simple Runtime.exec(arrayOf("su", "-c", "echo $warmth > $COLOR_FILE")) per call, matching the pattern already
▎ used by TolinoRootController and BoyueS62RootController.
▎
▎ The specific issues called out are all resolved:
▎ - PrintWriter silent failure is gone — Runtime.exec throws IOException on failure
▎ - No persistent stdout/stderr pipes to deadlock on
▎ - No stateful IPC channel; every call is self-contained and self-cleaning
▎ - @volatile on currentWarmth for correct cross-thread visibility
▎ - Early return when the value is unchanged avoids redundant su invocations
▎
▎ On an e-ink device with 11 warmth steps, per-call su latency hasn't been a problem in practice. If it ever is, the right fix is debouncing at the
▎ KOReader Lua level (fire only on drag release), not a persistent shell.

[hugleo]

Instead of writing directly to /sys/class, you can look at the messages in the topics below showing how the original driver was developed. Maybe you can find a miracle method that works.

android-luajit-launcher PR #450
KOReader issue #11110

[backcountrymountains]

Claude seems to still be changing this PR even though I instructed it that we must troubleshoot problems locally, first. I'm sorry about that.
We went down a path of trying to use the Nook's internal color temperature method, but, although it worked in an adb shell, it required android.permission.DEVICE_POWER, for koreader which is not possible to grant to a user app.

[backcountrymountains]

So, following @hugleo's suggestion to look for other methods of changing the warmth setting, I just gave Claude full access to my Nook and had it decompile the system apps to see how the warmth was changed.
Claude found a system app that changed the warmth and now utilizes that app instead of using root! Yay!

[Frenzie]

gave Claude full access […] Claude found a system app that changed the warmth

That's kind of scary.

Anyway, that means COLOR_FILE and setWarmthViaSu still need to be cleaned up, doesn't it?

[hugleo]

Another thing.
Since you figured out setWarmth, maybe you can also figure out getWarmth for this funcction:
override fun getWarmth(activity: Activity): Int = currentWarmth

This way, if you change the setting outside of KOReader, you can get the consistent value from the Android system.

[backcountrymountains]

gave Claude full access […] Claude found a system app that changed the warmth

That's kind of scary.

Anyway, that means COLOR_FILE and setWarmthViaSu still need to be cleaned up, doesn't it?

It is a bit scary! But I told Claude to just pull files from the device to decompile them, not start changing important things on the device. We'll see if it complies or does a complete rm -rf /.

Claude wanted to keep them as a backup, but I think it's okay to get rid of them. It should be noted that we are using com.nook.partner to do the warmth settings, but it is the same app that takes control of the device to force it into the nook ecosystem and act as the default launcher, so we have to do some setup tasks to disable some of it's intents.

I'll work on getting rid of those root-requiring commands