Conversation
|
What starting frontlight level are you usually testing from? I never noticed this, but I'm mostly using fairly low brigthness levels (~20%). I'm somewhat wary of touching it, because getting it to behave more-or-less correctly without more eggregious issues was kind of a bitch ;p. I'll give it a try on a few devices ;). (Unrelated sidebar: I've always been wary of my nasty hackish approach w/ runInSubprocess here, I've often pondered about just writing a dumb standalone script that sends the ioctl via ffi [we already do it for WiFi/BT power in the startup script] and that we just |
|
Oh, and do you observe a similar behavior for the USBMS frontlight toggle? (Because that's using a much nicer ramp, because it's slower, as there are none of the timing constraints I mentioned earlier there). |
|
I am using a high brightness (~60). The USBMS frontlight toggle works much better (almost perfect). |
|
In the last commit I have changed the whole ramping thing to UIManager scheduled tasks. The new property "UIManager" can be used in the other powerd's (kindle, Android), too. We could also drop the whole sub process thing and do a simple And the good thing is, it is easy to abort the whole ramp. A few test have to be done, so draft by now. |
| last_aux_capacity_pull_time = time.s(-61), -- timestamp of last pull | ||
|
|
||
| is_fl_on = false, -- whether the frontlight is on | ||
| UIManager = nil -- will be updated when available |
There was a problem hiding this comment.
I'm a bit torn on that one ;).
I understand the reasoning, but I'm not necessarily a fan of it.
If we can't think of anything better, I would at least rename it to ui_mgr to make it clear it's not our usual module-local.
(There's no real technical issues, though, given that UIManager is a singleton. The testsuite might rain on your parade, though).
|
|
||
| -- This ramp down goes faster on high intensity and slower at the end. | ||
| -- The user will notice a linear brightness change. | ||
| -- The whole function gets calles at max log(100)/log(0.75) = 17 times, |
There was a problem hiding this comment.
nit: called ;).
And thanks for dumbing down the maths for us there, I appreciate it ;).
| -- On some devices (Sage) setting intensity to zero happens immediately, | ||
| -- which will lead to a jump and not a ramp. So we postpone the last change by 0.5s to get it smooth | ||
| self.UIManager:scheduleIn(0.5, self._postponedSetIntensityHW, self, end_intensity) | ||
| -- no reschedule here, as we are done |
There was a problem hiding this comment.
This will probably still need a Device check, because a lot of the older devices have the opposite issue: no matter the timing or the previous brightness level, switching from on to off incurs an extra delay, so this would make it much worse ;).
| self.UIManager:unschedule(self.turnOffFrontlightRamp) | ||
| self.UIManager:unschedule(self.turnOnFrontlightRamp) | ||
| self.UIManager:unschedule(self.setIntensityHW) | ||
| self.fl_rump_running = false |
There was a problem hiding this comment.
nit: fun typo there ;o). (rump means ass, essentially ^^).
|
|
||
| function KoboPowerD:_postponedSetIntensityHW(end_intensity) | ||
| self:setIntensityHW(end_intensity) | ||
| self.running = false |
There was a problem hiding this comment.
Is this actually checked anywhere? Or was it supposed to be fl_ramp_running?
|
I definitely like this much better, thanks! And...
Yeah, definitely, go for it! |
| function BasePowerD:readyUI() | ||
| self.UIManager = require("ui/uimanager") | ||
| UIManager = require("ui/uimanager") | ||
| self:readyUIHW() |
There was a problem hiding this comment.
über nit: you can also simply pass the UIManager reference along, much like what _setEventHandlers does ;).
Another random idea I had last night would have been to move the full implementation to Generic (while keeping it opt-in, and only actually used on Kobo), with a few knobs via arguments to allow implementations to choose the scheduling frequency and the final delay, if any.
I'm perfectly happy with what you've done here, though, especially since I don't really see us using this on other platforms (I think Kindles do the ramp for us, for example). ;).
There was a problem hiding this comment.
I will resolve the über nit and sleep/think about a move of the ramp to generic/powerd.
There was a problem hiding this comment.
FWIW, that was just a random 3AM idea, I'm perfectly fine with what you've done here ;).
| -- NOTE: This is essentially what setIntensityHW does, except we don't actually touch the FL, | ||
| -- we only sync the state of the main process with the final state of what we're doing in the forks. | ||
| -- And update hw_intensity in our actual process ;). | ||
| self.hw_intensity = self.fl_min | ||
| -- NOTE: And don't forget to update sysfs_light, too, as a real setIntensityHW would via setBrightness | ||
| if self.fl then | ||
| self.fl.current_brightness = self.fl_min | ||
| end | ||
| self:_decideFrontlightState() |
There was a problem hiding this comment.
This shouldn't be necessary anymore, since everything happens in the main thread now ;).
(Ditto for the other ramp).
There was a problem hiding this comment.
Well the whole gymnastics is necessary (at least for the down ramp).
If the user toggles fl with right left corner tap, the ramp starts. If the user taps again during the down ramp (roughly <1s) the notification "fl off" is shown again. This is easy to reproduce, and annoying.
With that code a tap again toggles fl on, which seems to be what the user wants.
(I was not able to reproduce the behavior on the on ramp maybe cause it is shorter. Anyway leaving both in is symmetric.)
There was a problem hiding this comment.
Okay, yeah, that makes sense; I'd possibly simply rejig the comments a bit then ;).
There was a problem hiding this comment.
Sorry, please forget my comment above. This was the original idea, why I have inserted/moved the code above.
Now there is a check in isFrontlightOnHW()
return self.hw_intensity > 0 and not self.fl_ramp_down_running
which does the job even better.
|
Ready to merge? |
|
I haven't written any release notes yet, but I have done a bunch of testing
(i.e., just reading and doing some things) on the current nightlies.
…On Mon, Apr 17, 2023, 22:03 poire-z ***@***.***> wrote:
Not ignoring you and your 2 PRs (this and #10306
<#10306>) - but as I'm not
familiar with the code involved, nothing to say :) but no objection!
No risk with them (possibly the ramp change on various devices ?), and a
stable release being near (as due 10 days ago :) - @Frenzie
<https://github.com/Frenzie> : when do you plan for it?
—
Reply to this email directly, view it on GitHub
<#10305 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABRQBL47GFZX6W62ZTRH6DXBWOZHANCNFSM6AAAAAAWW6KMCE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Oops, forgot about it. I'd like to test it on a few devices first (will probably do that tomorrow). |
NiLuJe
left a comment
There was a problem hiding this comment.
Don't bother with my nits just yet, as I'm liable to fix them during testing later tonight ;).
Reviewed 2 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Frenzie, @pazos, @poire-z, and @zwim)
frontend/device/generic/powerd.lua line 46 at r4 (raw file):
function BasePowerD:readyUI() UIManager = require("ui/uimanager") self:readyUIHW()
nit: Pass the UIManager ref along, like in setEventHandlers
frontend/device/kobo/device.lua line 172 at r4 (raw file):
frontlight_settings = { wait_before_turn_off_s = 0.0, -- time to wait before turning FL off. },
This doesn't quite work as a default for devices where we actually set frontlight_settings, because when we do, we create an all new table, so this is effectively gone and never accessed.
(i.e., this needs to either move to PowerD, or not be inside a subtable).
EDIT: Oh, I see the nil is handled when this is actually checked, so this ends up effectively being dead code; I'd just handle the comment/documentation in the one case that needs it.
Code quote:
frontlight_settings = {
wait_before_turn_off_s = 0.0, -- time to wait before turning FL off.
},frontend/device/kobo/powerd.lua line 348 at r4 (raw file):
if UIManager then -- Do nothing, if the ramp is allready running
nit: already
frontend/device/kobo/powerd.lua line 359 at r4 (raw file):
end -- This is fairly the same functionallity as in Kobo's turnOnFrontlightHW().
nit: functionality
frontend/device/kobo/powerd.lua line 360 at r4 (raw file):
-- This is fairly the same functionallity as in Kobo's turnOnFrontlightHW(). -- The whole function gets calles at 100/5 + 1 = 21 times,
nit: called
|
@NiLuJe Do you want to merge thus, as you have done the latest changes? |
This keeps working on my GloHD, when toggling frontlight. I may have some frontlight related bits (although none related to ramping) that may be at play - but before I investigate if it's just me, does anybody else not get the ramping on Suspend? |
Checking in a dark room :) and I actually do see the ramping - but it's happening on a quite black cover, so wasn't really noticable. |
|
The flash to white before the screensaver has always been there, what's changed is that the frontlight ramp could start earlier than it does now (depending on screen, ambient temperature and wifi shenanigans), so now you actually do see it, always (as well as the actual screensaver), and ramping happens dead last, always. |
|
I understand - and I guess I can understand the reason (showing wifi off info before shutting the light). It also feels a bit inconsistent:
I'm not sure now how it was before, but it felt smoother having it all happening at the same time, giving a view on the screensaver (and white flash) when light was lower. Anyway, no big deal - but no big smoothness either :) |
|
The main problem is we can't actually do it all together, especially on older, UP devices, as it will either affect the refresh timing, or the wifi teardown timing (which is crash-prone on some of the more broken NTX boards), or the ramp's smoothness ;). The previous approach was designed to preserve the refresh timing above all (while attempting not to upset wifi too much), at the detriment of the frontlight appearance. The current one attempts to properly separate each step, so it may appear to take longer. I happen to like being able to actually see the cover for a bit, FWIW ;o). |
|
As for the resume behavior, there's just basically less stuff happening (no wifi involved, even if restore wifi is enabled), no extra flashes; and the PWM drivers happen to have different timing properties for off -> on than on -> off. I haven't noticed any stuttering specifically related to how we do it vs. the ramp itself in a vacuum on any of the devices I can test on, but since they all do behave extremely differently, I can't really rule that out either ;). So it's entirely possible the PWM controller found on your device doesn't like it as much as before. So, yeah, it's faster and a lot more tied together, and I also happen to like that (while I don't care about delays when going to suspend, because you're not using the device; I feel like getting you to a ready state ASAP on resume is rather more critical). |
Well, on suspend, seeing the (white screen! +) cover with full light on is less essential: you know what you've been just reading. (and seeing that white screen full light on feels like illuminating the whole room when I hit the torch button by error on my phone :) On resume, we see the cover at very low light for 1 second (so, hardly seeing it now when testing in the dark). It's on resume - resuming after a day or a week - that seeing it (even 1s, but with full light) might be more useful: that's what I had been reading, i nearly forgot - ready state here you are (otherwise, I may need to go into Book information) :) But yes, the ramp is indeed quicker and smoother. |
|
It feels like most of this might be very subjective then ;o). What might be more universal is the point about the flash to white; I don't particularly notice it because, while I do read mainly in the dark, my frontlight levels are rather low (... because of the aforementioned "I read in the dark" thing, granted ;p). On the other hand, a white background is what you were seeing right before, so... eh :/. What's jarring might be the flash more than anything else (unfortunately, it's rather necessary to avoid the cover becoming a ghostfest). Dunno, I'll see how I feel after a bit more usage ;o). |
Oh, so the white is a flash (I'm used to seeing black flashes), and not an explicite white paint from us? |
|
Nope, it's a standard flash to a white screen, then a flash to the screensaver. The flash itself is always the same ;o). (I thought I'd edited it to say "flash to white" instead, but apparently not ;p). |
|
On my Libra,
I guess this two level switching is coming from the smooth ramping code. |
|
I don't think I can reproduce on any of my devices, but it sorta makes sense, with the button, as it might be racing against at least a couple of refreshes. EDIT: Hmm, probably specific to the Libra (& its cohort of broken friends) because of the EPDC races workarounds, the refreshes are fenced properly on my devices with non-hobbled display drivers... |
kobo_backlight_toggle.mp4Looks like this. Slow motion video to make it more visible to the camera. |
You can try to (temporarily) drop the workarounds by removing the hasReliableMxcWaitFor devcap override to test that theory... |
* afterResume had *two* different implementations, so the historical one that handled frontlight fixups no longer ran (regression since koreader#10426) * isFrontlightOn was completely broken, for a couple of reasons: * There was no is isFrontlightOnHW implementation, so when it ran, it mostly always thought the frontlight was on, because self.fl_intensity doesn't change on toggle off. * _decideFrontlightState was never called on Kindle, so isFrontlightOnHW was never really called, making isFrontlightOn completely useless. Call it in setIntensityHW's coda, as it ought to be. And properly document that. Generic *was* calling _decideFrontlightState is setIntensity, but *before* actually setting the frontlight, which makes no goddamn sense, so get rid of that, too. * Also fix frontlight toggle notifications (regression since koreader#10305) TL;DR: The PowerD API being a mess strikes again.
* afterResume had *two* different implementations, so the historical one that handled frontlight fixups no longer ran (regression since #10426) * isFrontlightOn was completely broken, for a couple of reasons: * There was no is isFrontlightOnHW implementation, so when it ran, it mostly always thought the frontlight was on, because self.fl_intensity doesn't change on toggle off. * _decideFrontlightState was never called on Kindle, so isFrontlightOnHW was never really called, making isFrontlightOn completely useless. Call it in setIntensityHW's coda, as it ought to be. And properly document that. Generic *was* calling _decideFrontlightState is setIntensity, but *before* actually setting the frontlight, which makes no goddamn sense, so get rid of that, too. * Also fix frontlight toggle notifications (regression since #10305) TL;DR: The PowerD API being a mess strikes again.
* Rewrite the loop mechanism to use scheduled tasks instead of a single blocking-ish subprocess. * Change the actual logic to be more pleasing to the eye, especially on newer devices, as those *may* natively ramp on set; and fix a bad interaction with that behavior that could lead to no ramp at all on ramp down. * Simplify Generic's Suspend handling to deal with the refresh ordering in a saner manner. The screensaver might be visible a tad longer than before this change before the frontlight actually ramps off.
…ader#10597) Fix koreader#10588 Regression since koreader#10305 While we're there, rejig the FL toggle callback shenanigans so that implementation details don't leak through to *other* implementations. (i.e., leave the Kobo mess in Kobo land, with only a minimal impact on the public API and its implementation).
* afterResume had *two* different implementations, so the historical one that handled frontlight fixups no longer ran (regression since koreader#10426) * isFrontlightOn was completely broken, for a couple of reasons: * There was no is isFrontlightOnHW implementation, so when it ran, it mostly always thought the frontlight was on, because self.fl_intensity doesn't change on toggle off. * _decideFrontlightState was never called on Kindle, so isFrontlightOnHW was never really called, making isFrontlightOn completely useless. Call it in setIntensityHW's coda, as it ought to be. And properly document that. Generic *was* calling _decideFrontlightState is setIntensity, but *before* actually setting the frontlight, which makes no goddamn sense, so get rid of that, too. * Also fix frontlight toggle notifications (regression since koreader#10305) TL;DR: The PowerD API being a mess strikes again.
On my Sage toggling Frontlight on gives a smooth brightness ramp; but toggling it off is a jump in brightness.
This PR fixes that behavioir:
*) Toggling FL on is not changed
*) Toggling FL of does a ramp (with an exponential decay, to make it smooth :) as the observer should like. Anyway not tested on other devices as a Sage).
More infos are in the comments.
This change is