Support for Sony PRS-T* #724

protobits · 2018-09-02T15:26:45Z

This PR adds support for Sony PRS-T readers, which run a custom-made Linux distribution (based on Debian Jessie) which I named PRSTUX. I have a PRS-T2 only so I only that model is supported but most likely the other two models aren't two different. I have my ereader fully working with PRSTUX and koreader.

After this PR get merged I will make a PR for the main repo, with the submodule correctly pointing to this repo's current commit.

Please note that I had to include a fix in libjpeg-turbo/CMakeLists.txt file, since it wouldn't detect my architecture as ARM (since the check was specialized for armv6) and also CXX flags where not correctly set (only C flags were).

Frenzie

lgtm

Frenzie · 2018-09-02T15:42:47Z

ffi/mxcfb_sony_h.lua

+static const int WAVEFORM_MODE_A2 = 4;
+static const int WAVEFORM_MODE_AUTO = 257;
+static const int TEMP_USE_AMBIENT = 4096;
+static const int TEMP_USE_AUTO = 4097; // this does not exist, simply use a value different than above


I'm not quite sure I follow?

You mean the comment? I meant there's no actual AUTO constant for temperature in the headers, but looking at the code, if it doesn't receive a AMBIENT setting it will use the temperature sensor. Thus, I simply used a different value for that constant.

Okay, thanks! :-)

Huh, that's interesting ;).

I'd add the matching 0x1001 define in the header like we do on Kobo/Kindle, just so that it doesn't get zapped if someone decides to re-run gcc-lua one day ;).

protobits · 2018-09-02T15:48:42Z

PS: I realized I should've added a CI entry for this. Once I figure that out I can make a PR for that.

Frenzie · 2018-09-02T15:52:54Z

Ideally you'd probably have to create a Docker image, see:

koreader-base/.travis.yml

Lines 21 to 23 in 22d94e9

    
           - TARGET=kindle DOCKER_IMG=frenzie/kokindle:0.0.5 
        
           - TARGET=kobo DOCKER_IMG=frenzie/kokobo:0.0.5 
        
           - TARGET=pocketbook DOCKER_IMG=houqp/kopb:0.0.1

Those are from https://github.com/koreader/virdevenv/tree/13c37dcc21d6e8acffe53e0d88e2a6e3562bd169/docker/ubuntu

In lieu of that it'd also work to use the pre-Docker situation, see e4f185b#diff-354f30a63fb0907d4ad57269548329e3

However, the Docker images are used on GitLab to auto-build the nightlies as well.

poire-z · 2018-09-02T16:19:00Z

These input-prst, -DPRST refresh_prst ... strings sounds a bit anonymous to those who don't know the Sony devices ecosystem, and they could go on thinking it's an obscure compiler option or that it means POWER_RESET or People Republic of South Tanzania :)
Could it be possible to name them just input-sony, -DSONY, refresh_sony so all targets are named a manufacturer name that speaks to anybody?
Or if there is a chance Sony does other devices than PRST architectured differently, input-sony-prst, -DSONYPRST, refresh_sony_prst ?
Or -DSONYPRSTUX, if all that really depends on your PRSTUX OS?

protobits · 2018-09-02T16:24:21Z

Sony manufactured older readers which I didn't even intend to support, like PRSnnn. The most recent line was PRS-Tn (I guess because they had touch input). These later devices are mostly similar.
I also wondered how to name it, to make it specific to those supported models and not simply name the company which produced them.

If indeed you'd prefer a rename, I guess it would be better no name it prstux, instead of prst, since I'm depending custom OS anyway. Please let me know if you really prefer the rename, since I would have to change lots of names also on koreader repo.

poire-z · 2018-09-02T16:47:12Z

I indeed would prefer to see "sony" in all that, whether followed by PRST or PRSTUX or PRSTux.
For the frontend code, device:isSonyPRST or device:isSonyPRSTUX or device:isSonyAnythingYouLike
But let's see what @Frenzie and @NiLuJe think.

