Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slightly less crappy Nightmode #4871

Merged
merged 37 commits into from Apr 8, 2019

Conversation

Projects
None yet
3 participants
@NiLuJe
Copy link
Member

commented Apr 3, 2019

Companion PR to koreader/koreader-base#884

  • Basically flags devices known to be stable when using PxP inversion.
  • Plus, random fix for #4870 ;).
  • A few FrontLight tweaks & cleanups on Kobo:
    • Moved the Kobo-specific startup status insanity to Kobo-specific init
    • Made turnOff/turnOn frontlight do a smooth ramp down/up
    • On Kobo, use turnOff/turnOn for suspend/resume, to get that smooth toggle
    • On Kobo, for NaturalLight w/ a mixer, only set warmth for setWarmth, and only set Brightness for setBrightness, otherwise, it tried to set both with not in-sync values, which made the FL widget jittery.

NiLuJe added some commits Apr 3, 2019

NiLuJe added some commits Apr 3, 2019

Not actually a color space, more of a pixel format, so this is slightly
more accurate, while potentially less arcane than saying Y8 or pixel
format, or 8bpp...

The eInk palette is essentially a 4-bit grayscale palette though ;).
Those ovverrides are redundant
Generic already calls _setIntensity(fl_min), and fl_min is set to 0 on
both of these platforms, always.
Frontlight refinements...
Moved the Kobo-specific startup status insanity to Kobo-specific init
Made turnOff/turnOn frontlight do a smooth ramp down/up
On Kobo, use turnOff/turnOn for suspend/resume, to get that smooth
toggle
On Kobo, for NaturalLight w/ a mixer, only set warmth for setWarmth,
and only set Brightness for setBrightness, otherwise,
it tried to set both with not in-sync values,
which made the FL widget jittery
Move the ramping FL toggle to Kobo-only
The lipc handle behaved very wonkily on Kindle from a fork'ed process...
Fix setWarmth/setBrightness for non-mixer devices, too.
Because I assume white really means white...

Also, don't sleep on devices w/ NaturalLight for the ramping up/down, as
it's usually not needed, and sleeping just makes it jittery.
Kill debug
(And revert the non-mixer thing from the previous commit, because that
made non sense, and would probably lead to a very greenish or very
reddish screen ;)).
Revert that double-check from the generic code.
Since it was only ever really needed on Kobo.
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Okay, added some frontlight tweaks on Kobo, because it's kind of a mess there :D. c.f., the commit message from 5b70446 (although it's only worth reviewing the final state, as, err, things moved a lot after testing on various devices :D).

NiLuJe added some commits Apr 5, 2019

Possibly fix AutofrontLight test
It most likely relied on the insane re-set to on/off on powerd init,
which already did not make much sense on Kobo, but is definitely
utterly nonsensical on Kindle, while this is a Kindle-specific plugin
;).
:?
O_O
:!

NiLuJe added some commits Apr 5, 2019

:|
~_~
@@ -764,7 +764,7 @@ function ReaderHighlight:onHoldRelease()
text = "Copy",
enabled = Device:hasClipboard(),
callback = function()
Device.input.setClipboardText(self.selected_text.text)
Device.input:setClipboardText(self.selected_text.text)

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 5, 2019

Contributor

The clipboard functions are actually functions and not methods, so this one (with parameter) should not work anymore as intended when switched from . to :. The others won't mind, but I guess they should still use .. (cf #4482 (comment) where I thought about making them methods, but didn't :)

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Apr 5, 2019

Author Member

By "others" you mean Clipboard stuff, right? Basically, that only leaves hasClipboard: ;).

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 5, 2019

Contributor

Yep, (has|get|set)Clipboard(), that you restored as they were it seems, as they are no more in the diff.
Haven't looked at each of the Device:xxx if they are really a method, but mostly everything should be a method :)

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Apr 5, 2019

Author Member

Yeah, I only rg'd for evice\., so I shouldn't have touched anything else, and the has/get/can stuff in there is usually a method ;p.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

