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

Initial Kindle PW5 support #8856

Merged
merged 67 commits into from
Mar 14, 2022
Merged

Initial Kindle PW5 support #8856

merged 67 commits into from
Mar 14, 2022

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Feb 27, 2022

Requires koreader/koreader-base#1457

(Oh, hello there, "mystery" device ;p).


  • Rejig frontlight warmth API to more closely match the existing API, and, hopefully, clarify some of its quirks, and reduce boilerplate and duplicate code in platform implementations.
  • Tweak Kindle:setDateTime to prefer using the platform's custom script, as in interacts better with the stock UI. And make the fallbacks handle old busybox versions better.
  • Add Kindle PW5 support ;).
  • Add warmth support to the Kindle platform.
  • Random TextBoxWidget cleanups: make sure we immediately free destroyed instances.
  • FrontLightWidget: Refactor to make it slightly less obnoxious to grok and update; i.e., separate layout from update, and properly separate brightness from warmth handling. Move to simpler widgets instead of reinventing the wheel.
  • TextBoxWidgets: Implement setText to match TextWidget's API, as some callers may be using the two interchangeably (i.e., Button).
  • NaturalLightWidget: Make sure we pass a string to InputText
  • InputText: Add debug guards to catch bad callers not passing strings ;).

This change is Reviewable

@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 27, 2022

Pending a minor refactor of the FL widget because that thing's a mess.

Stuff requiring further investigation:

  • Driver really likes optimizing DU updates out, so flashing_ui barely flashes ^^.
  • Exiting KOReader currently exhibits symptoms similar to what Kindle: don't kill kb service on start #8122 fixed for earlier FW versions (namely, long UI stalls). Haven't had time to look into it yet, and low priority as rebooting works. (It's still a Kindle, though, so, rebooting still takes an assload of time).

@Frenzie Frenzie added this to the 2022.03 milestone Feb 27, 2022
@Frenzie Frenzie added the Kindle label Feb 27, 2022
@yparitcher
Copy link
Member

(Oh, hello there, "mystery" device ;p).

Did you find an exploit to jailbreak, or was entry via serial? :)

@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 28, 2022

I'm not the exploit finder, and it won't be published until Amazon manages to ship a patched FW, because responsible disclosure and all that ;).

But, no, it's a good old chain of sneaky *nix trickery, quite fun :).

AFAIK, serial is a no-go on recent devices, secure boot and all that jazz :/.

@yparitcher
Copy link
Member

yparitcher commented Feb 28, 2022 via email

@Frenzie
Copy link
Member

Frenzie commented Feb 28, 2022

Disagree, 8 GB storage with no (micro)-SD is grossly underspecced and nigh unusable.

Edit: to be clear, 32 GB on the Kobo Libra 2 is also an affront. But, you know, it's not 8 GB.

@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 28, 2022

The Signature has 32GB storage, IIRC (it's wildly too expensive, though ;)).

Comment on lines 1172 to 1195
function TextBoxWidget:setText(text)
if text == self.text then
return
end

self.text = text

if self.use_xtext then
self:_measureWithXText()
else
self:_evalCharWidthList()
end
self:_splitToLines()

self:update()
end
dbg:guard(TextBoxWidget, "setText",
function(self, text)
assert(type(text) == "string",
"Wrong text type (expected string)")
end)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this sneaky stuff? :-P

Copy link
Member Author

@NiLuJe NiLuJe Mar 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To match TextWidget's API, because Button might call it since the multi-line support pr ;).

(Also, I needed it too ;p).

Comment on lines 1172 to 1184
function TextBoxWidget:setText(text)
if text == self.text then
return
end

self.text = text

if self.use_xtext then
self:_measureWithXText()
else
self:_evalCharWidthList()
end
self:_splitToLines()

self:update()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I would trust this. Seems a few more things may be done in :init() like dealing with height, that doing this quick fake-init() might mess ?

@yparitcher
Copy link
Member

Disagree, 8 GB storage with no (micro)-SD is grossly underspecced and nigh unusable.

Edit: to be clear, 32 GB on the Kobo Libra 2 is also an affront. But, you know, it's not 8 GB.

To me the most important thing is to fit in my pocket :)
Otherwise it is is not portable,
so a bigger screen while still fitting in my pocket, kinda outweighs storage concerns.
no one else make a pocketable > 6" screen

