EInk controller support for Nook Glowlight 4/4e/4 plus

[Codereamp]

This change is 

[Codereamp]

Continuing discussion

Btw, I don't know whether it will help or not, but one of my experiments was one when I supposedly brought NativeSurfaceView to top with the following lines

            view = NativeSurfaceView(this)
          /* added-> */     view?.setZOrderOnTop(true);
          /* added-> */     view?.holder?.setFormat(PixelFormat.TRANSPARENT);
            window.takeSurface(null)
            view?.holder?.addCallback(this)
            setContentView(view)

I didn't achieve the objective, but nothing crashed and everything was drawing as expected.

Are you sure?

Any view that inherits from the Activity class is already hooked in the view hierarchy.

It's just a guess if for some reasons making a default SurfaceView in databinding/xml View would create some (visual) conflict. The added lines was borrowed from some existing code from the stackoverflow.

I see no problem with changes in the test as long as it doesn't hurt other tests.

I also thought that some regression might appear. One of possible changes might be to turn needsView into one with dual result, the second one in the list being "needsViewForTesting" sense. In this case just the new controller will risk the new feature.

[Codereamp]

A follow up to my confusion and @pazos reaction. I looked at the log while following what happens on the screen, the controller call (addEpdc) works as a flag for the next drawing and doesn't affect the current one. It affects any small changes involved.

It works like this. KOReader, for instance, decided that it's time to make a 1 in 6 full-refreshes, the log indicates that the addEpdc call was made, a new page was shown, but no visible changes of a full refresh. Then I'm clicking the bottom part to reveal the status bar. It shows, but with a full screen refreshing. The same full refresh outside the page flipping happens when I tap the top menu area.

[Codereamp]

Huh, I must admit that this peculiarity doesn't look good for plenty of contexts:

• clock updating on the device in this refresh-waiting state will unexpectedly refresh for the user when the next minute comes
• with the chapter-bounding refreshes, the actual one will be on the next page which would also look like unexpected
• "always flash on pages with images" also will flash on wrong times
• even if the SurfaceView for TestActivity is implemented, it would require some additional steps to force the "waiting" refresh

[Codereamp]

Probably calling the conventional Invalidate for the rect or the whole view right after addEpdc call might overcome this "full-refresh on hold". Or some other post/before/after. I will try today

[hugleo]

Also consider:

Thread.sleep(delay)

[Codereamp]

Thread.sleep(delay)

Tried with and without it plus invalidate/postInvalidateDelayed with and without delays. Also tried to call an invalidate method of ViewRootImpl with mode parameter (involves setAccessible call to work around the protection). No luck.

Partial luck was with additional methods of the Surface : lockCanvas/unlockCanvasAndPost. So I managed to force the refresh at about the time of the request, but the reader at this moment shows quickly the right page, then a refreshed one two pages before (or after) then returns to the right one. For forward if I listed 440, 441, ... it's time for 442 refreshed, I see briefly 442, refreshed 440, the screen is still, after key pressing I see 443. For backward 430, 429, .. it's time for 428 refreshed, briefly 428, then 430 refreshed, after key press 427. The code used is below

override fun setEpdMode(targetView: android.view.View,
                        mode: Int, delay: Long,
                        x: Int, y: Int, width: Int, height: Int, epdMode: String?)
{
    try {
      Log.i(NGL4TAG, String.format(Locale.US, "setEpdMode to addEpdc: type:%d x:%d y:%d w:%d h:%d",
        mode, x, y, width, height))

      val surf: Surface = (targetView as SurfaceView).holder.surface

      val rect: Rect = Rect();
      rect.left = x;
      rect.top = y;
      rect.right = width;
      rect.bottom = height;

      val Canv: Canvas = (Class.forName("android.view.Surface").getDeclaredMethod("lockCanvas", Rect::class.java).invoke(surf, rect)) as Canvas;

        val iArr = intArrayOf(x, y, width, height, mode)

        Class.forName("android.view.Surface").getDeclaredMethod("addEpdc", IntArray::class.java).invoke(
          surf, iArr);

      Class.forName("android.view.Surface").getDeclaredMethod("unlockCanvasAndPost", Canvas::class.java).invoke(surf, Canv)

    } catch (e: Exception) {
        Log.e(NGL4TAG, e.toString())
        false
    }
}

It was strange, I would understand if I messed up with the canvas and the current page was kept, but two page difference might mean that something went wrong with (callback) result and KOReader went opposite way just for one page.

Side note: I noticed that methods einkUpdate, setEpdMode, requestEpdMode everywhere use width and height actually assuming right and bottom, but I suspect it has some historical roots)

[hugleo]

Any other useful method? You can get all methods with something like:

val surfaceClass = android.view.Surface::class.java

val methods: Array<Method> = surfaceClass.declaredMethods

for (method in methods) {
    Log.i(NGL4TAG, method.name)
}

or

val myClass = Class.forName("android.view.View") val methods: Array = myClass.declaredMethods

Don't remember exactly.

[Codereamp]

val methods: Array = myClass.declaredMethods

Thanks for pointing out, another tool in the toolbox.

Currently the decoompiled sources helped a lot, including the cross-reference in the bodies, below I will show the methods from the Surface class not present in the corresponding android 8.1 source. native prefix is probably the JNI methods/calls to .so libraries of the framework surface_unique_methods.txt. Interestingly, many names points to the handwriting part of einking

[Codereamp]

A fix probably to mitigate a delayed refresh problem

• I'm learning as I go :) So the lockCanvas api is a official one so no need for the reflection.
• I was actually messing up with previous buffers, the [docs says](https://developer.android.com/reference/android/view/SurfaceHolder#lockCanvas())

The content of the Surface is never preserved between unlockCanvas() and lockCanvas(), for this reason, every pixel within the Surface area must be written. The only exception to this rule is when a dirty rectangle is specified, in which case, non-dirty pixels will be preserved.

And this exception part was what allowed a new experiment, passing a rect with the size (1, 1) makes refreshing act at the moment it was requested. I briefly see a non-refreshed (non-inverted) version of the new page, but the refresh is probably 100 ms after it .The new code now is

val surf: Surface = (targetView as SurfaceView).holder.surface

val rect: Rect = Rect();
rect.left = x
rect.top = y
rect.right = x + 1
rect.bottom = y + 1

val canv: Canvas = surf.lockCanvas(rect)
if (canv != null)
{
  val iArr = intArrayOf(x, y, width, height, mode)

  Class.forName("android.view.Surface").getDeclaredMethod("addEpdc", IntArray::class.java).invoke(surf, iArr);
  surf.unlockCanvasAndPost(canv)
} else {
  Log.e(NGL4TAG, "lockCanvas returned null")
}

Still, a single pixel preserved from some previous state is not ok, so tried to pass size (0, 0) and this didn't work, no refresh at all, so trying to lock an empty canvas somehow makes the following addEpdc call invalid. What's interesting that the canvas is not null according to the log.

I don't like the solution, but I kind of better understand how SurfaceView/Surface are supposed to work. I may be wrong, please correct me. The dual nature of SurfaceView is mentioned everywhere. But how these two layers work together is not always clear. It never mentioned explicitly, but if the call setWillNotDraw(false) is made then there will probably be two surfaces that is mixed by the surface flinger independently of the view hierarchy (resource wasting is mentioned everywhere so it was an implicit conclusion0. The order of these surfaces is determined by the setZOrderOnTop call. As I understand, general invalidates affects onDraw handlers so the surface that other than SurfaceView' one. What I work with in the code above is the SurfaceView' one. In this case I would understand that the invalidate with the mode attached works for TestActivity (where there's a single surface absorbing actions from views), and not working in the MainActivity where invalidates don't reach and affect the other surface. But as other controllers with needsView show, it matters and works for other devices. I still hope to find a good solution on this device.

[hugleo]

Same behavior if you override the SurfaceView on the MainActivity.kt ?

override fun onDraw(canvas: Canvas) { super.onDraw(canvas)

some proper logic here and execute the refresh only when need.

val iArr = intArrayOf(x, y, width, height, mode) Class.forName("android.view.Surface").getDeclaredMethod("addEpdc", IntArray::class.java).invoke(surf, iArr)

and you call postInvalidate should trigger onDraw.

This sounds pretty ugly, is just for test, :)

[Codereamp]

This sounds pretty ugly, is just for test, :)

I'm very open to suggestion given how many unknowns are here :)

You mean to add

    private class NativeSurfaceView(context: Context): SurfaceView(context),
        SurfaceHolder.Callback {
        init { holder.addCallback(this) }
        override fun surfaceCreated(holder: SurfaceHolder) { setWillNotDraw(false) }
        override fun onDraw() { // <-- added  
            // detect the the mode, if full refresh then addEpdc

and to call postInvalidate as other controllers do in the traditional place?

[Codereamp]

While trying to implement something with onDraw, at least moving into the direction suggested (by @hugleo), I just once again found something useful in past attempts.

The method with bringing NativeSurfaceView to top actually works if coupled with default postInvalidateDelayed call. I mentioned it previously as not working, but it looks like I made the variants with general invalidate calls with mode attached.

So the following change allows refreshing at the expected times with delays as low as 10 ms (in the log the actual delay between requesting and committing from sunxi driver was around 110-120 ms)

        val surfaceKind: String = if (device.needsView) {
            view = NativeSurfaceView(this)
              view?.setZOrderOnTop(true); // for NGL4
              view?.holder?.setFormat(PixelFormat.TRANSPARENT); // for NGL4
            window.takeSurface(null)
            view?.holder?.addCallback(this)
            setContentView(view)
            "SurfaceView"
        } else {
            "Native Content"
        }

the call in the driver was

    override fun setEpdMode(targetView: android.view.View,
                            mode: Int, delay: Long,
                            x: Int, y: Int, width: Int, height: Int, epdMode: String?)
    {

       Log.i(NGL4TAG, String.format(Locale.US, "defaulting to requestEpdMode: type:%d delay: %d x:%d y:%d w:%d h:%d",
          mode, delay, x, y, width, height))
       requestEpdMode(targetView, mode, delay, x, y, width, height)

    }

As for the surface and onDraw handler, it's a strange beast really. The handler is always called after postInvalidateDelayed call, but I could not manage to draw a primitive like drawRect in order for it be visible on top of the text (for testing purpuses). But it is there and not only affects refreshes when brought to top, it also behaves the same if I at the same time I

• remove ... setFormat(PixelFormat.TRANSPARENT); line
• append canvas.drawColor(Color.TRANSPARENT, PorterDuff.Mode.CLEAR); in the onDraw handler

Maybe I'm just capable of providing the right command/mode/blend.

Two questions if this new route is considered:

• The change of the order is the serious one, but overall for me it looks better than addEpdc approach. Probably this means that somehow a controller might provide a special request for this. Maybe turning needsView into enumeration? With (no, default, ontop) values. I know that it would require some refactoring, when the current implementations will get "no" and "default".
• Also I'm keeping the work into "full-only" mode as was suggested, but the method _updateFull has some things hard-coded that prevents full-only controllers with other platform names use their delays of choice. But I know that changing this file is risky, but at least the function might call self:_updatePartial(full, delay_page) for full-only controllers with unrecognizable names.

[hugleo]

IMHO this could be implemented direct on drivers that need it

override fun setEpdMode(targetView: android.view.View,
                        mode: Int, delay: Long,
                        x: Int, y: Int, width: Int, height: Int, epdMode: String?)
{
    requestEpdMode(targetView, mode, delay, x, y, width, height)
}

to something like: if (mode == "full") { requestEpdMode(targetView, mode, delay * 2, x, y, width, height) }

Escalate these questions to @pazos :)

[Codereamp]

IMHO this could be implemented direct on drivers that need it

Yes, the view manipulations might also be implemented in a common ancestor to be (optionally) overridden by the controller. I suspect that in the delay case it was much much easier and faster to experiment/patch in the lua layer and somehow the change has survived. With so many small details I doubt anyone can stand to make proper refactoring

[pazos]

Maybe turning needsView into enumeration? With (no, default, ontop) values. I know that it would require some refactoring, when the current implementations will get "no" and "default".

@Codereamp: sorry for the delay!

It is not needed. You can safely assume the new view will be on the top of the stack of the view hierarchy as long as it is transparent. The reason it isn't there is because none of the eink devices required it until now.

The same with chromebooks and android tv. Both platforms have issues without a surfaceView, but the issue goes away with one, even when it isn't on top.

AFAIK there's no performance hit with that change. (ie: between a opaque surfaceview on the bottom vs a transparent surfaceview on top). All of this is handled by the window compositor.

So, feel free to go with it :)

[pazos]

Also I'm keeping the work into "full-only" mode as was suggested, but the method _updateFull has some things hard-coded that prevents full-only controllers with other platform names use their delays of choice.

Yay, the device abstraction is an eternal work in progress that gets better each time we support a new kind of device. I'm mostly agaisnt doing changes without a particular purpose, so, unless the device you're trying to support requires it, please leave it as is.

Aaand, if you need it, please tell me what values you want in there, so we can think about a proper abstraction :)

[Codereamp]

@pazos, thanks for returning :) Also one step at a time is a good thing here, because I really see no really bad consequences without eInk support on this device, but having the feature working is also a good thing

Aaand, if you need it, please tell me what values you want in there, so we can think about a proper abstraction :)

I will post a modified version and the original, if you find the changes suitable I might make a PR in the other project

• I moved if has_eink_screen to the outer layer because it's more readable and really doesn't change the logic
• As the search shows, currently only three platform names are used for eink: freescale, qualcomm, rockchip so this new else part should not affect any of existing controllers.
• Any future platforms that might require to change this code will just add some elseif before final default else
• If it's ok, I will make a new name for NGL4 controller, probably ngl4. Technically there's no such platform, so I thought about more neutral name like (unannounced, unknown, einkgeneric), but one should remember that this name will probably be displayed in the interface so it's better to be about something
• I tested this version on my ngl4e (ensuring that the modified version is in the *.7z lua package) with ngl4 platform name, the refresh is visible, the delay according to the log is what was provided by getWaveformDelay.

So, the modified version

function framebuffer:_updateFull()
    if has_eink_screen then
      -- freescale ntx platform
      if (eink_platform == "freescale" or eink_platform == "qualcomm") then
        if has_eink_full_support then
            -- 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, 500)
        end
      -- rockchip rk3x platform
      elseif (eink_platform == "rockchip") then
        android.einkUpdate(full)
      -- all possible platforms other than these three (freescale, qualcomm, rockchip) uses default call and delay param
      else
        self:_updatePartial(full, delay_page)
      end
    end
end

The current version

function framebuffer:_updateFull()
    -- 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
            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, 500)
        end
    -- rockchip rk3x platform
    elseif has_eink_screen and (eink_platform == "rockchip") then
        android.einkUpdate(full)
    end
end

[pazos]

I moved if has_eink_screen to the outer layer because it's more readable and really doesn't change the logic

No problem with that.

As the search shows, currently only three platform names are used for eink: freescale, qualcomm, rockchip so this new else part should not affect any of existing controllers.

No need for an else statement there until there's a "consumer" for that branch :)

Any future platforms that might require to change this code will just add some elseif before final default else

That's something for the future, so again, there's no need for the else branch.

If it's ok, I will make a new name for NGL4 controller, probably ngl4. Technically there's no such platform, so I thought about more neutral name like (unannounced, unknown, einkgeneric), but one should remember that this name will probably be displayed in the interface so it's better to be about something