Frenzie · 2018-09-02T17:26:14Z

I agree that something closer Sony PRST would be less mysterious at a glance.

NiLuJe

A few remarks, but generally okay ;).

I have no issue with the PRST naming, but then I'm used to it from the MobileRead days ;).

NiLuJe · 2018-09-02T17:21:10Z

Makefile.third

@@ -38,10 +38,13 @@ $(TURBOJPEG_LIB) $(JPEG_LIB): $(THIRDPARTY_DIR)/libjpeg-turbo/CMakeLists.txt
 	cd $(JPEG_BUILD_DIR) && \
 		$(CMAKE) \
 		-DCMAKE_VERBOSE_MAKEFILE:BOOL=OFF \
-		$(if $(findstring armv6, $(ARM_ARCH)),-DWITHOUT_SIMD:BOOL=ON,) \
+		$(if $(findstring arm, $(ARM_ARCH)),-DWITHOUT_SIMD:BOOL=ON,) \


Nope, I want SIMD enabled on armv7 ;).

If you had a build issue related to ARM detection, that's to be addressed in the crappy CMakeFile for libjpeg-turbo (there's a bunch of eInk & Android specific cases, it should be somewhat obvious, ping me if that looks too arcane ;)).

Namely, you probably need to add PRST the ginormous ENV(...) branch ;).

NiLuJe · 2018-09-02T17:22:11Z

Makefile.third

 		-DCHOST="$(CHOST)" \
 		-DCFLAGS="$(CFLAGS)" \
 		-DLDFLAGS="$(LDFLAGS)" \
+		-DCMAKE_CXX_COMPILER="$(CMAKE_CXX_COMPILER)" \


Nope, libjpeg-turbo is C only, we shouldn't need that.

If CMake complains, you have a CMake toolchain setup snafu somewhere earlier ;).

NiLuJe · 2018-09-02T17:23:40Z

ffi/framebuffer_mxcfb.lua

+        self.mech_refresh = refresh_prst
+        self.mech_wait_update_complete = prst_mxc_wait_for_update_complete
+
+        self.waveform_fast = C.WAVEFORM_MODE_DU


Unless there's a platform-specific issue with A2 (like there is on REAGL), we usually go with A2 instead of DU for that ;).

I think I remember A2 being slower than DU, but I can re-check.

In theory, it should be the fastest you can go, at the expense of quality (being basically pure B & W).

DU is more lenient, it handles a few shades of gray, and as such should be a tiny bit slower.

Since we only use this for pure B&W elements (mainly menu/button highlights), if A2 works, that's better.

In practice, there's a few cases where A2 doesn't behave as expected, but that's usually limited to REAGL screens, which I think the T2 predates, so that shouldn't be the case here ;).

NiLuJe · 2018-09-02T17:24:26Z

ffi/framebuffer_mxcfb.lua

+
+        self.waveform_fast = C.WAVEFORM_MODE_DU
+        self.waveform_ui = C.WAVEFORM_MODE_AUTO
+        self.waveform_flashui = C.WAVEFORM_MODE_GC16


In the same vein, unless there's a specific platform issue where AUTO doesn't always honor UPDATE_FULL, this should be AUTO too.

EDIT: answered above comment here

ok, i will change to AUTO.

NiLuJe · 2018-09-02T17:25:42Z

ffi/mxcfb_sony_h.lua

+static const int WAVEFORM_MODE_A2 = 4;
+static const int WAVEFORM_MODE_AUTO = 257;
+static const int TEMP_USE_AMBIENT = 4096;
+static const int TEMP_USE_AUTO = 4097; // this does not exist, simply use a value different than above


Huh, that's interesting ;).

I'd add the matching 0x1001 define in the header like we do on Kobo/Kindle, just so that it doesn't get zapped if someone decides to re-run gcc-lua one day ;).

protobits · 2018-09-02T21:17:16Z

Ok, I will then rename everything to sony_prstux or sony_prst. I will also look into reverting those changes to libjpeg-turbo cmakelist and see what is wrong with the toolchain that may have triggered that.

I'm building a Docker image for building the code, which could be later used for CI. Should I base it on the same base image used for those on the virdevenv repo? Or could I roll my own? I see you use gcc 4.8 and I was using stock ARM 4.9 (cross compilation) found on Ubuntu 16.04, since it matches that of ARMHF Debian Jessie.

NiLuJe · 2018-09-02T21:20:43Z

Match the TC of your target device if possible, yeah. (c.f., https://github.com/koreader/koxtoolchain for the Kindle/Kobo TCs, for instance).

protobits · 2018-09-02T21:42:19Z

Ok, so here's the error I get if I revert the changes. I see a mix of i386 and arm compilation. I don't fully understand why or how it happens. What I do see is that CMakeLists grabs gcc for ARM but g++ for x86. Also, SIMD appears to mean SSE and not NEON. That's why I originally assumed I did not want SIMD for any arm arch.
What could be wrong ?

https://gist.github.com/v01d/33cdf5879b1aed33dc844d03e2ea0eff

protobits · 2018-09-02T22:31:16Z

@NiLuJe I think I traced it back to CMAKE_SYSTEM_NAME not being set for PRST. I added the check $ENV{PRST} but it does not seem to be set (I'm exporting it as you see in the Makefile).

NiLuJe · 2018-09-02T23:13:53Z

Yeah, that's what happens when CMAKE_SYSTEM_NAME/CMAKE_SYSTEM_PROCESSOR aren't set or mismatched w/ libjpeg-turbo, it mysteriously decides it wants to build x86 assembly :D.

Adding an OR ($ENV{PRST}) to that check should be enough, provided the CHOST also matches arm-*, which should be the case here...

EDIT: And yes, that export in Makefile.defs looks perfectly fine.

NiLuJe · 2018-09-02T23:16:07Z

Try a new build from a clean tree? I've had some weird & unreproducible issues when hot-swapping CMake configs...

protobits · 2018-09-03T00:22:22Z

@NiLuJe so it appears that there's an issue with checking environment variables like that. I can only get it to work by doing something like if(DEFINED ENV{PRST}). I found related issues on google, see for example:
https://itk.org/Bug/view.php?id=14737

It seems adding the '$' is problematic and particularly checking environment variables. Then again, this issue might be due to some cmake version. I have 3.5.1 on my docker image.
Should I change these tests to DEFINED as above?

NiLuJe · 2018-09-03T00:52:38Z

I had the exact same issue when I initially started writing this using DEFINED (but then I was doing it wrong at the time :D), so I'm not terribly surprised about it ;).

If it works for you and doesn't break the CI, go for it ;).

I'm on Gentoo here, so a fairly current CMake version (3.12.1, FWIW).

We're doing a metric-ton of tests this way, though.

NiLuJe · 2018-09-03T00:55:25Z

IIRC, there might be a similar issue with the CHOST expansion in the regex, I tweaked the syntax a few times until either I stopped doing it wrong and/or it happened to play nice with the various CIs.

protobits · 2018-09-03T12:32:59Z

Hi, I addressed the aforementioned issues in my last commit. I also added a CI entry using my docker build image. This failed due to some dumb issue in the Docker file which I just fixed. If you want/can re-trigger the job I think it should run OK now.

Next step is the rename.

poire-z · 2018-09-03T13:14:31Z

(prst build restarted)

NiLuJe

ACK.

That just leaves the minor A2/AUTO waveform mode tweaks ;).

poire-z · 2018-09-03T18:11:55Z

In case you missed it: the 2nd build run failed too. Reason:

cleaning up crengine checkout...
[sudo] password for ko: 
No output has been received in the last 10m0s,

NiLuJe · 2018-09-03T19:51:24Z

(I launched it a 3rd time and it failed the same way, so not a quirk ;)).

protobits · 2018-09-03T22:38:37Z

Sorry, those changes were left on my reader when testing and did not made the commit. Just pushed that. I also fixed the docker image (supposeddly).

NiLuJe · 2018-09-04T01:46:41Z

autogen.sh: error: could not find libtool. libtool is required to run autogen.sh.

(I don't know if we have a real documentation for how to setup those Docker images, but in any case, feel free to jot down what worked/didn't work somewhere... ;)).

protobits · 2018-09-04T01:53:14Z

Thanks! I saw the error as well. I think I just solved that also. I'm pushing here the rename and see if this time builds OK.

NiLuJe · 2018-09-04T02:38:05Z

👍 LGTM!

protobits · 2018-09-05T02:06:29Z

Cool, thanks! After this is merged I will make the PR on koreader repo.

Frenzie · 2018-09-08T05:23:49Z

@v01d My bad because I only implied it a bit by linking to the virdevenv repo rather than explicitly saying it, but could you please also submit a PR for your Docker image here?

https://github.com/koreader/virdevenv/tree/master/docker/ubuntu

protobits · 2018-09-08T13:10:20Z

Ok, will do soon.

Support for Sony PRS-T*

34c2b50

Frenzie approved these changes Sep 2, 2018

View reviewed changes

Frenzie requested a review from NiLuJe September 2, 2018 15:43

Frenzie added the enhancement label Sep 2, 2018

NiLuJe requested changes Sep 2, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

fixes

221f9a9

NiLuJe requested changes Sep 3, 2018

View reviewed changes

PRST: switch to A2 and AUTO modes for fast and flash_ui

c3e3c32

renamed prstux -> sony_prstux (and all casings and combinations)

8f797aa

NiLuJe approved these changes Sep 4, 2018

View reviewed changes

NiLuJe merged commit d621bbd into koreader:master Sep 5, 2018

Frenzie mentioned this pull request Oct 28, 2018

Sony PRSTUX missing koreader/virdevenv#25

Closed

Support for Sony PRS-T* #724

Support for Sony PRS-T* #724

Conversation

protobits commented Sep 2, 2018 • edited Loading

Frenzie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

protobits commented Sep 2, 2018 • edited Loading

Frenzie commented Sep 2, 2018

poire-z commented Sep 2, 2018

protobits commented Sep 2, 2018

poire-z commented Sep 2, 2018

Frenzie commented Sep 2, 2018

NiLuJe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

protobits Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

protobits commented Sep 2, 2018

NiLuJe commented Sep 2, 2018 • edited Loading

protobits commented Sep 2, 2018 • edited Loading

This comment has been minimized.

protobits commented Sep 2, 2018

NiLuJe commented Sep 2, 2018 • edited Loading

NiLuJe commented Sep 2, 2018

protobits commented Sep 3, 2018

NiLuJe commented Sep 3, 2018 • edited Loading

NiLuJe commented Sep 3, 2018

protobits commented Sep 3, 2018

poire-z commented Sep 3, 2018

NiLuJe left a comment

Choose a reason for hiding this comment

poire-z commented Sep 3, 2018

NiLuJe commented Sep 3, 2018

protobits commented Sep 3, 2018

NiLuJe commented Sep 4, 2018

protobits commented Sep 4, 2018

NiLuJe commented Sep 4, 2018

protobits commented Sep 5, 2018

Frenzie commented Sep 8, 2018

protobits commented Sep 8, 2018

protobits commented Sep 2, 2018 •

edited

Loading

protobits commented Sep 2, 2018 •

edited

Loading

NiLuJe Sep 2, 2018 •

edited

Loading

NiLuJe Sep 2, 2018 •

edited

Loading

protobits Sep 2, 2018 •

edited

Loading

NiLuJe commented Sep 2, 2018 •

edited

Loading

protobits commented Sep 2, 2018 •

edited

Loading

NiLuJe commented Sep 2, 2018 •

edited

Loading

NiLuJe commented Sep 3, 2018 •

edited

Loading