@Frenzie
Copy link
Member

Frenzie commented Mar 6, 2022 via email

Comment on lines 155 to 160
self:_computeDimensions()
self:_updateLayout()
if self.editable then
self:moveCursorToCharPos(self.charpos or 1)
end
self.dimen = Geom:new(self:getSize())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit unconfortable with these kind of changes. It might be all right, but after a few minutes looking around, I can't say I'm 100% sure - it's less readable that how it was before (_computeDimensions actually renders the text, so this naming is hiding abit the whole core of what's really happening).
And what about this self.dimen here that won't get updated by your setText() if you give it larger text that makes a few more lines ?

I'd be more confortable if you just did in your setText() : free() and then :init(). That's what we do in InputText:initTextBox() - it would feel less risky, and the wee additional recomputing this would do vs. your splitting would be bearable for the rare users of this new setText().

This comment was marked as duplicate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I like catching design/implementation bugs with stuff like that ;o).

@NiLuJe
Copy link
Member Author

NiLuJe commented Mar 7, 2022

Note to self: Mostly done testing this on the KOReader side (as in, haven't looked into the driver merging behavior nor the stock UI hangs).

Note to self: check if those aren't simply wait_for_any timeouts.

Note to self: and/or weird interaction with the new React UI, because there be lotsa new services I know nothing about running.

So, just have to check that the warmth API tweaks didn't horribly break on Kobo ;o).

@@ -151,6 +152,26 @@ function TextBoxWidget:init()
self.text_height = self.lines_per_page * self.line_height_px
end

self:_computeTextDimensions()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, we're doing more than "computing text dimensions": we render the text...

And now that you're not even using this TextBoxWidget in FrontlightWidget, it feels unnecessary to hack this module and leave in there some code paths we're not even taking.

Copy link
Member Author

@NiLuJe NiLuJe Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's kind of implied by the verb (same as _measureText) ;).

There's nothing really left of concern in init vs this, AFAICT.

And Button can take this path, so I'll take something that appeared to work just fine over a crash ;).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing really left of concern in init vs this

Except readability (not having to jump around to see what's happening) and messing my git history :)

And Button can take this path, so I'll take something that appeared to work just fine over a crash

Which you could have solved with a dumber setText() doing :free() + :init().

@NiLuJe
Copy link
Member Author

NiLuJe commented Mar 8, 2022

Note to self: check if those aren't simply wait_for_any timeouts.

Huh. Good hunch ;).

[pid  2089] 18:42:05.604305 [b69d78dc] ioctl(19, MXCFB_SET_PREFETCH or MXCFB_SET_PWRDOWN_DELAY, 0) = 0 <0.000210>
[pid  2174] 18:42:05.615880 [b69d78dc] ioctl(19, MXCFB_SEND_UPDATE, {update_region={top=1492, left=567, width=1, height=1}, waveform_mode=WAVEFORM_MODE_AUTO => WAVEFORM_MODE_AUTO, update_mode=UPDATE_MODE_PARTIAL, update_marker=320, temp=TEMP_USE_AMBIENT, flags=0, dither_mode=EPDC_FLAG_USE_DITHERING_PASSTHROUGH, quant_bit=0, alt_buffer_data={phys_addr=NULL, width=0, height=0, alt_update_region={top=0, left=0, width=0, height=0}}, swipe_data={direction=MTK_SWIPE_DOWN, steps=0}, hist_bw_waveform_mode=MTK_WAVEFORM_MODE_DU, hist_gray_waveform_mode=MTK_WAVEFORM_MODE_GC16, ts_pxp=0, ts_epdc=0}) = 0 <0.001632>
[pid  2177] 18:42:05.618518 [b69d78dc] ioctl(19, MXCFB_WAIT_FOR_ANY_UPDATE_COMPLETE, {flag=FLAG_NONE <unfinished ...>
[pid  2174] 18:42:05.659475 [b69d78dc] ioctl(19, MXCFB_SEND_UPDATE, {update_region={top=1492, left=567, width=1, height=1}, waveform_mode=WAVEFORM_MODE_AUTO => WAVEFORM_MODE_AUTO, update_mode=UPDATE_MODE_PARTIAL, update_marker=321, temp=TEMP_USE_AMBIENT, flags=0, dither_mode=EPDC_FLAG_USE_DITHERING_PASSTHROUGH, quant_bit=0, alt_buffer_data={phys_addr=NULL, width=0, height=0, alt_update_region={top=0, left=0, width=0, height=0}}, swipe_data={direction=MTK_SWIPE_DOWN, steps=0}, hist_bw_waveform_mode=MTK_WAVEFORM_MODE_DU, hist_gray_waveform_mode=MTK_WAVEFORM_MODE_GC16, ts_pxp=0, ts_epdc=0}) = 0 <0.006690>
[pid  2089] 18:42:05.698332 [b69d78dc] ioctl(19, MXCFB_WAIT_FOR_UPDATE_SUBMISSION, 321) = 0 <0.000480>
[pid  2089] 18:42:07.046736 [b69d78dc] ioctl(19, MXCFB_WAIT_FOR_UPDATE_COMPLETE, {update_marker=321, collision_test=3064201216 => 3064201216}) = 0 <0.000073>
[pid  2089] 18:42:07.047140 [b69d78dc] ioctl(19, MXCFB_SEND_UPDATE, {update_region={top=0, left=0, width=1236, height=1648}, waveform_mode=MTK_WAVEFORM_MODE_GC16 => MTK_WAVEFORM_MODE_GC16, update_mode=UPDATE_MODE_FULL, update_marker=322, temp=TEMP_USE_AMBIENT, flags=0, dither_mode=EPDC_FLAG_USE_DITHERING_PASSTHROUGH, quant_bit=0, alt_buffer_data={phys_addr=NULL, width=0, height=0, alt_update_region={top=0, left=0, width=0, height=0}}, swipe_data={direction=MTK_SWIPE_DOWN, steps=0}, hist_bw_waveform_mode=MTK_WAVEFORM_MODE_DU, hist_gray_waveform_mode=MTK_WAVEFORM_MODE_GC16, ts_pxp=0, ts_epdc=0}) = 0 <0.000788>
[pid  2177] 18:42:07.691264 [b69d78dc] <... ioctl resumed> => markers=[]}) = 0 <2.072520>
[pid  2177] 18:42:07.692982 [b69d78dc] ioctl(19, MXCFB_WAIT_FOR_ANY_UPDATE_COMPLETE, {flag=FLAG_NONE => markers=[]}) = 0 <2.078578>

Not quite sure what the best way to handle that will be...

(Especially since that kernel facility seems to be a moving target, it might not behave in the same way on 5.14.2)

NiLuJe added 10 commits March 8, 2022 22:48
(We cannot support USBMS, so doing FM stuff on UsbPlugOut makes no
sense).
It's a custom wrapper that will update the stock UI properly, too.

(In addition to doing the date + hwclock dance)
Handle it via a sysfs poke, like on Cervantes.
Always honor the native scale, and dislpay it as-is
@NiLuJe
Copy link
Member Author

NiLuJe commented Mar 8, 2022

Note to self: check if those aren't simply wait_for_any timeouts.

Huh. Good hunch ;).

[pid  2089] 18:42:05.604305 [b69d78dc] ioctl(19, MXCFB_SET_PREFETCH or MXCFB_SET_PWRDOWN_DELAY, 0) = 0 <0.000210>
[pid  2174] 18:42:05.615880 [b69d78dc] ioctl(19, MXCFB_SEND_UPDATE, {update_region={top=1492, left=567, width=1, height=1}, waveform_mode=WAVEFORM_MODE_AUTO => WAVEFORM_MODE_AUTO, update_mode=UPDATE_MODE_PARTIAL, update_marker=320, temp=TEMP_USE_AMBIENT, flags=0, dither_mode=EPDC_FLAG_USE_DITHERING_PASSTHROUGH, quant_bit=0, alt_buffer_data={phys_addr=NULL, width=0, height=0, alt_update_region={top=0, left=0, width=0, height=0}}, swipe_data={direction=MTK_SWIPE_DOWN, steps=0}, hist_bw_waveform_mode=MTK_WAVEFORM_MODE_DU, hist_gray_waveform_mode=MTK_WAVEFORM_MODE_GC16, ts_pxp=0, ts_epdc=0}) = 0 <0.001632>
[pid  2177] 18:42:05.618518 [b69d78dc] ioctl(19, MXCFB_WAIT_FOR_ANY_UPDATE_COMPLETE, {flag=FLAG_NONE <unfinished ...>
[pid  2174] 18:42:05.659475 [b69d78dc] ioctl(19, MXCFB_SEND_UPDATE, {update_region={top=1492, left=567, width=1, height=1}, waveform_mode=WAVEFORM_MODE_AUTO => WAVEFORM_MODE_AUTO, update_mode=UPDATE_MODE_PARTIAL, update_marker=321, temp=TEMP_USE_AMBIENT, flags=0, dither_mode=EPDC_FLAG_USE_DITHERING_PASSTHROUGH, quant_bit=0, alt_buffer_data={phys_addr=NULL, width=0, height=0, alt_update_region={top=0, left=0, width=0, height=0}}, swipe_data={direction=MTK_SWIPE_DOWN, steps=0}, hist_bw_waveform_mode=MTK_WAVEFORM_MODE_DU, hist_gray_waveform_mode=MTK_WAVEFORM_MODE_GC16, ts_pxp=0, ts_epdc=0}) = 0 <0.006690>
[pid  2089] 18:42:05.698332 [b69d78dc] ioctl(19, MXCFB_WAIT_FOR_UPDATE_SUBMISSION, 321) = 0 <0.000480>
[pid  2089] 18:42:07.046736 [b69d78dc] ioctl(19, MXCFB_WAIT_FOR_UPDATE_COMPLETE, {update_marker=321, collision_test=3064201216 => 3064201216}) = 0 <0.000073>
[pid  2089] 18:42:07.047140 [b69d78dc] ioctl(19, MXCFB_SEND_UPDATE, {update_region={top=0, left=0, width=1236, height=1648}, waveform_mode=MTK_WAVEFORM_MODE_GC16 => MTK_WAVEFORM_MODE_GC16, update_mode=UPDATE_MODE_FULL, update_marker=322, temp=TEMP_USE_AMBIENT, flags=0, dither_mode=EPDC_FLAG_USE_DITHERING_PASSTHROUGH, quant_bit=0, alt_buffer_data={phys_addr=NULL, width=0, height=0, alt_update_region={top=0, left=0, width=0, height=0}}, swipe_data={direction=MTK_SWIPE_DOWN, steps=0}, hist_bw_waveform_mode=MTK_WAVEFORM_MODE_DU, hist_gray_waveform_mode=MTK_WAVEFORM_MODE_GC16, ts_pxp=0, ts_epdc=0}) = 0 <0.000788>
[pid  2177] 18:42:07.691264 [b69d78dc] <... ioctl resumed> => markers=[]}) = 0 <2.072520>
[pid  2177] 18:42:07.692982 [b69d78dc] ioctl(19, MXCFB_WAIT_FOR_ANY_UPDATE_COMPLETE, {flag=FLAG_NONE => markers=[]}) = 0 <2.078578>

Not quite sure what the best way to handle that will be...

(Especially since that kernel facility seems to be a moving target, it might not behave in the same way on 5.14.2)

Whelp, not forgetting to disable the "fast mode" thingy on exit appears to have helped, yay.

setWarmth / self.fl_warmth are "public" (-ish, as far as the second one
is concerned), and always in the [0, 100] range.

setWarmthHW is private, local to each implementation, and always uses
the platform's native scale.
Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 16 files at r1, 5 of 8 files at r2, 3 of 4 files at r4, 1 of 1 files at r6, 8 of 8 files at r7, 3 of 3 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Frenzie, @NiLuJe, @poire-z, and @yparitcher)


frontend/device/kindle/device.lua, line 64 at r6 (raw file):

Previously, yparitcher wrote…

This should probably be named isWifiOn or isWifiUp as it only checks if the wireless interface is up not if it is connected.

Done.

@NiLuJe NiLuJe marked this pull request as ready for review March 8, 2022 22:55
@NiLuJe NiLuJe requested a review from pazos as a code owner March 8, 2022 22:55
@NiLuJe
Copy link
Member Author

NiLuJe commented Mar 8, 2022

Whelp, finally done with this :).

@zwim
Copy link
Contributor

zwim commented Mar 12, 2022

One question: What does the now not so mysterious device show on cat /sys/power/state?

@NiLuJe
Copy link
Member Author

NiLuJe commented Mar 12, 2022

It might not be directly useful, for a couple reasons:

  • We leave power management to the stock system on Kindle.
  • Said daemon implements a two-tiered sleep state tracker, and on most recent devices, the deeper sleep state involves a custom-ish suspend to disk on a custom bit of storage hardware (or... something, never really looked into it).

That said, I'll give it a poke the next time it's plugged ;).

(What I can say is that it's hilariously more power efficient, even completely awake: a Sage/Elipsa in suspend drains more power than an idle PW5 at reasonable light levels (!). Computational spikes also drain way, way less power).

@yparitcher
Copy link
Member

Just got my PW5 (5.14.0.1) It (just) fits my pocket :) waiting for the JB to try this wink wink ;)

InputText: Add debug guards to catch bad callers not passing strings ;).

Fix koreader#8899
@NiLuJe NiLuJe merged commit 217a73f into koreader:master Mar 14, 2022
uroybd pushed a commit to uroybd/koreader that referenced this pull request Mar 16, 2022
* Rejig frontlight warmth API to more closely match the existing API, and, hopefully, clarify some of its quirks, and reduce boilerplate and duplicate code in platform implementations.
* Tweak Kindle:setDateTime to prefer using the platform's custom script, as in interacts better with the stock UI. And make the fallbacks handle old busybox versions better.
* Add Kindle PW5 support ;).
* Add warmth support to the Kindle platform.
* Random TextBoxWidget cleanups: make sure we immediately free destroyed instances.
* FrontLightWidget: Refactor to make it slightly less obnoxious to grok and update; i.e., separate layout from update, and properly separate brightness from warmth handling. Move to simpler widgets instead of reinventing the wheel.
* TextBoxWidgets: Implement `setText` to match TextWidget's API, as some callers may be using the two interchangeably (i.e., Button).
* NaturalLightWidget: Make sure we pass a string to InputText
* InputText: Add debug guards to catch bad callers not passing strings ;).
@octavianx octavianx mentioned this pull request Mar 20, 2022
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Mar 21, 2022
Frenzie pushed a commit that referenced this pull request Mar 22, 2022
Fix #8913
Regression since #8856

Change tacks, to allow full granularity with the +/- buttons.
@@ -104,6 +126,7 @@ local Kindle = Generic:new{
-- NOTE: We can cheat by adding a platform-specific entry here, because the only code that will check for this is here.
isSpecialOffers = isSpecialOffers(),
hasOTAUpdates = yes,
hasFastWifiStatusQuery = yes,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good morning, thank you for that especially.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, good to see you (alive) ! How are you ?
(And where were/are you living? We all got very faimiliar with Ukraine geography lately :/)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, so-so, still migrating. Hope to appear here more frequently if I can.

@yparitcher yparitcher mentioned this pull request Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants