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

added onyx note air 2 and onyx sdk light control #342

Merged
merged 10 commits into from
Dec 6, 2021

Conversation

bezmi
Copy link
Contributor

@bezmi bezmi commented Nov 29, 2021

I've adapted code found at calin-darie/gentle-glow-onyx to create a LightsInterface using the onyx SDK. I've tested and confirmed that it is working for my Boox Note Air 2.

The interface adapts (more intuitive) brightness/warmth values to the warm/cold values that onyx uses, whilst maintaining perceptually uniform brightness across the entire range. A caveat of this is that maximum brightness is limited.

I've also added the Note Air 2 to the EPDFactory as I have confirmed that full refresh works using the OnyxEPDController.

Had to bump minSDK to 19 for compatibility with the Onyx SDK packages.


This change is Reviewable

@pazos
Copy link
Member

pazos commented Nov 29, 2021

Thanks for the patch. We cannot accept it because the minSdk bump.

First try the controller used for other onyx devices. See koreader/koreader#8482 (comment). You'll need the last nightly to run the new test activity.

If that doesn't work or works worse than this implementation you're welcome to submit a patch. Instead of using the sdk onyx provides you need to RE the calls used to get/set light levels and use these calls directly.

@bezmi
Copy link
Contributor Author

bezmi commented Nov 29, 2021

I tried the tests in latest nightly before writing this up. The existing onyx controller does not work at all, unfortunately.

I can override the minSDK change by setting tools:overrideLibrary="com.onyx.android.sdk.device and wrap OnyxSDKCoolWarmCcontroller with a check of the android build version to ensure safety. Would that be acceptable?

I've never decompiled/reverse engineered on the android platform, so would like to avoid it if possible.

@pazos
Copy link
Member

pazos commented Nov 29, 2021

I can override the minSDK change by setting tools:overrideLibrary="com.onyx.android.sdk.device and wrap OnyxSDKCoolWarmCcontroller with a check of the android build version to ensure safety. Would that be acceptable?

Indeed. Please don't add the check within the controller class. Just wrap methods inside try-catch blocks.

I would expect:

  1. the app works on 4.0+
  2. the test app would work with the new class in all android versions. On versions below onyx sdk minApi all posible exceptions are catched.

@@ -28,6 +28,7 @@
android:name=".MainApp"
android:icon="@mipmap/icon"
android:banner="@mipmap/banner"
tools:replace="android:label,android:allowBackup"
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot from 2021-11-30 01-46-05

There is a manifest merge conflict without these tags, is there an alternative way of handling this?

@Galunid
Copy link
Member

Galunid commented Nov 29, 2021

There is no need to include the onyx-sdk as dependency at all. I already have it decompiled, so I can just look up the methods you need and we can use reflection to call them. That's what onyx-sdk does anyway. @bezmi Would that be acceptable for you?

Also does the previous controller not work at all, or just not work in the way you want it?

@bezmi
Copy link
Contributor Author

bezmi commented Nov 29, 2021

I've decompiled the SDK now as well (easier than I though) and they're just using reflection to call methods in android.onyx.hardware.DeviceController.

The file locations are the same; /sys/class/backlight/white/brightness for the cold light and /sys/class/backlight/warm/brightness for the warm light. When running OnyxWarmthController, access permission is not granted and it throws a FileNotFoundException.

@bezmi
Copy link
Contributor Author

bezmi commented Nov 30, 2021

I've removed the onyx sdk dependency and used reflection to call the api directly. The note air 2 falls under the SDMDevice file in the SDK, so that's what I've named the controller.

@pazos
Copy link
Member

pazos commented Nov 30, 2021

@@ -34,7 +34,7 @@
android:usesCleartextTraffic="true"
android:requestLegacyExternalStorage="true"
android:supportsRtl="true"
android:extractNativeLibs="true" >
android:extractNativeLibs="true">
Copy link
Member

Choose a reason for hiding this comment

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

Please remove. If you want to make style changes do it in a separate PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, this one just snuck in

@@ -50,13 +50,15 @@ object DeviceInfo {
ONYX_C67,
ONYX_KON_TIKI2,
ONYX_NOVA2,
ONYX_NOTEAIR2,
Copy link
Member

Choose a reason for hiding this comment

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

nit: NOTE_AIR2

TOLINO
}

enum class LightsDevice {
NONE,
ONYX_C67,
ONYX_NOVA2,
ONYX_NOTEAIR2,
Copy link
Member

Choose a reason for hiding this comment

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

^^

nit: NOTE_AIR2

@pazos
Copy link
Member

pazos commented Nov 30, 2021

The new driver looks ok. It should be compatible with devices supported by our previous controller. If @Galunid wants to try and works for him we can have a single driver for all devices. If not we keep both and call it a day :)

@Galunid
Copy link
Member

Galunid commented Nov 30, 2021

That's my thoughts as well. It should work, but I have some stuff to deal with. I'll test it in the evening/tomorrow, and if it's good, we'll get rid of the previous one.

@Galunid
Copy link
Member

Galunid commented Nov 30, 2021

I tested it and it works on Nova 2, but it handles the brightness/warmth differently from what we do now. What I mean is that 100% brightness + 100% warmth sliders result in 100% warmth LEDs and none of the white ones. Reducing warmth (when brigntness is 100%) increases whiteness.

This may work in unexpected ways with Autowarmth plugin. Personally I'm not a huge fan of the new behavior, because it's different from every other warmth device. I'd rather make it behave like the old one, but working on Note Air 2 too.

@bezmi
Copy link
Contributor Author

bezmi commented Nov 30, 2021

@pazos I'll make all of the requested changes and add to TestActivity.kt.

@Galunid I personally don't like the brightness=cold/white and warmth=warm LED approach. Instead of manual mixing of two colours, it makes more sense for brightness to be related to the combined illuminance and warmth to be related to the colour temperature. Use of the autowarmth plugin is what drove me to write this as I wanted to change colour temperature without perceptual variation in brightness.

Is the current default behaviour specific to onyx, or do the other e-reader devices also behave the same way?

Nevertheless, I'm happy revert to the default brightness behaviour for this pull request and remove the WarmColdToWarmthBrightness stuff. I may see if it's possible write a script for KOReader that will allow toggling to my preferred mode of operation in future.

@pazos
Copy link
Member

pazos commented Nov 30, 2021

Is the current default behaviour specific to onyx, or do the other e-reader devices also behave the same way?

It is the same behaviour on all devices in all platforms :)

frontlight controls white leds. "natural light" controls orange leds (rgb leds in a few devices).
A bunch of things rely on this behaviour, including vertical swipe gestures (left for brightness/ right for warmth).

Nevertheless, I'm happy revert to the default brightness behaviour for this pull request and remove the WarmColdToWarmthBrightness stuff

That would be awesome :)

I may see if it's possible write a script for KOReader that will allow toggling to my preferred mode of operation in future.

It is doable but requires a few tweaks here and there. Currently epd and lights are hardcoded based on device properties. We need to add stuff to ignore detected drivers and let the users to push their own. It is actually useful to have it implemented but needs a few hours of free time.

All starts here:

https://github.com/koreader/android-luajit-launcher/blob/master/app/src/main/java/org/koreader/launcher/MainActivity.kt#L104

Which leads to https://github.com/koreader/android-luajit-launcher/blob/master/app/src/main/java/org/koreader/launcher/device/Device.kt#L8-L9

All of this starts before we run the LuaJIT vm so there's no way to change that as-is. We might need to declare both drivers mutable and add setters for them.

The EPD driver is a bit more tricky since there're a few constants that we retrieve once: https://github.com/koreader/koreader-base/blob/master/ffi/framebuffer_android.lua#L15 and those constants are tied to a specific driver.

The lights driver should be way easier because it is self-contained.

I would like to help you with the implementation as a mid-term task. I'm very busy and my to-do list keeps growing :/

@bezmi
Copy link
Contributor Author

bezmi commented Nov 30, 2021

I was thinking more along the lines of adjusting the functionality of koreader/frontend/device/android/powerd.lua such that the device driver can remain the same.

The user will toggle a setting which will convert the behaviour from mixing of cold/warm LEDs to brightness=combined brightness of both LEDs and warmth=requested colour temperature. This will run an extra step in powerd.lua that changes the requested total brightness + colour temperature into the expected cold/warm values for the device driver.

It will require some device specific information, such as the light output and colour temperature of the warm and cold LEDs for the math to ensure that brightness remains perceptually uniform as colour temperature is altered. Users should be able to tweak these values to get it just right for their device.

Currently, the light output and colour temperatures are pretty much hard coded in the driver I wrote. The values work okay for the Note Air 2, but probably won't be perfect for other devices unless Onyx keeps their choice of warm/cold LEDs identical over the product lines.

Do you think the approach I've outlined above is do-able?

P.s. happy to be pinged whenever testing is required for my device.

@pazos
Copy link
Member

pazos commented Dec 1, 2021

Do you think the approach I've outlined above is do-able?

if android.setScreenWarmth + android.setScreenBrightness are enough to fullfit your needs all the rest can be written in the frontend.

But check first if it behaves as you expected. Actions, gestures and plugins use powerd functions. The dialog does not use them.

There's no easy way to fix that without:

a) reverting back to the good-old frontlight widget, used on the rest of platforms, while keeping android's device abstraction in place.

or

b) writting your custom driver as part of this repo and enable it on runtime based on a flag

Keep in mind device abstraction is a PITA and time is limited.
Any patch that makes the code easier to maintain is welcome :)

@bezmi
Copy link
Contributor Author

bezmi commented Dec 1, 2021

I've tested and pushed the requested changes. Just realised I forgot the test activity. Will push it through soon.

@Galunid
Copy link
Member

Galunid commented Dec 2, 2021

Great, thanks! I haven't tested it yet, but I will do it later today. Could you try to fix the Codacy Static Code Analysis issues? Those are just styling changes.

@bezmi
Copy link
Contributor Author

bezmi commented Dec 2, 2021

No problem, took me a couple of goes, but that's what squashing is for :). I'm not sure about the current naming, if you guys remove the old OnyxWarmthController then I guess this can take over the name.

a) reverting back to the good-old frontlight widget, used on the rest of platforms, while keeping android's device abstraction in place.

@pazos, I was able to get this working with the alternative brightness implementation after some trivial changes. However, I don't think it's a change that anyone would enjoy. I certainly prefer the style of the native dialog. I think I'll put a pin in it for now and keep a patch for myself with the other (better!) driver. Don't really see anyone else complaining about the warmth implementation on these devices anyway.

}

object FrontLight {

Copy link
Member

Choose a reason for hiding this comment

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

Redundant new line


object FrontLight {

private val fLController = forName("android.onyx.hardware.DeviceController")
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer flController, since afaik we use Frontlight, not FrontLight

@Galunid
Copy link
Member

Galunid commented Dec 3, 2021

I tested it and it works on Nova 2! Feel free to nuke the old one and replace it with this one. Not sure if @pazos has any comments, but it looks ready to merge, after fixing those nits I mentioned in the review.

@bezmi
Copy link
Contributor Author

bezmi commented Dec 4, 2021

Apologies for the multiple commits, I missed a few changes when removing the OnyxSDMController references. OnyxWarmthController has been updated with the new driver code.

@pazos
Copy link
Member

pazos commented Dec 5, 2021

@Galunid: please merge when ready. Looks quite good :)

@Galunid
Copy link
Member

Galunid commented Dec 6, 2021

@Frenzie Please give me permission to merge ^_^

I'll build and test it in a few hours code is looking alright

@Frenzie
Copy link
Member

Frenzie commented Dec 6, 2021

@Galunid I was going to say @pazos' permission suffices but then I realized you must mean GitHub user groups. I'll look into it.

@Galunid Galunid merged commit f7d8112 into koreader:master Dec 6, 2021
pazos added a commit to pazos/android-luajit-launcher that referenced this pull request Dec 26, 2021
@pazos pazos mentioned this pull request Dec 26, 2021
@pazos
Copy link
Member

pazos commented Dec 26, 2021

@bezmi: this PR got reverted because koreader/koreader#8584.

Feel free to submit the PR again with the following changes:

  1. The driver shouldn't override OnyxWarmthController, use another name.
  2. Just move your own tested devices to the new driver
  3. All the methods of the new class must be protected (ie: calling those methods won't make the VM crash even when invoked from an unsupported device). The best way to test this is asign your new driver to the emulator and check that everything behaves fine (ie: stacktraces printed for each attempt to call an unsupported method, no crashes)

sorry for the inconvenience

Frenzie pushed a commit to koreader/koreader that referenced this pull request Dec 26, 2021
* revert koreader/android-luajit-launcher#342 
* store apks in private dir and remove them after update
@bezmi
Copy link
Contributor Author

bezmi commented Feb 22, 2022

I'm happy to rename it back to the old OnyxSDKController and update the error catching. At least if the controller is available, people whose device doesn't work with the standard onyx controller will be able to try with the SDK based one instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants