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

Better Forma handling (fix #4291) #4414

Merged
merged 38 commits into from Dec 28, 2018

Conversation

Projects
None yet
4 participants
@NiLuJe
Copy link
Member

NiLuJe commented Dec 22, 2018

  • Enforce Portrait on startup, to avoid getting touch coordinates wrong if we were started from Nickel in inverted Portrait.
  • Fix button mapping, and made it behave properly in landscape. I may have massively ruined non-Portrait mappings for everyone else, but, hey, this made sense, and it works here ;p.
  • Fix NaturalLight handling, thanks to @cairnsh & @pazos in #4291 ;)
@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 22, 2018

The only mildly annoying thing left is the FrontLight widget: it expects a scale of 0 to 100 for the warmth, while this device only actually handles 0 to 10, so this leaves a lot of "dead" steps.

UI work is really not my thing, so I'm leaving that as an exercise to the reader for now.
(The fact that I personally barely ever move from a single warmth step doesn't help).

NiLuJe added some commits Dec 22, 2018

Make that more generic.
The search screen supports landscape, and we can get started from there
;).
@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 22, 2018

Waiting on BASE#780 for that wonderful Mk7 detection fail! ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 22, 2018

So, better add that to the list of changes, too:

  • Makes screen handling vastly superior on the Clara & Forma ;).

(As well as those odd ducks, the AuraSEr2 & H2O²r2).

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Dec 22, 2018

Completely off topic, but would you mind taking a quick peek on Gitter? I don't know the Kindle details. ;-)

@Frenzie
Copy link
Member

Frenzie left a comment

Code looks good, and fingers crossed that the device stuff is as well. :-D

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 22, 2018

Random remark about the button remapping on rotation:

I just checked on old Kindles with buttons (K3 & K4), and on landscape, they do NOT remap the page turn buttons, which is stupid.
But I assume this is why we didn't either ;).

The current status of the code seems to come from the Oasis days, and those may do funky stuff with button keycodes depending on orientation, and/or having the buttons bound in the reverse to begin with.

So, yeah, this makes sense on the Forma & on non-touch Kindles, but it may feel weird on the Oasis 1 or 2, or both.

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 22, 2018

I'm also hoping the Nickel setting to invert the buttons is entirely handled on Nickel's side and doesn't actually affect us on the Forma ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 22, 2018

Speaking of UI stuff, being able to switch to inverted rotations would be NTH (especially inverted portrait here), but the ScreenMode stuff seems fairly gnarly, so, err, pass ;p.

(Besides adding more buttons to the bottom menu, I was thinking a tap on the orientation icon would swap the icon to a mirrored version, and the button choices to their Inverted variant, with an immediate toggle to the Inverted variant of the current orientation).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 22, 2018

So, on my end, that's it for now, ready to go :).

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Dec 22, 2018

If you tap the bottom orientation thingy twice then you actually get inverted, don't you?

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 22, 2018

@Frenzie : Doesn't appear to?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Dec 22, 2018

Might be that it was never implemented for portrait, only for landscape.

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 22, 2018

Ah! Indeed, that works for Landscape!

I'll see what I can do...

@pazos
Copy link
Contributor

pazos left a comment

Not sure but being Kobo I would expect that gyro events are registered in nickel hardware status and then wake the device from sleep, like the usb plug/unplug stuff.

if self.frontlight_mixer then
-- And the warmth is now on a scale of 0 to 10 ;).
warmth = math.floor(warmth/10)
self:_write_value(self.frontlight_white, brightness)

This comment has been minimized.

@pazos

pazos Dec 22, 2018

Contributor

can we use self:_set_light_value(self.frontlight_white, brightness) and leave frontlight_white as is?

This comment has been minimized.

@pazos

pazos Dec 22, 2018

Contributor

I mean, writting bl_power is not neccesary but shouldn't hurt.

This comment has been minimized.

@NiLuJe

NiLuJe Dec 22, 2018

Member

I went against that, because bl_power appeared to be set to an insane value when I checked (like -1465879254 or something) ;).

This comment has been minimized.

@NiLuJe

NiLuJe Dec 22, 2018

Member

-1676681792, to be exact ;p.