No please, you're subclassing NTXEPDController and all those subclasses are named "freescale". I know freescale, qualcomm and rockchip don't have magic meanings, but at least they're 3 and not 33 :)

I tested this version on my ngl4e (ensuring that the modified version is in the *.7z lua package) with ngl4 platform name, the refresh is visible, the delay according to the log is what was provided by getWaveformDelay.

The same should happen with platform freescale as long as the driver is full-only

[Codereamp]

The same should happen with platform freescale as long as the driver is full-only

Are you sure? If you look at the branches in _updateFull in my quote above or in the original, there's a hard-coded 500 ms delay instead of the provided getWaveformDelay value if the platform is freescale and getMode = full-only (in this case has_eink_full_support is false, the excerpts leading to this are below)

->>>> has_eink_full_support \base\ffi\framebuffer_android.lua local has_eink_full_support = android.isEinkFull() ->>>> android.isEinkFull \platform\android\luajit-launcher\app\src\main\java\org\koreader\launcher\MainActivity.kt

override fun isEinkFull(): Boolean {
        return device.hasFullEinkSupport
    }

->>>> device.hasFullEinkSupport \platform\android\luajit-launcher\app\src\main\java\org\koreader\launcher\device\Device.kt val hasFullEinkSupport = epd.getMode() == "all"

[Codereamp]

Btw, it probably looked like I suggested to refactor the routine for the sake of purity or something else. Not at all :) I just could not get the page delay if my controller was freescale/full-only. The only way was to modify this routine or go all insteaf of full-only

[hugleo]

The 500 ms delay introduced here is due to the related issue: https://github.com/koreader/koreader-base/pull/933

There are some refactoring here: https://github.com/koreader/android-luajit-launcher/pull/322

And also here: https://github.com/koreader/koreader-base/commit/ec3d5021852a1acf3471364a020d07b7de439356

It seems that the 500 ms delay was let over in this consideration.

I think was mistakenly added for Qualcomm as well but can't remove it because there are already a bunch of new devices added after that.

I suggest to:

removing the 500 ms delay from framebuffer_android.lua.

Remove const val EINK_WAVEFORM_DELAY = 0 from:

https://github.com/koreader/android-luajit-launcher/blob/master/app/src/main/java/org/koreader/launcher/device/epd/freescale/NTXEPDController.kt

Add the same line to the specific controller, specifying the value. For example:

https://github.com/koreader/android-luajit-launcher/blob/master/app/src/main/java/org/koreader/launcher/device/epd/NookEPDController.kt

const val EINK_WAVEFORM_DELAY = 500

Refactor this for all drivers/platforms, including Qualcomm. Note that some drivers, such as device/epd/TolinoEPDController.kt, need to keep EINK_WAVEFORM_DELAY = 0.

I'm not sure if this is the best approach, @pazos can give some thoughts on that.

[pazos]

The same should happen with platform freescale as long as the driver is full-only

Are you sure? If you look at the branches in _updateFull in my quote above or in the original, there's a hard-coded 500 ms delay instead of the provided getWaveformDelay value if the platform is freescale and getMode = full-only (in this case has_eink_full_support is false, the excerpts leading to this are below)

->>>> has_eink_full_support \base\ffi\framebuffer_android.lua local has_eink_full_support = android.isEinkFull() ->>>> android.isEinkFull \platform\android\luajit-launcher\app\src\main\java\org\koreader\launcher\MainActivity.kt

override fun isEinkFull(): Boolean {
      return device.hasFullEinkSupport
  }

->>>> device.hasFullEinkSupport \platform\android\luajit-launcher\app\src\main\java\org\koreader\launcher\device\Device.kt val hasFullEinkSupport = epd.getMode() == "all"

You're right, I got it the other way around :p

It is easier to hardcode the big 500ms delay on the drivers that require it:

https://github.com/koreader/android-luajit-launcher/blob/master/app/src/main/java/org/koreader/launcher/device/epd/NookEPDController.kt#L39

and

