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

Some more FL fixes #4924

Merged
merged 5 commits into from Apr 15, 2019

Conversation

Projects
None yet
6 participants
@NiLuJe
Copy link
Member

commented Apr 15, 2019

Fix #4923 & #4925

NiLuJe added some commits Apr 15, 2019

Don't override hw_intensity when we start with the FL off
Fix #4923

First toggle is still be funky, but it essentially recovers.
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Note that in this specific case I can't get the first on/off cycle to behave in a sane manner because BasePowerD stomps on our fl_intensity/hw_intensity shenanigan, so it essentially breaks the first toggle, because it tries to set a state that is already the current one, instead of jumping directly to the other side of the equation (which is what would have happened had fl_intensity not been stomped on).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

But at least this way it ends up toggling something, instead of being stuck because it's below fl_min.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Aha. Got it.

This shit is insane. :D.

@NiLuJe NiLuJe merged commit 717db55 into koreader:master Apr 15, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@houqp

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Yeah, the frontlight handling logic is not fun to work with...

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

The fact that the FL status couldn't actually be queried from the HW until Mk. 7 is seriously bonkers. That probably didn't help designing this when everyone wanted to support his own workflow ;).

@cramoisi

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

so it essentially breaks the first toggle, because it tries to set a state that is already the current one

@NiLuJe : I'm not sure I understand. I applied the PR but nothing really changed, the frontlight doesn't toggle on

@cramoisi

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

OTA update fixed the thing, thanks :)

@Hzj-jie

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

It looks like the frontlight status is still breaking on my voyage after this change. Circumstances:

  1. When the frontlight is off, level / on is shown in status bar; when the frontlight is on, off is shown.
  2. The status bar won't update immediately after auto-frontlight switches the frontlight.
  3. Several first clicks on bottom-left (kobofrontlight plugin I remember) do not take effect.

@Frenzie Frenzie added the bug label Apr 17, 2019

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

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Also, I don't know if this is an intentional change but now you can reduce the brightness all the way to 0. Adjusting brightness (from 1 to 100) and toggling (between 1-100 and 0) have been separate actions.

@Biep

This comment has been minimized.

Copy link

commented Apr 17, 2019

And see my remark at #4925.

@cramoisi

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

On my KA1 all is working as intended with the last OTA update... (toggle is working and value is recorded between KOReader session)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

I disable the gesture controller. I mean with the widget. But it looks like something is going wrong while updating (probably caused by my being out of disk space the other day). So I'll get back to you on that one.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

I'll double check on my end, because yeah, that shouldn't haven't changed, and I remember checking ;).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Pressing Min on the widget goes to 1. Pressing -1 goes to 0. I'm 99 % sure it stopped at 1 before.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

The frontlight still keeps turning on after resuming from sleep btw. But only through the toggle button on the widget I think, not with the gesture manager action.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

You're right, the stable does seem to behave the same way, going down to 0.

Never mind then, although it is slightly confusing that -1 can go lower than min.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

@Hzj-jie : I can only reproduce the status bar thing on my end, and it's probably unsolvable without massive kludges like on Kobo (kthxbye).

We do NOT update our own internal tracking of the FL on toggle on purpose (so we can restore it to the same level on the next toggle).
So we can't force-feed the status bar the "new" value, because it gets it from that internal variable we're relying on.
It's either that, or it might actually be getting it from a hard-query to the HW on Kindle, and, since it's done asynchronously, the FL probably hasn't yet been updated when we try to read the "new" status, so we essentially update to the "old" status ;).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Btw, doesn't the statusbar update once or twice a minute these days for the clock?

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

I vaguely seem to remember a scheduleIn with a * 60 somewhere, so, possibly.

@cramoisi

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@Frenzie only if you enable the option "Auto refresh time" (I didn't enable it, the page-turn refresh is good enough for me)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

@cramoisi Right, thanks. So if one wants the FL status to adjust, I'd suggest turning that on. :-) (Or does it only refresh the time?)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

@cramoisi

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@Frenzie : I don't know ; you could revert the patch for testing purpose :)
@NiLuJe : I use FL at 1% : in darkness I can read without killing my eyes but in daylight, it's invisible, and I cannot tell if it's on or off without this widget

@Biep

This comment has been minimized.

Copy link

commented Apr 20, 2019

Actually, 1% is too bright for me in the dark - but I understand it is the minimum achievable.
Is that because the Nickel API is used, or because the underlying hardware (or Linux) doesn't allow smaller values?

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

There's no such thing as a Linux limit, but it is indeed at the Linux kernel level that the limits we're directly dealing with are implemented.

Either the people at Kobo or the people who provide the hardware to Kobo implement a Linux driver, hopefully based around necessary hardware limits and not around some idea that no one would want it less bright. For example, if you reduce the frequency/brightness of a LED too much you'd effect flickering. Such limits could be hardwired (nigh impossible to get around given how intricately it's all put together) or hardcoded (quite hard to get around, but potentially easier).

You can find the kernel source here: https://github.com/kobolabs/Kobo-Reader

With that you can compile a custom kernel and end up with something like https://github.com/lgeek/okreader

For several Kobo devices we could get multitouch that way even though Kobo doesn't provide it on those devices. I can only assume they thought it didn't work well enough.

On some devices it may also be possible to gain USB OTG support doing something like that (e.g., attach a keyboard via a USB OTG converter). E.g., here's a custom kernel that does that: https://github.com/hjr/XCSoar-Kobo-Build There's a valid reason for not including support out of the box since most USB devices wouldn't be able to get enough power, but I still find it slightly disappointing that they went through the effort of disabling it. Especially since you could always use a powered USB hub to provide the power.

But anyway, that's a level of hacking that's well beyond running KOReader. It's dangerous, hard, and unnecessary. At least on a device like my H2O. Some other devices can be taken apart much easier, so you could just swap the internal SD card for some harmless experimentation.

@Biep

This comment has been minimized.

Copy link

commented Apr 20, 2019

Thanks for the comprehensive answer!

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

Note that you can set all three variables behind the FL handling via ntx_io ioctls, but, as Frenzie said, there's a chance the 0-100 mappings are made this way because going below what 1 is currently mapped to would yield undesirable behavior.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

(Theoretical answer, I've never actually tried said ioctls, and the kernel sources sometimes don't match what's actually currently available/working/safe in current production kernels).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

c.f., CM_FRONT_LIGHT_DUTY, CM_FRONT_LIGHT_FREQUENCY, CM_FRONT_LIGHT_R_EN @ arch/arm/mach-mx5/mx50_ntx_io.c (or ntx_io.c in a similar-ish path on newer kernels).

The current mapping tables are in there, and I think you can get the ID of the one used by your device from ntx_hwconfig.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

That seems to indicate that what's mapped to 1 isn't necessarily the lowest it can go. (Based on a quick peek here because I don't want to bug my poor laptop with giant kernel sources.)

I did a quick search for the kind of program I would write to test and it already lives here. (Of course it'd be a bit harder on devices with three of 'em.)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

That topic also has a hardware hack: https://www.mobileread.com/forums/showpost.php?p=2504565&postcount=71

My H2O isn't nearly that moddable though, plus I think the lowest brightness is about just right. Is it brighter on the Forma?

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

I don't recall, but I'm fairly sure those ioctls are gone on the Forma (IIRC, they're at least empty in the sources), only 241 is working.

(I posted something in the Nightmode/FL PR, too, that I used to test the ramp-up/ramp-town).

EDIT: Yup. Gonzo.

#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE //[
		case CM_FRONT_LIGHT_SET:
			{
#ifdef CONFIG_BACKLIGHT_LM3630A
				extern int ntx_set_fl_percent(int percent);

				ntx_set_fl_percent(p);
#endif
			}
			break;

		case CM_FL_LM3630_SET:
			break;
		case CM_FL_LM3630_TABLE:
			break;

		case CM_FL_HT68F20_GETDUTY:
			break;

		case CM_FL_HT68F20_SETDUTY:
			break;

		case CM_FRONT_LIGHT_AVAILABLE:
			{
			 	i = (unsigned long) (gptHWCFG->m_val.bFrontLight?1:0) ;
				dwChk = copy_to_user((void __user *)arg, &i, sizeof(unsigned long));
			}
			break;

		case CM_FL_MSP430_R_DUTY:
			break;

		case CM_FL_MSP430_W_DUTY:
			break;

		case CM_FL_MSP430_FREQUENCY:
			break;

		case CM_FRONT_LIGHT_R_EN:
			break;
		case CM_FL_MSP430_W_H_EN:
			break;
		case CM_FL_MSP430_PWM_EN:
			break;
#endif //]CONFIG_BACKLIGHT_CLASS_DEVICE 
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.