Add support for QualcommOnyxEPDController

[Galunid]

koreader/android-luajit-launcher#250 introduces new QualcommOnyxEPDController. This adds proper refresh modes to support it. It's still needs some fine-tuning.

---

This change is 

[Galunid]

Not to self: rename onyx_* to qualcomm_*

[pazos]

Based on these changes the EPD driver is freescale/NTX or somehow compatible. It does makes sense as there's no Qualcomm platform in onyx device abstraction

Please do check that 6 != 7 for onyx_waveform_regal.

Probably 7 is on onyx the same as in other ntx devices, called regald.

[NiLuJe]

Speaking of REAGLD, in plain Linux-land, we actually never use that (except on the extremely shitty special snowflake that is the Kobo Aura), so I doubt anything recent actually ought to use that, unless the meaning was twisted in the Android mangling ;).

[Galunid]

They are similar, but not compatible, onyx_waveform_regal == 6, if I make onyx_waveform_regal ==7, it crashes the EPD driver i.e. screen starts blinking weirdly in ~0.5s intervals and doesn't draw properly, and logcat screams something along the lines of incorrect waveform mode. You're looking at an outdated device tree. The new one has

Dunno what SDM stands for but it's the one responsible for Nova 2.

EDIT:
One mystery solved

[NiLuJe]

Wild guess: SnapDragon + MSP (our usual Linux NTX boards use an MSP430).

[Galunid]

This is how it looks so far. I'm not sure how to "fix" the bug with menu, it seems 2 consecutive updates somehow mess it up and one gets stuck. Getting rid of reagl stuff helped a bit.

[NiLuJe]

Does disabling menu highlights in the e-Ink settings help ("Flash buttons and menu items")?

If it does, that would imply that something is trying to be cute with "fast" updates, which may not be entirely unheard of if those are A2.

[Galunid]

@NiLuJe Nope, it doesn't help, it's pretty similar to "refresh happens before page is changed", I set 100ms delay on full regal, full gc16 and it helped a lot. Unless you change multiple page in quick succession, then it rarely gets stuck. Menu still has this problem though

[pazos]

To avoid if .. else in each of the modes (except for full) I would go with

local function getWaveforms(platform)
    --common for freescale and qualcomm EPD Controllers
    local full, partial = 32, 0
    local wf_du, wf_gc16 = 1, 2
    local partial_du, partial_gc16 = wf_du, wf_gc16

    -- different contants
    local full_gc16, full_regal, partial_regal
    if platform == "freescale" then
        local wf_regal = 7
        full_gc16 = wf_gc16 + full
        full_regal = wf_regal + full
        partial_regal = wf_regal
    elseif platform == "qualcomm" then
        local wf_regal = 6
        local mode_wait = 64
        local mode_reagl = 4096
        full_gc16 = wf_gc16 + full + mode_wait
        full_regal = wf_regal + full
        partial_regal = wf_regal
    end
    return full_gc16, full_regal, partial_gc16, partial_regal, partial_du
end

local full_gc16, full_regal, partial_gc16, partial_regal, partial_du = getWaveforms(eink_platform)

[Galunid]

@pazos Yes, I was thinking of something like this, but the delays are the problem, since freescale generally doesn't need any, and Onyx will need quite a few of them

[pazos]

@pazos Yes, I was thinking of something like this, but the delays are the problem, since freescale generally doesn't need any, and Onyx will need quite a few of them

They are not used except of FullImpl() or do you plan to add them?.

Why?. Are you sure you're disabling the system epd updates?. If the system doesn't update KO should update without any delay, like it is doing now on this PR.

[Galunid]

They are not used except of FullImpl() or do you plan to add them?.

Yes, despite me forgetting [WIP], it's still very much work in progress

Are you sure you're disabling the system epd updates?.

Yes, every system screen update has
I SDM : update_to_display[0/1] -- marker[79] waveform_mode = 255, update_mode = 0, Rect[0 0 1872 1404], flags = 0 entry in adb logcat. I take it that lack of this message means there was no system update.

If the system doesn't update KO should update without any delay, like it is doing now on this PR.

That's what I thought too, but that seems not to be the case, see this video. To me it seems to work like this: update gets requested -> update gets finished -> view state is updated, instead of view state is updated -> update gets requested -> update gets finished. Likely hood of this happening is increased when going multiple pages forward in quick succession. I don't know what causes this

