rien / reStream

Stream your reMarkable screen over SSH.
MIT License
734 stars 56 forks source link

Detect more rM 1 devices with debatable changes #36

Closed LinusCDE closed 3 years ago

LinusCDE commented 3 years ago

Hi,

since I heard of the recent success of reading rM 2 devices, I revisited the project once more (had used a version that was a few months old). Since I saw the rust files and am currently really into it, I took a look and found that you also had the problem of detecting the device version.

Currently some rM 1 device will not get detected since there is (to my knowledge) one additional name for reMarkable 1 devices. See @Eeems' comment (highly recommended issue to gain information regarding all kinds of rM 2 changes). He should also be able to check it and see that the binary will actually not work on his device.

Other than simply adding reMarkable Prototype 1 to the list, I chose to use the detection from libremarkable. I added the support there as part of implementing rM 2 input for libremarkable (PR) and this implementation should get updated whenever we encounter an unsupported device. (The pr also has some discussion about what approach of model detection would be best).

While libremarkable::framebuffer::common provides DISPLAYWIDTH and DISPLAYHEIGHT, I didn't use them since your code is pretty readable and short and this change would harm that. They are also just constants and a potential rM 3 with a new size would introduce a breaking change there anyway (most likely constant -> lazy_static which needs a * in front).

Advantages:

Disadvantages:

*2 This was my initial design. I see this can be improved. Some ideas would be simply making Model::current_model() pub or handling the Unknown model variant in later functions in device and not failing upon creation. I used the Unknown variant since it is used everywhere else in the lib as well, but think I may have done a poor choice in using this variant. Feel free to fix it in a PR to libremarkable.

If you would rather handle the versioning yourself, just tell me and I'll add the extra version name instead of using libremarkable here. As said in the title, it is debatable since adding a full fat library for such a simple task can very well be considered overkill.


Side note 1: I would probably be best practice to add the Cargo.lock file as well. This is useful when someone will try to builds this a couple of years in the future and can't since he doesn't know the specific versions used. The general wisdom in rust is, that libs do not check this file in but bins do as far as I understand it.

Side note 2: Maybe it's worth here to strip the binary. Example: source /usr/local/oecore-x86_64/environment-setup-cortexa9hf-neon-oe-linux-gnueabi && $STRIP target/armv7-unknown-linux-gnueabihf/release/restream. This reduces the size from 3,4 MB to 354 KB (90%).


P.S.: Just wanted to say that I really appreciate this project. Before it came out, I did a few tests and thought that streaming videos was not doable since the processing would've been either too much or the bandwidth to low. The framerates you achieved by simply using lz4 blew my mind!

rien commented 3 years ago

Hi @LinusCDE, thank you for the detailed and very informative PR! It's a great idea to centralize the reMarkable version strings, but with the disadvantages you mentioned I was a bit hesitant.

I have added a PR to improve the error handling of Model::current_model(). If that is accepted, I would be glad to incorporate this in the restream code.

Also thanks for the Rust tips! I've had some experience with Rust development, but I'm glad I still have to learn some things.

LinusCDE commented 3 years ago

Also started using rust about a year ago. Took me a few months to really start with it and then some to get comfortable with it. I'm halfway through the rust book so I still have to learn more about advanced traits.

I also added a note to the PR you opened. It was an oversight from me, but having current_version() public will actually make the error usable by other devs. I think I kept it private since it was not used anywhere else and I thought nobody would need to. But not being bound to a panic is probably a good reason to have it public.

rien commented 3 years ago

Once https://github.com/canselcik/libremarkable/pull/54 is merged, please create a new PR on the main branch.