Closed hasezoey closed 1 year ago
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.
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 https://github.com/koreader/koreader/pull/9511#issuecomment-1243049473, it works without root
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,
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
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 also sounds like a familiar NTX quirk, so I doubt there's a sane solution ;).
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.
updated 8995d0da046405b9f228998c4ddc1d720f923e59 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 https://github.com/koreader/koreader/pull/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
@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.
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
The C bits look sound ;).
Yup, I can't fully judge it but it doesn't smell. ;-)
Updated to resolve some review questions, among other things:
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)
Updated to rename the controllers TolinoRoot
and TolinoNtx
as suggested (because i also think its a better name)
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?
could you rebase on top of master to see an updated codacy analysis?
merged latest master
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
hey, just force pushed. When I requested a rebase I meant:
i know what rebasing is and i used it myself, but i dont like to do it when it has merge commits or a simple merge of master or when someone else might have a copy of it already, it just gets messy
as asked, force-pushed a rebase onto latest master, while preserving merges (and proper signatures)
also dropped the renaming of TAG
variables commits
Yes, your force push broke things. It was ok when I force pushed.
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)
your last commit was left in the void.
Could you please fix that? Thanks
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 https://github.com/koreader/android-luajit-launcher/commit/908d6a70b82f5282cb99d8c8f98903e9afccc747, that was the last commit (that you had pushed after your rebase), where the parent is https://github.com/koreader/android-luajit-launcher/commit/2039ce161669394b4e080a3cf7fe8f8f7cb26711, which is still included
Yup, I'm at work and understood your comment https://github.com/koreader/android-luajit-launcher/pull/382#issuecomment-1249116057 wrong. All good as is.
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?
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 https://github.com/koreader/android-luajit-launcher/pull/382#issuecomment-1248400785
my local solution currently for that is: DeviceInfo.kt.patch
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.
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
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 ifTOLINO_EPOS
orTOLINO_VISION5
gets added first / last, and i had also already tried to comment-outTOLINO_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 whenTOLINO_VISION5
istrue
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 ;)
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:
*1: but as can be seen in https://github.com/koreader/android-luajit-launcher/issues/351#issue-1100647150, the Shine3 is listed as Hardware: e60k00
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 :)
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:
HashMap
to something that uses FIFO (unsorted) or do a custom implementationI stated that I wouldn't mind to break it. And prefeer to break it better than refactoring for a worse implementation or custom workarounds.
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?
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.
Please check it works fine that way. The rest of the PR is ready to go.
added with 2ea428e92b3ca2bcf6b816aebede4df3e0355c4f 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)
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 ;).
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)
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.
This PR adds support in
TolinoWarmthController.kt
to use/dev/ntx_io
to set the Warmthfixes #351
requires https://github.com/koreader/koreader/pull/9511This change is