┌─(ROOT@(none):pts/0)───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────(/)─┐
└─(0.22:58%:22:28:96%:#)── cat /sys/class/backlight/mxc_msp430.0/bl_power                                                                                                                                                                                                                                                                               ──(Sat, Dec 22)─┘
-1676681792
┌─(ROOT@(none):pts/0)───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────(/)─┐
└─(0.20:58%:22:28:96%:#)── cat /sys/class/backlight/tlc5947_bl/bl_power                                                                                                                                                                                                                                                                                 ──(Sat, Dec 22)─┘
0

This comment has been minimized.

@NiLuJe

NiLuJe Dec 22, 2018

Member

Given that, I refrained from trying to write to it ;p.

This comment has been minimized.

@NiLuJe

NiLuJe Dec 22, 2018

Member

In somewhat related weirdness, as I jotted down in device, for the FL, you can pretty much assume brightness is WO (it's pretty much always 0 in nickel), while actual_brightness is RO, and always reflects the actual level (... unless you try weird shit like setting the brightness of the mixer manually).

touch_snow_protocol = true,
display_dpi = 300,
hasNaturalLight = yes,
frontlight_settings = {
frontlight_white = "/sys/class/backlight/mxc_msp430.0",
frontlight_red = "/sys/class/backlight/tlc5947_bl",
frontlight_white = "/sys/class/backlight/mxc_msp430.0/brightness",

This comment has been minimized.

@pazos

pazos Dec 22, 2018

Contributor

see the other comment 👍

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 22, 2018

Okay, I think I got the rotation logic right, and fixed an old bug in the process (maybe?).
Thanks for the hint about the landscape trickery, @Frenzie ;).

This depends on BASE#781.

It's a bit weird, I couldn't get ReaderView:onSetScreenMode to take three args with the last two being optional (as in, able to be nil) (ideally, I'd have gone with new_mode, rotation, interactive).
When I tried that, I then needed to set rotation to nil in ReaderRolling:onChangeScreenMode, but doing so, self.ui:handleEvent(Event:new("SetScreenMode", mode, nil, true)) appeared to drop that true once it got back to ReaderView:onSetScreenMode (but rotation being nil indeed made it through)?!


In the same vein, this probably doesn't handle PDF, because unlike creoptions which uses an intermediate ChangeScreenMode event, koptoptions uses the SetScreenMode event directly, and I couldn't figure out how to pass it an extra argument that isn't part of the options...

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 22, 2018

@pazos: That's an interesting comment about the gyro.... Not familiar with this, what should I check?

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 26, 2018

And I have no idea what got into the testsuite, yay?

Sure, those last few commits ain't pretty, but, still...

NiLuJe added some commits Dec 26, 2018

Okay, unbreak KOpt rota...
Without re-screwing the tests, hopefully...
@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 26, 2018

Oohkay, I'm done! :).

NiLuJe added some commits Dec 26, 2018

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 26, 2018

If a ColorSetting that falls between the 11 actual breakpoints is stored by KOReader, Nickel will round it to the nearest step, so, not much harm done, besides a potential 1-step diff that can/will easily be corrected.

@pazos

This comment has been minimized.

Copy link
Contributor

pazos commented Dec 26, 2018

@NiLuJe: I guess that other devices without red leds will be ok using the new mixer interface (Since the current behaviour leds to slightly modified white brightness when applying a new color). I'm talking about Kobo Clara HD and Aura H2O2, Rev2. In both cases the frontlight.mixer is /sys/class/backlight/lm3630a_led/color

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 26, 2018

Quite possibly, but I don't have any of those to check if the scale is as finicky as it is on this one, and apparently it works as it currently is, so, eh ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 26, 2018

In any case, that reminded me of something I'd forgotten to jot down on my TODO-list, so, at least if someone wants to try, they now easily can since the scale is no longer hard-coded ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 27, 2018

Final request for comment, @Frenzie / @pazos , before I merge this?

@faassen

This comment has been minimized.

Copy link

faassen commented Dec 27, 2018

I'm a random guy who may not know what he's doing with a new kobo forma: I built this branch locally (with koxtoolchain; at least I hope -- it's installed and presumably used when I use './kodev release kobo').

Then I used kfmon to start it. Unfortunately when I try to start koreader it crashes immediately (some message about it exiting unexpextedly flashes by quickly) and then reboots the kobo forma. I tried using the proper pre-built release but that was even worse: the kobo forma crashed and presumably had a broken database as it didn't pick up any books anymore, but that might have just been a case of extra bad luck.

Is there any way I can help in debugging this?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Dec 27, 2018

@NiLuJe
Nothing jumps out at a quick skim.

@faassen

I built this branch locally (with koxtoolchain; at least I hope -- it's installed and presumably used when I use './kodev release kobo').

It would be if it's in PATH, otherwise likely not. It says which compiler it's using. But note that this PR only changes some Lua code, so besides making sure you also update the relevant file in base no compilation is necessary.

I tried using the proper pre-built release but that was even worse: the kobo forma crashed and presumably had a broken database as it didn't pick up any books anymore, but that might have just been a case of extra bad luck.

How did you install and run it?

@faassen

This comment has been minimized.

Copy link

faassen commented Dec 27, 2018

I don't want to hold up this PR and this was definitely in the wrong place to comment, my apologies. It's just I went out of my way to try to build this branch specifically. I'll move over to an issue.

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 27, 2018

As @Frenzie said, there's no compiled code changes, so, no need to build anything ;).

The way I personally do it is start from the latest nightly, then a simple

for file in $(git diff --name-only upstream/master) ; do ; [[ -f "${file}" ]] && scp "${file}" "root@kindle:/mnt/onboard/.adds/koreader/${file}" ; done

In both my koreader & koreader-base branch (with an upstream remote pointing, to, well, upstream, i.e., koreader/...)

(That works for mostly everything, except the stuff in platform/, so in this specific case, I scp'ed that manually).

(And, yes, all my devices are aliased to "kindle" over USBNet, ^^. Note that USBNet is not a requirement, (just an sshd w/ scp support) it's just more convenient for longer sessions ;)).

@faassen

This comment has been minimized.

Copy link

faassen commented Dec 27, 2018

Thanks for the tip! I will try that in the future.

Here's the issue. #4425

I am looking forward to trying out your work on my Forma!

NiLuJe added some commits Dec 27, 2018

Fix insidiously broken USBMS behavior after a restart
Don't ask me why, or how.
AFAICT, this is new, I don't recall FW 4.8 behaving like this.
@pazos

pazos approved these changes Dec 28, 2018

Copy link
Contributor

pazos left a comment

🥇

NiLuJe added some commits Dec 28, 2018

@NiLuJe NiLuJe merged commit f6743a4 into koreader:master Dec 28, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment