Add support for setting Warmth for Tolino with ntx_io

[hasezoey]

This PR adds support in TolinoWarmthController.kt to use /dev/ntx_io to set the Warmth

fixes #351
requires koreader/koreader#9511

---

This change is 

[pazos]

Please test your code before opening a PR ;)

Also, even if this worked without root it must go on a different driver and leave this one as is.

But it won't work without root. And if you have root there're better ways of doing the same.

[hasezoey]

Please test your code before opening a PR ;)

i dont know what you mean, i did test it

Also, even if this worked without root it must go on a different driver and leave this one as is.

why? from what i can tell, this is a controller for Tolino devies, and i set it up for a tolino device, with fallback to the old way

But it won't work without root. And if you have root there're better ways of doing the same.

like i already said in koreader/koreader#9511 (comment), it works without root

[pazos]

i dont know what you mean, i did test it

Sorry about that. My fault. I was biased based on previous experiences (not with you).

why? from what i can tell, this is a controller for Tolino devies, and i set it up for a tolino device, with fallback to the old way

because we don't have all past tolino devices to test this new way of doing stuff, which is fragile. And we have past experiences with changes in drivers that work on newer ones but borked legacy devices.

Just create a new driver, let people test your driver and report if it also work. We can do a device migration from one to another without nuking the old one.

it works without root

That's fantastic. You can code it like a simple c function and call it from your driver. Like we're already doing in https://github.com/koreader/android-luajit-launcher/blob/master/jni/lzma/un7z.cpp#L9,

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

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

[hasezoey]

That's fantastic. You can code it like a simple c function and call it from your driver. Like we're already doing in

good to hear that it is possible this way, but i dont trust my self with c

[hasezoey]

also i forgot to note in the opening description of this PR: getting the warmth value does not work and defaults to MAX, if it is possible to read from ntx_io then i have not found that way yet (and /sys/class/backlight/lm3630a_led/color is only readable by root)

[NiLuJe]

That also sounds like a familiar NTX quirk, so I doubt there's a sane solution ;).

[pazos]

also i forgot to note in the opening description of this PR: getting the warmth value does not work and defaults to MAX, if it is possible to read from ntx_io then i have not found that way yet (and /sys/class/backlight/lm3630a_led/color is only readable by root)

That's a drag but I think it won't be too bad. After all it will affect just the first read. After that you can keep current level into a var and update it each time the ioctl call doesn't fail.

Ofc it would be great to retrieve current warmth level from system somehow.

good to hear that it is possible this way, but i dont trust my self with c

No problem with that. I can help. Could you please rework this PR as a different driver? I can submit a PR against your branch so you can test it.

[hasezoey]

updated 8995d0d to have a new controller named TolinoVision5WarmthController.kt which is basically a copy of TolinoWarmthController.kt with modifications from this PR applied and unused values removed
also adds TOLINO_VISION5 in DeviceInfo for detection

for now i have named all new things (controller and enum value in DeviceInfo) to be Vision5 / VISION5 because that is what i use and test it on, but i am up for renaming this

i have also tested the change and it works great (after keeping the state) without root, but it still requires koreader/koreader#9511

No problem with that. I can help. Could you please rework this PR as a different driver? I can submit a PR against your branch so you can test it.

if you can / want to do it, thanks, but i will probably replay later because i need to stop for today

[pazos]

@hasezoey: done in https://github.com/hasezoey/android-luajit-launcher/pull/1

Please test when you have a moment.

I'm also interested in @NiLuJe review.

I've tried to be very verbose about errors, mostly to be useful in the test activity. ioctl should return -1 and populate errno if there's an error or return 0 or above if ok. So in native code we just return different negative values for different errors and log them in the driver.

If some other device uses /dev/ntx_io or another special device that can be manipulated with ioctl() we can refactor the code as the function signature is generic enough to work in such cases.

[hasezoey]

re https://github.com/hasezoey/android-luajit-launcher/pull/1#issuecomment-1246741868

Then it is a mater of merging that PR and rebasing this branch on top of the new master branch :)

it seems like i dont need to merge master, because it is still up-to-date

[NiLuJe]

The C bits look sound ;).

[Frenzie]

Yup, I can't fully judge it but it doesn't smell. ;-)

[hasezoey]

Updated to resolve some review questions, among other things:

