Hey @floriansnow I have a slightly different use-case for the boot logo: show the last displayed page so that one can start reading a few seconds after pressing the power button instead of waiting 15 seconds for KOReader to start and load the book. I opened #15124 — may I ask you to clarify whether there's a conflict with what's implemented by this PR, or if this PR is just a fix for newer PocketBook firmwares and doesn't interact with the "Current Page" boot logo setting at all?

@liskin For full disclosure: I'm by no means an expert on KOReader nor on Lua; I just dug around the code to find what I needed. That being said, I don't have your use case, but this shouldn't affect it at all from what I can tell. My change affects what calls KOReader makes when setting a boot logo and it determines the size differently. My change does not touch what gets set in the first place. The settings inside the PocketBook firmware is yet another thing.

@liskin For full disclosure: I'm by no means an expert on KOReader nor on Lua; I just dug around the code to find what I needed. That being said, I don't have your use case, but this shouldn't affect it at all from what I can tell. My change affects what calls KOReader makes when setting a boot logo and it determines the size differently. My change does not touch what gets set in the first place. The settings inside the PocketBook firmware is yet another thing.

Hm, well, that didn't tell me much. 🙁 So I went and did my own research (on my InkPad 3). It's bad news all over.

It seems that regardless of what's set in the system settings (Personalize → Logos), both WriteStartupLogo and PageSnapshot overwrite the boot logo. That means your PR sets the boot logo to the book cover regardless of user preference. So that's not good and we'll need to make this optional somehow.

Additionally, it seems that the sync invocation takes several seconds, so this PR slows down any rotation changes considerably (up to about 10 seconds if the format is color bmp). Are you sure the sync is necessary? Is that perhaps just a remnant of your previous attempts to fix the boot logo problem before you discovered that WriteStartupLogo is necessary?

(I think I need to file 2 separate issue reports for this as both of these problems are seriously affecting the latest KOReader release on PocketBook devices.)

Perhaps I'm missing something, but this whole behavior is in a plugin for setting a cover image. So of course, this behavior overrules what the PocketBook firmware itself does. I mean the PocketBook firmware also doesn't necessarily know what books KOReader opens, so the two are separate. AFAICT, the behavior is intentional.

However, this is all optional: From how I understand your use case, you don't want to set a cover image, so you simply don't need this plugin.

In regards to the sync: I tested this on two different PocketBook devices and noticed no slowdown. The iv2sh is slow which is why it runs in the background. If I leave out either one, the boot logo does not get set. I don't understand what it has to do with rotation changes.

Digging through your contributions, I figured out that you use https://github.com/ckilb/pocketbooksync.koplugin to sync progress to the PocketBook firmware and also to get the current page as a boot logo. Now I understand why the PocketBook firmware setting and the KOReader behavior are linked to you.

From what I can tell, you benefited from the bug that bothered me: KOReader was unable to properly set the boot logo so your other plugin worked as you want it. Now, the boot logo gets set properly and that overwrites what the other plugin does. What I said before about disabling the Cover Image plugin is still what I think your use case needs.

Perhaps I'm missing something, but this whole behavior is in a plugin for setting a cover image. So of course, this behavior overrules what the PocketBook firmware itself does. I mean the PocketBook firmware also doesn't necessarily know what books KOReader opens, so the two are separate. AFAICT, the behavior is intentional.

However, this is all optional: From how I understand your use case, you don't want to set a cover image, so you simply don't need this plugin.

What you're missing is that both your and my use-cases are just one of many. The cover plugin worked perfectly fine for setting the power-off logo, and then the boot logo could be set to something else. Perhaps the last page (my use-case), perhaps the PocketBook logo, perhaps some other image.

With your changes, the cover plugin always sets the boot logo. If someone wants to only have the book cover as the power-off logo, they're stuck — your suggestion of disabling the cover plugin conflicts with needing to enable that plugin to generate the power-off logo.

Also, the following code suggests that the power-off logo use-case is the primary one:

Thus I believe the "fix" will need to be a bit more elaborate than this PR. We need some way to tell whether the user wishes to use the cover for the power-off logo, the boot logo, or both. Some ideas:

• Use GetGlobalConfig and ReadString to read PocketBook system settings
• Add an option toggle to KOReader (⚙ → Screen → Cover image → something) to enable/disable WriteStartupLogo

In regards to the sync: I tested this on two different PocketBook devices and noticed no slowdown. The iv2sh is slow which is why it runs in the background. If I leave out either one, the boot logo does not get set. I don't understand what it has to do with rotation changes.

Interesting! What devices? Would you perhaps be able to upload libinkview.so and iv2sh from those devices somewhere so I can decompile it and look at what it does? On InkPad 3 firmware 6.8, WriteStartupLogo is fast (like 1 second), but the sync takes several seconds. Still, isn't it really weird that the sync is needed? I don't understand how that could affect anything. WriteStartupLogo reads the bitmap, writes it to a tmp file in memory (/tmp is tmpfs) and then sends an IPC request to set that as a boot logo. Invoking or not invoking sync can't possibly change the contents of that in memory tmp file…

Anyway, rotation: regardless of whether "Rotate image" is enabled in the cover plugin settings, every rotation change triggers the cover update, which generates the image (this might be cached), saves it to the cover file (always, so even if cached, this takes like half a second), then triggers updatePocketBookBootLogo() which does sync synchronously and only the iv2sh bit is done in the background. As a result, rotation now takes like 5 seconds in the latest KOReader release on InkPad 3.

The solution here is to either:

• remove sync if it's not needed (which is something I still believe might be possible because I've read the decompiled code and can't see a reason why it would be necessary)
• os.execute("{ sync; iv2sh WriteStartupLogo " .. util.shell_escape({self.cover_image_path}) .. "; } &") instead of two os.executes if the sync truly is needed

Btw, some extra info I discovered while debugging — the reason why the power-off logo works fine but the boot logo needed your fix is this:

When PocketBook shuts down, it executes the hw_show_poweroff_logo function, which reads the global config file, looks at the offlogo setting, and displays the logo on the screen. It's simple stupid, and happens to be a bit racy — if a button is pressed after the logo is displayed but before the device actually shuts down, something else might end up on the screen, like the home screen for example (and stay there until the device is powered on again).

The boot logo, however, is handled differently. My guess is that the IPC req mentioned previously uploads the boot logo from the tmp file to nvram so that it can be displayed early in the boot process. The hw_show_poweroff_logo function only sets the boot logo if the bootlogo setting equals "@random_logo". All the other options (PocketBook logo, Book Cover, Current Page, …) are expected to be handled via WriteStartupLogo. So that's why we needed your fix.

(But that's all just extra context so that others don't need to rediscover it. Doesn't change anything I said in the previous comment.)

Thanks for the deep dive into the firmware internals! That's really helpful context and I appreciate you documenting it for future contributors.

I want to step back a bit because I think we may have been talking past each other, and I don't want to come across as defensive about this PR. As I mentioned at the start, I'm not an expert on KOReader or Lua. I found a problem, dug around the code, and submitted what I thought was a straightforward fix. So I want to be upfront that I may be wrong about some of my assumptions.

My interpretation of the cover image plugin is that it's meant to set the book cover as the device's logo wherever possible, and that it simply couldn't set the boot logo before because the WriteStartupLogo call was missing. Your firmware analysis actually confirms that the boot logo needs to be uploaded to nvram. So from my perspective this was completing existing functionality, not adding new scope.

That said, I can see your point that users might want the cover as their power-off logo without also setting the boot logo. I didn't consider that use case, partly because I had a hard time understanding your scenario until I realized you were using the pocketbooksync plugin. But you're right that it's not just about that plugin; the PocketBook firmware itself offers independent settings for boot logo and power-off logo, so users could have reasons to want them set differently regardless of what plugins they're running.

I think this might be a question for the maintainers: what is the intended scope of the cover image plugin on PocketBook? Should it set both logos, only the power-off logo, or should it be user-configurable? I don't think the code as it existed before my PR gives a clear answer either way, and I'd rather not guess.

On the sync: my devices are a PocketBook Verse Pro Color and an InkPad Color 3, both on relatively recent firmware. Without the sync, the boot logo simply doesn't get set on either device. I agree it's weird given what you found in the decompiled code, but that was my experience. It's possible there's some additional weirdness at play, but I didn't mess with my devices in any meaningful way besides installing KOReader. I'm not sure about the legal implications of uploading parts of the firmware binaries, so I'd rather not do that without checking first, but I'm happy to run tests on my devices if that helps narrow things down.

Regarding the rotation slowdown: I hadn't considered that code path, and I can see how a synchronous sync would cause problems there. Your suggestion of combining both into a single backgrounded command makes sense to me as an immediate improvement regardless of how the broader design question is resolved. I think I remember that not working for the use case of this PR, though, but perhaps I'm misremembering.

I'm happy to work on a follow-up PR once there's some consensus on the direction, or to help test if someone else wants to take the lead on this.

I think this might be a question for the maintainers: what is the intended scope of the cover image plugin on PocketBook? Should it set both logos, only the power-off logo, or should it be user-configurable? I don't think the code as it existed before my PR gives a clear answer either way, and I'd rather not guess.

I think I took this PR as saying that the functionality already existed but that it happened to be broken on some devices. I'd probably just make it a checkbox to also set the boot logo or something?

Okay, glad to hear we're on the same page now. I took a stab at implementing the reading of PocketBook's settings and acting accordingly — both in pocketbooksync plugin (ckilb/pocketbooksync.koplugin@01b66a9) and also as a userpatch for the cover plugin:

local Device = require("device")
local ffi = require("ffi")
local inkview = ffi.load("inkview")
local lfs = require("libs/libkoreader-lfs")
local logger = require("logger")
local userpatch = require("userpatch")
local util = require("util")

ffi.cdef[[
struct iconfig_s * GetGlobalConfig();
const char *ReadString(struct iconfig_s *cfg, const char *name, const char *deflt);
]]

userpatch.registerPatchPluginFunc("coverimage", function(CoverImage)
    function CoverImage:updatePocketBookBootLogo()
        if Device.isPocketBook() then
            -- enable PocketBook's Personalize → Logos → Power-off Logo → Book Cover
            os.remove("/mnt/ext1/system/logo/bookcover")
            lfs.link(self.cover_image_path, "/mnt/ext1/system/logo/bookcover", true)

            -- enable PocketBook's Personalize → Logos → Boot Logo → Book Cover
            local bootlogo = ffi.string(inkview.ReadString(inkview.GetGlobalConfig(), "bootlogo", "@default_boot_logo"))
            if bootlogo == "@cover_logo" then
                logger.dbg("CoverImage: updating PocketBook boot logo from", self.cover_image_path)
                -- run iv2sh in the background to avoid blocking KOReader
                os.execute("iv2sh WriteStartupLogo " .. util.shell_escape({self.cover_image_path}) .. " &")
            end
        end
    end

    -- if Rotate image is disabled, we don't need to bother recreating the cover image
    -- (this speeds up rotation changes by about half a second on PocketBook InkPad 3)
    function CoverImage:onSetRotationMode(rotation)
        logger.dbg("CoverImage: onSetRotationMode", rotation)
        if self.cover_image_rotate then
            self:createCoverImage(self.ui.doc_settings)
        end
    end
end)

(place into patches/2-pocketbook-cover.lua)

With these two changes in place, everything works perfectly on my InkPad 3:

• power-off logo can be set to "Book Cover" and it does the right thing (doesn't need to be set to the custom "cover.bmp" logo as was the case until now)
• power-off logo can be set to anything else and that also works (obviously)
• boot logo can be set to "Book Cover" and the sync-less iv2sh WriteStartupLogo call is enough to make that work
• boot logo can be set to "Current Page" and the pocketbooksync plugin then calls PageSnapshot on suspend and thus one may start reading almost immediately after powering the reader on (that's what FR: current page as boot logo on PocketBook devices #15124 is about)
• if boot logo is set to anything else, we don't set it at all and thus the system setting is respected
• additionally, if "Rotate image" is disabled in the cover plugin settings, we don't bother recreating it on rotation changes, making those changes near instant instead of taking over half a second

Would you perhaps be willing to test the patch? No need to bother with the pocketbooksync plugin — you just won't be able to use Boot Logo → Current Page. Although… that's rather trivial to add to the patch as well — let me know if you're interested 🙂

I'm not sure about the legal implications of uploading parts of the firmware binaries, so I'd rather not do that without checking first, but I'm happy to run tests on my devices if that helps narrow things down.

Fair enough. I can download the firmware packages and extract the files from there if need be, although it's a rather complicated procedure that I last did like 2 years ago. It's entirely possible that different firmware versions really do this stuff differently. Looking at the release notes for the 6.10 firmware for InkPad Color 3, it seems the logo personalisation is a new feature, whereas the InkPad 3 has had it basically forever…

I think this might be a question for the maintainers: what is the intended scope of the cover image plugin on PocketBook? Should it set both logos, only the power-off logo, or should it be user-configurable? I don't think the code as it existed before my PR gives a clear answer either way, and I'd rather not guess.

I think I took this PR as saying that the functionality already existed but that it happened to be broken on some devices. I'd probably just make it a checkbox to also set the boot logo or something?

I don't mind guessing as much as as Florian, and my guess is that the functionality didn't exist. One could try to manually set the cover image path to /mnt/ext1/system/logo/bootlogo/cover.bmp, and then set that as the Boot Logo in the device settings, but I suspect the logo wouldn't get updated when the file changed.

There was some discussion of power-off and boot logos in #6597 (look for "decal"). Some of those comments suggest that setting bootlogo was implemented, but I can't find it anywhere in the git history. Perhaps those comments actually suggest that it all happens automatically (in old firmwares where there were no settings for the logos)?

Anyway, yeah, I can now see why guessing is silly 😆

Wow, that was quick! I went to sleep and now you prepared a patch! Thank you very much! I am absolutely up for testing the patch.

Perhaps we should continue this discussion to a PR for your changes or an issue?

Perhaps we should continue this discussion to a PR for your changes or an issue?

• coverimage update (on every rotation!) slow (~5 s) on PocketBook #15177
• coverimage sets PocketBook boot logo unconditionally, ignoring device settings #15178

I will also open a quick PR to not update the cover image on rotation changes if "Rotate image" is disabled