[pazos]

@Galunid: I still don't get it, I'm afraid.

The behaviour until now is:

"Unsupported devices" -> let the system driver to update the screen.
"Supported for full updates" -> let the system driver to update the screen on its own, but add a full flashing refresh each n pages.
"Supported for all updates" -> system driver doesn't work (ie: Tolinos, Crema ..), so we do update the screen all the time.

On devices where we do all the work delays aren't needed (KO writes to the native window, pretty much like we're doing with /dev/fb0 on linux, and just after that it requests an EPD update)

On devices where we handle full updates only the delay makes sense. We don't prevent the system from updating,so we need to give it some time between https://github.com/koreader/koreader-base/blob/master/ffi/framebuffer_android.lua#L141 and the request, because we're usually faster and win the race (so the full epd update happens before the partial update driven by the system)

Having a FULL_EINK_UPDATE device that needs a delay between android.lib.ANativeWindow_unlockAndPost(android.app.window) and one of the framebuffer:refresh implementations seems wrong, because there's nothing to wait for.

[Galunid]

function framebuffer:refreshFullImp(x, y, w, h)
    self:_updateWindow()
    self:_updateFull()
end

Would it be possible that self:_updateFull() finishes execution before android.lib.ANativeWindow_unlockAndPost(android.app.window)? I'm not too familiar with how ffi stuff works, but assuming ffi calls are non blocking, it could be possible?

The delays are there, because sometimes update happens before the "state" is refreshed, what's causing that is beyond me, I wholeheartedly agree, that delays should be unnecessary, but reality just proves me wrong ;)

"Supported for all updates" -> system driver doesn't work (ie: Tolinos, Crema ..), so we do update the screen all the time.

This should be the case here. There are 2 tests I'm going to run:

• what happens when I "prevent" refresh, but don't refresh screen later (it should remain the same, no matter what I do) - confirmed, no system screen updates unless I force one
• what happens when I get rid of "delay" thread, perhaps it somehow messes things up - nothing changes, I can see update requested in logs, but screen state doesn't change (even though koreader changed page)
  That's all that comes to my mind.

[pazos]

Would it be possible that self:_updateFull() finishes execution before android.lib.ANativeWindow_unlockAndPost(android.app.window)? I'm not too familiar with how ffi stuff works, but assuming ffi calls are non blocking, it could be possible?

I don't think so. What happens between ANativeWindow_lock and ANativeWindow_unlockAndPost should block. Basically we lock a surface/canvas to draw, allocate a new ANativeWindow_Buffer, make a copy of the blitbuffer and finally post the surface to the HWComposer.

The delays are there, because sometimes update happens before the "state" is refreshed

That's my point entirely :). On "normal" EPD that means two updates, one requested by us, which wins the race, and other requested by the system. If you disable EPD updates the only update we can see is the one we request (if that happens before the surface is unlocked then you'll see the previous frame, as EPD updates don't change what is drawn into the native window).

what happens when I "prevent" refresh, but don't refresh screen later (it should remain the same, no matter what I do) - confirmed, no system screen updates unless I force one
what happens when I get rid of "delay" thread, perhaps it somehow messes things up - nothing changes, I can see update requested in logs, but screen state doesn't change (even though koreader changed page)
That's all that comes to my mind.

Nothing against this PR as is (if that's neccesary). Please add a bunch of comments. If you use just a couple of delays then I still think that something like #1221 (review) makes sense (just return these few delays) and makes the (rest of the) code more legible :)

[Galunid]

@pazos Please take a look and see if it's good to merge. I tested it on onyx and works fine.

[pazos]

Maybe makes sense to use a name that represents our intent instead of the waveform and mode. That would mean calling it "full", "partial", "ui", "partialUi" and "fast" instead of "full_gc16", "partial_regal", "full_regal", "partial_du".

[pazos]

same as above

[pazos]

like with "delay_ui" and "delay_page" I wouldn't mind a "delay_fast" set to 0 for obvious reasons :)

[pazos]

The whole file is still a mess and that's not your fault :)

I mean, it started in https://github.com/koreader/koreader-base/pull/798/files and got complicated in 910d0f9.

I think it would be cool to move the platform specific logic to android-luajit-launcher and just retrieve values with something like:

local full, partial, ui, ui_partial, fast, delay, delay_ui, delay_fast = android.getEpdConstants()

This will clean this file a lot!

All out of the scope of this PR, which I think is fine. Anyways our future self will take care of it :)

Waiting for @Frenzie to review & merge various things (this, koreader/android-luajit-launcher#250 and koreader/android-luajit-launcher#257)

[pazos]

@Galunid: see a diff that would clean this file by moving all platform code to android-luajit-launcher

diff -uNr a/framebuffer_android.lua b/framebuffer_android.lua
--- a/framebuffer_android.lua	2020-10-27 13:32:29.967820415 +0100
+++ b/framebuffer_android.lua	2020-10-27 13:40:49.169520428 +0100
@@ -11,43 +11,10 @@
 -- does the device needs to handle all screen refreshes
 local has_eink_full_support = android.isEinkFull()
 
--- for *some* rockchip devices
-local rk_full, rk_partial, rk_a2, rk_auto = 1, 2, 3, 4 -- luacheck: ignore
 
-local framebuffer = {}
+local full, partial, full_ui, partial_ui, fast, delay_page, delay_ui, delay_fast = android.getEinkConstants()
 
-local function getWaveformsAndDelays(platform)
-    --common for freescale and qualcomm EPD Controllers
-    local full, partial = 32, 0
-    local wf_du, wf_gc16 = 1, 2
-    local partial_du, partial_gc16 = wf_du + partial, wf_gc16 + partial
-
-    -- different contants
-    local full_gc16, full_regal, partial_regal
-    local delay_page, delay_ui
-    if platform == "freescale" then
-        local wf_regal = 7
-        full_gc16 = wf_gc16 + full
-        full_regal = wf_regal + full
-        partial_regal = wf_regal + partial
-        delay_page = 0
-        delay_ui = 0
-    elseif platform == "qualcomm" then
-        local wf_regal = 6
-        local mode_wait = 64
-        full_gc16 = wf_gc16 + full + mode_wait
-        -- partial regal seems broken (it refreshes full screen causing a "flicker")
-        -- having partial gc16 looks better, but since freescale uses partial_regal
-        -- variable, I overwrite it here
-        full_regal = wf_regal + full
-        partial_regal = wf_gc16 + partial -- wf_regal + partial
-        delay_page = 250
-        delay_ui = 100
-    end
-    return full_gc16, full_regal, partial_gc16, partial_regal, partial_du, delay_page, delay_ui
-end
-
-local full_gc16, full_regal, partial_gc16, partial_regal, partial_du, delay_page, delay_ui = getWaveformsAndDelays(eink_platform)
+local framebuffer = {}
 
 -- update a region of the screen
 function framebuffer:_updatePartial(mode, delay, x, y, w, h)
@@ -64,16 +31,16 @@
     -- freescale ntx platform
     if has_eink_screen and (eink_platform == "freescale" or eink_platform == "qualcomm") then
         if has_eink_full_support then
-            -- we handle the screen entirely. 250 delay for qualcomm, no for freescale
-            self:_updatePartial(full_gc16, delay_page)
+            -- we handle the screen entirely
+            self:_updatePartial(full, delay_page)
         else
             -- we're racing against system driver. Let the system win and apply
             -- a full update after it.
-            self:_updatePartial(full_gc16, 500)
+            self:_updatePartial(full, 500)
         end
     -- rockchip rk3x platform
     elseif has_eink_screen and (eink_platform == "rockchip") then
-        android.einkUpdate(rk_full)
+        android.einkUpdate(full)
     end
 end
 
@@ -168,7 +135,7 @@
     android.lib.ANativeWindow_unlockAndPost(android.app.window);
 end
 
-function framebuffer:refreshFullImp(x, y, w, h)
+function framebuffer:refreshFullImp(x, y, w, h) -- luacheck: ignore
     self:_updateWindow()
     self:_updateFull()
 end
@@ -176,40 +143,35 @@
 function framebuffer:refreshPartialImp(x, y, w, h)
     self:_updateWindow()
     if has_eink_full_support then
-        -- in case of qualcomm it's actually partial_gc16
-        self:_updatePartial(partial_regal, delay_page, x, y, w, h)
+        self:_updatePartial(partial, delay_page, x, y, w, h)
     end
 end
 
 function framebuffer:refreshFlashPartialImp(x, y, w, h)
     self:_updateWindow()
     if has_eink_full_support then
-        self:_updatePartial(full_regal, delay_page, x, y, w, h)
+        self:_updatePartial(full, delay_page, x, y, w, h)
     end
 end
 
 function framebuffer:refreshUIImp(x, y, w, h)
     self:_updateWindow()
     if has_eink_full_support then
-        -- in case of qualcomm it's actually partial_gc16
-        self:_updatePartial(partial_regal, delay_ui, x, y, w, h)
+        self:_updatePartial(partial_ui, delay_ui, x, y, w, h)
     end
 end
 
 function framebuffer:refreshFlashUIImp(x, y, w, h)
     self:_updateWindow()
     if has_eink_full_support then
-        self:_updatePartial(full_regal, delay_ui, x, y, w, h)
+        self:_updatePartial(full_ui, delay_ui, x, y, w, h)
     end
 end
 
 function framebuffer:refreshFastImp(x, y, w, h)
     self:_updateWindow()
     if has_eink_full_support then
-        -- qualcomm shouldn't have refresh here, since the update will "merge",
-        -- and there will be no "black flash" on icons/menu items and nothing
-        -- bad happens even if the "flash" doesn't appear
-        self:_updatePartial(partial_du, 0, x, y, w, h)
+        self:_updatePartial(fast, delay_fast, x, y, w, h)
     end
 end

diff --git a/assets/android.lua b/assets/android.lua
index 4ca52c2..979548a 100644
--- a/assets/android.lua
+++ b/assets/android.lua
@@ -1993,6 +1993,38 @@ local function run(android_app_state)
         end)
     end
 
+    android.getEinkConstants = function()
+        local isEink, platform = android.isEink()
+        if not isEink then return end
+        if platform == "rockchip" then
+            -- rockchip devices
+            local full, partial, fast, auto = 1, 2, 3, 4
+            return full, partial, auto, auto, fast
+        else
+            -- freescale/qualcomm
+            local mode_full, mode_partial = 32, 0
+            local wf_du, wf_gc16 = 1, 2
+            local delay_fast = 0
+            local fast, partial = wf_du + mode_partial, wf_gc16 + mode_partial
+            local full, full_ui, partial_ui, delay, delay_ui
+            if platform == "freescale" then
+                local wf_regal = 7
+                full = wf_gc16 + mode_full
+                full_ui = wf_regal + mode_full
+                partial_ui = wf_regal + mode_partial
+                delay, delay_ui = 0, 0
+            elseif platform == "qualcomm" then
+                local wf_regal = 6
+                local mode_wait = 64
+                full = wf_gc16 + mode_full + mode_wait
+                full_ui = wf_regal + mode_full
+                partial_ui = wf_gc16 + mode_partial
+                delay, delay_ui = 250, 100
+            end
+            return full, partial, full_ui, partial_ui, fast, delay, delay_ui, delay_fast
+        end
+    end
+
     android.needsWakelocks = function()
         return JNI:context(android.app.activity.vm, function(jni)
             return jni:callIntMethod(

Not tested but luachecked.

[Galunid]

@pazos I didn't test it yet, but it looks reasonable. Wouldn't it be better to add a method to all EPDControllers, that returns List/Array of modes? I think it'd be cleaner solution, since you only need to modify one place, instead of two, if you were to add a new controller. I'm not sure how problematic mapping android variables to lua is, but it shouldn't be too bad.

EDIT:
I'll try to test it tomorrow, since I'm a bit busy today.

[pazos]

Wouldn't it be better to add a method to all EPDControllers, that returns List/Array of modes? I think it'd be cleaner solution, since you only need to modify one place, instead of two, if you were to add a new controller.

I thought about it. I don't think it would be a good idea:

• epdInterface just exposes a single method to update/refresh the screen. It doesn't check if a given mode is valid for a given device.
• kotlin doesn't handle multiple return values. You need to return them all in a single array/string and unpack values in assets/android.lua. If you add a new platform which requires a new constant you need to modify the values returned in every other platform to match the array/string format.
• a function in assets/android.lua is still required anyways.

At the end the code in base just calls android.einkUpdate(mode) or android.einkUpdate(mode, delay, x, y, width, height) based on isEinkFull.