• change "TAG" in both the new driver and old driver to differentiate between them
• add comments explaining some variables
• add comments explaining that some value can also be read somewhere on the system
• update class comment to note the original and which models it is for

[pazos]

As you can see in other driver's name I'm not very good naming stuff :p. So feel free to suggest alternatives.

My recommendation is: rename the old TolinoWarmth to TolinoRoot and the new one to TolinoNtx. IMHO the warmth part can be skipped since there's no need for a tolino without warmth (ie: the behaviour of brightness is like on regular android devices)

[hasezoey]

Updated to rename the controllers TolinoRoot and TolinoNtx as suggested (because i also think its a better name)

[pazos]

great, almost ready to go!

to-do: handle the new driver in LightsFactory. I still need to figure a proper way to do that.

could you rebase on top of master to see an updated codacy analysis?

[hasezoey]

could you rebase on top of master to see an updated codacy analysis?

merged latest master

[pazos]

hey, just force pushed.

When I requested a rebase I meant:

git checkout master
git pull origin master
git checkout tolinoNTX_IOWarmth
git rebase master
git push myremote tolinoNTX_IOWarmth -f

I'm fairly sure that git being git there're a million ways of doing the same

[pazos]

Yes, your force push broke things. It was ok when I force pushed.

[hasezoey]

Yes, your force push broke things

broke what?

from what i can tell, it builds just fine and after the install it also works just fine (in the compat report)

[pazos]

your last commit was left in the void.

Could you please fix that? Thanks

[hasezoey]

your last commit was left in the void.

what last commit?

like i said, i dropped the commits that renamed the TAG variable because it was reverted again (and the revert also got dropped), and from what i can tell, that were the only (aside from the "merge master" commits that are missing)

Edit: see 908d6a7, that was the last commit (that you had pushed after your rebase), where the parent is 2039ce1, which is still included

[pazos]

Yup, I'm at work and understood your comment #382 (comment) wrong. All good as is.

[pazos]

And a final question because I got it wrong in the first place: with this PR in both the TestActivity's and KOReader's lightdialog work as expected?

[hasezoey]

And a final question because I got it wrong in the first place: with this PR in both the TestActivity's and KOReader's lightdialog work as expected?

no, with this PR the new driver works as expected, and is correctly set in the TestActivity, but not "globally", i (locally) added a fix in a local branch, but i will wait for the official solution you mentioned in #382 (comment)

my local solution currently for that is:
DeviceInfo.kt.patch

[pazos]

And a final question because I got it wrong in the first place: with this PR in both the TestActivity's and KOReader's lightdialog work as expected?

no, with this PR the new driver works as expected, and is correctly set in the TestActivity, but not "globally", i (locally) added a fix in a local branch, but i will wait for the official solution you mentioned in #382 (comment)

my local solution currently for that is: DeviceInfo.kt.patch

Hmm. My solution isn't required. I got it wrong. There're no TOLINO on the Lights iterator, just TOLINO_EPOS and TOLINO_VISION, so the only way the factory is assigning you a bad driver is because your device is flagged as an EPOS before it is flagged as a VISION.

You can test that by removing TOLINO_EPOS from the lightsMap.

If that is the case then we need to fine tweak the TOLINO_EPOS condition. I think it got reported somewhere, let me check.

[hasezoey]

You can test that by removing TOLINO_EPOS from the lightsMap.
If that is the case then we need to fine tweak the TOLINO_EPOS condition. I think it got reported somewhere, let me check.