https://github.com/koreader/android-luajit-launcher/blob/master/app/src/main/java/org/koreader/launcher/device/epd/OnyxEPDController.kt#L39

(I'm not sure about the second one, it is using 500 right now but has a sensible 250ms hardcoded. Pinging @Galunid)

and leave the thing:

function framebuffer:_updateFull()
    if has_eink_screen then
      if (eink_platform == "rockchip") then
         android.einkUpdate(full)
      else
        self:_updatePartial(full, delay_page)
    end
end

[pazos]

yay, @hugleo and I spotted the same things at the same time. Is this magic? :smile:

[Galunid]

iirc. onyx devices used to need some delay (150-250ms). I'm not sure if that's still needed since there were a few firmware updates since then. In any case 500ms is definitely too much ;)

[hugleo]

For the new ones onyx tabultrac or related, need between 150-250ms as well. No need for 500 ms.

[pazos]

Btw, it probably looked like I suggested to refactor the routine for the sake of purity or something else. Not at all :) I just could not get the page delay if my controller was freescale/full-only. The only way was to modify this routine or go all insteaf of full-only

@Codereamp

Again, apologies. I got it wrong :p.

Changes were pushed to master, so feel free to rebase from it. You shouldn't need to do any change outside the new controller.

[Codereamp]

@pazos, I'm glad I was understood :)

A couple of questions:

• After my initial PR some things have changed, so it would involve editing existing and maybe new files. What is the best way to do this and keep the conversation? I'm familiar only with basic operations with git so here it's better for me to be guided, thanks
• Are we ok with SurfaceView "bring to top" changes for all controllers so I will make it a part of the future changes? Honestly I'm still a little scared about this. Is it possible to make some branch with this only change and suggest to the existing users with working refreshes to test it?
• What about adding this controller to TestActivity? I might as well try to do necessary changes to be fine-tuned later by the core developers. I think that supporting Nooks is good, but allowing testing new constants on new devices would be even better additions.

[pazos]

Are we ok with SurfaceView "bring to top" changes for all controllers so I will make it a part of the future changes? Honestly I'm still a little scared about this. Is it possible to make some branch with this only change and suggest to the existing users with working refreshes to test it?

Yes, for all devices that need a view. It shouldn't have any impact. If it has some impact we might want to consider another change, but not now, please :)

What about adding this controller to TestActivity? I might as well try to do necessary changes to be fine-tuned later by the core developers. I think that supporting Nooks is good, but allowing testing new constants on new devices would be even better additions.

Feel free to improve the TestActivity as much as you like. I'm mostly done with it :p

But please make it on the future, on another PR.

After my initial PR some things have changed, so it would involve editing existing and maybe new files. What is the best way to do this and keep the conversation? I'm familiar only with basic operations with git so here it's better for me to be guided, thanks

It is a bit harder than usual because your feature branch is currently master. So you need to

1. make sure your master branch status is clean.
2. create a new feature branch named whatever: git checkout -b whatever
3. the whatever branch now has your commits.
4. switch to the master branch with git checkout master.
5. remove from the master branch all your local commits (ie: if there're two git reset --hard HEAD~2)
6. now fetch upstream changes in the master branch git pull origin master (or whatever remote ref that points to https://github.com/koreader/koreader.git, you can see it with git remote -v)
7. finally switch to whatever branch and rebase from master:

git checkout whatever
git rebase master

[Codereamp]

It is a bit harder than usual because your feature branch is currently master. So you need to ....

Thanks for the detailed explanation. As I understand in this case I will have a feature branch ready for the new pull request. Should I request a new one and leave this one as is?

[pazos]

It is a bit harder than usual because your feature branch is currently master. So you need to ....

Thanks for the detailed explanation. As I understand in this case I will have a feature branch ready for the new pull request. Should I request a new one and leave this one as is?

You can avoid the need of a new one if you do (on top of previous steps)

git checkout master
git rebase whatever
git push origin master -f

But, yeah. Both options work for me