(You're courageous for having a take at that frontlight stuff :) I swear I wouldn't anymore after #3143 and #3163, too fragile ... But great if you make that cleaner :)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Looks alright at a glance; didn't test; nothing to add to @poire-z :-)

Unbreak Clippy
Thanks ;).
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

I mainly want to murder the unit test that's (failing) to test a non-existent scenario, but the actual changes look sound ;p.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

I'll test all that on my Kobo this evening.
(Looks like 2019.04 is nearly ready for release, so may be wait for it to happen before merging this risky frontlight stuff and new'y HW invert :)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Yes, I had some time yesterday so I prepped a little release note. I can tag it tomorrow unless there are any urgent remaining issues I'm not aware of?

PS https://github.com/koreader/koreader/milestones/2019.04

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

unless there are any urgent remaining issues I'm not aware of?

Nope. No real changes since a few days, should be pretty stable :)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

I'll test all that on my Kobo this evening.

No problem noticed so far on my GloHD.
No change noticed with nightmode, but I never really used it.
The progressive off<>on frontlight toggling is nice too (it's smooth when ON>OFF, but when OFF>ON, there is somehow a small pause at ~20% and it feels a bit less smooth - GloHD only?).

Okay, this passes.
And it almost makes sense.
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@poire-z : Noticed that on the H2O, too, so, that'd be all devices using the ioctl (i.e., without NaturalLight).

I'm not sure if it's a quirk of the ioctl itself when going to 0, or load-related, since it tends to hang just to display the "Frontlight Off" toast.
Which, incidentally, makes it readable, so I'm not too bothered by it, but it is weird ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@poire-z: Yep, it's a hardware quirk.

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/ioctl.h>
#include <sys/stat.h>

#define CM_FRONT_LIGHT_SET 241

int main(int argc, char **argv)
{
	int ntx_io, retval, ptr;
	ntx_io = open("/dev/ntx_io", O_RDONLY | O_NONBLOCK | O_CLOEXEC);

	uint8_t fl_min = 0U;
	uint8_t fl_max = 100U;
	uint8_t fl_cur = 20U;
	// Ramp-down
	for (uint8_t i = 1U; i <= 5; i++) {
		ptr = __builtin_ifloorf(fl_cur - ((fl_cur / 5.0f) * i));
		retval = ioctl(ntx_io, CM_FRONT_LIGHT_SET, ptr);
		printf("ioctl(ntx_io, CM_FRONT_LIGHT_SET) set to %03d%%; returned %d\n", ptr, retval);
		if (i < 5) {
			usleep(35 * 1000);
		}
	}

	usleep(2500 * 1000);
	// Ramp-up
	for (uint8_t i = 1U; i <= 5; i++) {
		ptr = __builtin_iceilf(fl_min + ((fl_cur / 5.0f) * i));
		retval = ioctl(ntx_io, CM_FRONT_LIGHT_SET, ptr);
		printf("ioctl(ntx_io, CM_FRONT_LIGHT_SET) set to %03d%%; returned %d\n", ptr, retval);
		if (i < 5) {
			usleep(35 * 1000);
		}
	}

	close(ntx_io);
}

Behaves the same way ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@poire-z : Re-reading your message, you're seeing the stutter on the opposite side I do. Huh. (I'm seeing it when shutting the light off, i.e., the ramp down).

EDIT: And, now that I've put two and two together, I'm pretty sure I've seen this happening in Nickel, which makes sense, now ;).

@Frenzie Frenzie added this to the 2019.05 milestone Apr 6, 2019

@NiLuJe NiLuJe merged commit 4005bf6 into koreader:master Apr 8, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Apr 11, 2019

Frenzie added a commit that referenced this pull request Apr 12, 2019

[fix] Update AutoWarmth state on resume (#4901)
Regression since #4871
Fix #4895

* Hide the "Configure" button in the FL/NL widget on devices where it
won't do anything

(i.e., those w/ a NL mixer).

@NiLuJe NiLuJe referenced this pull request Apr 20, 2019

Merged

Some more FL fixes #4924

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.