i think i wrote about this before, but i cannot find it anymore, so i will write it here again:
i had already tried this, and noticed that HashMap is always outputting a sorted iterator, so it does not matter if TOLINO_EPOS or TOLINO_VISION5 gets added first / last, and i had also already tried to comment-out TOLINO_EPOS, which i noted to be working, so i just added (locally) a overwrite condition
and i was also hesitant to change existing conditions (based on comments from other PR's), so i did not and also dont know enough to change the implementation of the lights selection or how the project wants it to be done - so i just added a overwrite when TOLINO_VISION5 is true

[pazos]

You can test that by removing TOLINO_EPOS from the lightsMap.
If that is the case then we need to fine tweak the TOLINO_EPOS condition. I think it got reported somewhere, let me check.

i think i wrote about this before, but i cannot find it anymore, so i will write it here again: i had already tried this, and noticed that HashMap is always outputting a sorted iterator, so it does not matter if TOLINO_EPOS or TOLINO_VISION5 gets added first / last, and i had also already tried to comment-out TOLINO_EPOS, which i noted to be working, so i just added (locally) a overwrite condition and i was also hesitant to change existing conditions (based on comments from other PR's), so i did not and also dont know enough to change the implementation of the lights selection or how the project wants it to be done - so i just added a overwrite when TOLINO_VISION5 is true

aaaahhh, now all makes sense.

I would rather prefeer to tweak Tolino epos to be true with current condition if not HARDWARE.contentEquals("e70k00").
If somehow that makes Tolino EPOS/EPOS2 no longer useable with previous driver that would mean they're also "e70k00" and, if there're no other way to differenciate them, we can always move them to the new driver and call it a day.

Apologies for the missunderstanding, I'm doing all this in coffee breaks and have little time to get things right ;)

[hasezoey]

I would rather prefeer to tweak Tolino epos to be true with current condition if not HARDWARE.contentEquals("e70k00").
If somehow that makes Tolino EPOS/EPOS2 no longer useable with previous driver that would mean they're also "e70k00" and, if there're no other way to differenciate them, we can always move them to the new driver and call it a day.

if the tolino software download site is anything to go by, then:

• Epos2 and Vision5 (also Shine3 and Page2) are the same download package*¹
• Epos1 is its own package
• Epos3 is its own package

*¹: but as can be seen in #351 (comment), the Shine3 is listed as Hardware: e60k00

Tolino Software Update Download Site

[pazos]

Thanks for the info.

I'm afraid we can't figure out how things are without temporarily breaking (or not?) the lights controller on the EPOS/EPOS2. Even if we break them it is not that traumatic. On a couple of months somebody will report the regression paired with test results and we will be in a better position to judge :)

[hasezoey]

I'm afraid we can't figure out how things are without temporarily breaking (or not?)

if you really dont wanna break it, then i guess the following options would be possible:

• change from HashMap to something that uses FIFO (unsorted) or do a custom implementation
• use a overwrite (like i do locally)

[pazos]

I stated that I wouldn't mind to break it. And prefeer to break it better than refactoring for a worse implementation or custom workarounds.

[hasezoey]

I stated that I wouldn't mind to break it

to me it sounded more like you are considering to break it, so i just wrote some "options" i could think of

---

when do you wanna do this change, in this PR or separately?

[pazos]

I stated that I wouldn't mind to break it

to me it sounded more like you are considering to break it, so i just wrote some "options" i could think of

when do you wanna do this change, in this PR or separately?

In this PR is fine.

It should be a matter of doing something like

diff --git a/app/src/main/java/org/koreader/launcher/device/DeviceInfo.kt b/app/src/main/java/org/koreader/launcher/device/DeviceInfo.kt
index 8b8b9d2..ec54674 100644
--- a/app/src/main/java/org/koreader/launcher/device/DeviceInfo.kt
+++ b/app/src/main/java/org/koreader/launcher/device/DeviceInfo.kt
@@ -386,6 +386,7 @@ object DeviceInfo {
         TOLINO_EPOS = BRAND.contentEquals("rakutenkobo")
             && MODEL.contentEquals("tolino")
             && DEVICE.contentEquals("ntx_6sl")
+            && !HARDWARE.contentEquals("e70k00")

Please check it works fine that way. The rest of the PR is ready to go.

[hasezoey]

Please check it works fine that way. The rest of the PR is ready to go.

added with 2ea428e and also tested to work in non-TestActivity

personally, i also think this will not affect EPOS1 or EPOS2, because if the hardware numbering is anything to go by, it should be older though i dont know about EPOS3, but i think it is at least e80k00 (which is from what i know android 8.1)

[NiLuJe]

i think it is at least e80k00 (which is from what i know android 8.1)

Nope, those are NTX board ids ;).

e for EPD, 80 for 8.0", and the rest probably has some deeper meaning, too ;).

[hasezoey]

e for EPD, 80 for 8.0", and the rest probably has some deeper meaning, too ;).

i guess makes sense, i didnt think it had much meaning but more like PCB version / hardware revision number (which probably would increase with generation)

[Nepochal]

Hi,
like I mentioned in #351 this merge did not add ntx_io for non rooted tolino shine 3 devices. They use the hardware key e60k00 which was not considered in this change.