TomWhitwell / SlowMovie

MIT License
342 stars 67 forks source link

Multiple Displays #64

Closed robweber closed 3 years ago

robweber commented 3 years ago

This is my second draft at this. The first one was way too cumbersome and didn't fit with the focus of this project. This one takes more of a library approach and involves minimal changes to the codebase here.

Goal

Provide a way to allow multiple display types to be used with slowmovie.py with the most minimal impact to the developing codebase here. The solution should be user friendly and not require end users to muck about changing python files.

Solution

I've spun off a separate repo specifically for abstracting EPD drivers for "vsmp" (very slow movie player) projects. Right now it will handle a mock display (for testing without hardware) and any Waveshare devices in the Waveshare e-Ink repo. Within the next few days I hope to add Inky displays as well, just waiting for my Inky hardware to arrive.

vsmp-epd is published to PyPi and is available as a dependency. This makes it easy to integrate with this project. You can see from the changes to slowmovie.py that it's almost a 1:1 replacement in terms of interacting with the library. The main 7.5in display from the instructions is the default but using the -e argument you can specify any compatible display.

Pros/Cons

The big pro here is this keeps all the E-ink abstraction code separate from this project. The goal here is to provide the media player, not drag along the interaction code for the displays. It also allows for specifying different display types pretty easily for the end user.

The major con is mainly Waveshare itself. Because it's not available as a pip install it will still be a manual build per the install instructions. I can include the Inky libraries as dependencies with vsmp-epd but Waveshare will need to have it's own build instructions (unless you're not using that type of display).

Fun Note: I've been running this abstraction code for 2 days now with no issues on my home display.

robweber commented 3 years ago

Just wanted to ping this PR and see if there are any thoughts on it. Is this a direction anyone is interested in this or are we pretty much just supporting the existing device (hardcoded) and directing people to roll their own via a fork if they need to do something different?

I've made some pretty good progress on the abstraction library and have basic Inky support hammered out. My plan is to reach out to a few of the known forks where people have done other devices and ask them for a little help in testing to make sure something obvious isn't being missed. Should make this a pretty robust solution.

missionfloyd commented 3 years ago

Looks like a good way to expand display support without messing with much of the code here.

I do have one (very minor) thought. Why name the module vsmp-epd? Surely this is suitable for any e-ink project.

qubist commented 3 years ago

Seems like this would close #62 (EDIT: and #24 and #5?)

robweber commented 3 years ago

I'll take a look at resolving those suggestions - they all look good.

In the abstraction repo I added support for device specific config files to simplify some post-image processing. The idea is something like: what if you mounted your display in such a way that all images were upside down (due to case or other physical limitations) or for Inky displays you can use 3 colors instead of 2. With the ini file a user can create a rule that's applied every time the epd.display() function is called. This will automatically rotate, or whatever, without having to have all sorts of options for that code as part of the slowmovie.py file. Keep the "display" separate from the "player".

I also added a CLI testing utility for people to quickly test their setup. Basically vsmp-epd-test -e devicename. This is just a quick check that they have all the system drivers and things in place. It's installed with the module so available as a system command.

robweber commented 3 years ago

Modified per the suggestions above.

I have been thinking about the library name a bit. @missionfloyd is right - this is generally suited for any e-ink display project where you want to support multiple displays. If a rebrand is a good thought it should be done prior to merging anything into this project since it will affect naming.

Any thoughts? Something like universal-epd or abstract-epd? Naming things isn't my strong suit but I think it should be descriptive and not something random like a character name.

qubist commented 3 years ago

Maybe something like multi-epd. The multi-epd library. I think that gets across the essence of the library: supporting multiple e-paper displays in the same library.

missionfloyd commented 3 years ago

omni-epd?

robweber commented 3 years ago

Both of those are pretty good omni sounds pretty cool so I'll run with that. I'll make the necessary changes to the library and then update this PR. So far there is support for:

I'd like to get more confirmed hardware tests but a problem occurred to me. I can find lots of forks where people have modified code to work with other displays (Inky and Waveshare). The issue is you can't send messages on Github so there isn't any way to reach out and ask them for help testing anything. If anyone has ideas on that front that would be great.

qubist commented 3 years ago

The issue is you can't send messages on Github so there isn't any way to reach out and ask them for help testing anything.

Can you add an issue on their fork explaining the situation and asking for testing?

EDIT: seems like you can't add issues on forks. You can add a PR though, that could be an idea. Draft PR that's just for discussion? Very work-around-y, but it might work. One other idea: go to their profile and hope they put an email there? And then hope the people who modified the code to work with another display were also people who put an email on their profile.

missionfloyd commented 3 years ago

It seems to me that this will fail on 3-color displays. They take two images (their drivers could really use some work.)

robweber commented 3 years ago

@missionfloyd - Well that's not good. My oversight there. I looked at a good handful of those classes and they all appeared to be the same format. It troubled me a bit they didn't inherit methods from a parent class but thought it was just laziness on their part. Guess there was a reason for that.

Two options really that I can see:

1) just remove those devices from the approved list for now 2) create some special handling for those in the abstract class

I think it could be handled fairly well by taking the Image passed in and performing some transforms on it. It could be stripped to 3 colors (black/white/red) and then separated again so color 1 went to the black image and color 2 to the red image. The inky library does something similar for their Impression displays to convert an image to 7 colors only. Without a device to test it on though I'd be a little apprehensive about just writing that code and calling it ready.

I created an issue for this if you want to continue this discussion there on possible solutions.

robweber commented 3 years ago

After a lot of work in the omni-epd repo @missionfloyd and I have gotten this abstraction library working pretty well. It should handle pretty much any of the waveshare devices as well as the Inky devices; a full list is here. There is also support for different modes on displays that offer more than B/W as a color option. This allows the owners for those types of displays to utilize all the options the display might have without changing any of the code here.

I did a clean Raspberry Pi OS install and setup all the library requirements from scratch using the instructions in this branch; which are just the normal instructions + adding the omni-epd library. Everything installed great and was working. Before merging this in I'd encourage more testing if possible. Cloning this branch and then running: sudo pip3 install git+https://github.com/robweber/omni-epd.git#egg=omni-epd or sudo pip3 install -r requirements.txt (with the updated requirements file here) will install the library on your Rpi. For the default Waveshare 7.5in display just running Slowmovie like normal will work. For other displays just pass in the name via the -e option. Names are given in the support display list above.

qubist commented 3 years ago

Would it still help for me to test this even thought I only have the one display?

robweber commented 3 years ago

@qubist - Testing even with the default display is still a help. I may have missed something that prevents it from loading or maybe the instructions aren't clear. Tried to make it "plug n play" but there is always some weird edge case.

qubist commented 3 years ago

Ok: cloned the branch, went into Install dir, ran sudo pip3 install -r requirements.txt, ran the install script for good measure, then ran the test. Works fine on my display. I made sure that the -e option existed to validate I had the right version running. One thing I did run in to:

Screen Shot 2021-04-30 at 12 12 29 PM

I cloned the branch into a differently-named directory (SlowMovie-Multi) because I wanted to leave my old one alone. The install script told me to run the wrong file. I'm also seeing now the whiptail menu told me about that:

                 │  Repo set to 'https://github.com/TomWhitwell/SlowMovie/main'     │
                 │  Setting up in local directory '/home/pi/SlowMovie'              │

Maybe I just shouldn't have done this and nothing needs to be changed?

robweber commented 3 years ago

The install script assumes /home/pi/SlowMovie is the directory you're updating. You probably just ended up re-running a pull on your existing install. Usually I'll rename my working directory to SlowmovieKeep or something and then clone the branch I'm working on so the install script runs properly. When I'm done I delete it and rename back. Running the pip command seems have brought in the required libs though so seems like it's all working OK right?

qubist commented 3 years ago

@robweber Yeah, I think my run of the install script just did nothing related to this PR. The pip command brought everything in fine I believe, although—if it failed to, would I see an error without trying to run things with a non-default screen?

robweber commented 3 years ago

You'd definitely have problems when trying to load omni-epd if things weren't working right. That import would fail right out of the gate before the rest of slowmovie.py had a chance to do anything. One thing you could try is just giving it a bogus value - like -e test. That should fail out and print a list of supported displays since there isn't a match.

The test utility omni-epd-test is also available as a program from the CLI, running that would confirm the libraries are working. Just run that from anywhere.

robweber commented 3 years ago

I moved the default waveshare device to the conf file and removed the default for -e in the main program. This should allow the user to use basically any combination of the conf file, CLI, or omni-epd.ini file to set their default. Also updated the README to add the other INI file values that could be used. I left the contrast option for now, we can always remove in another PR just to keep this from modifying too many pieces.

robweber commented 3 years ago

Looking through this I think we're ok. All major issues should be addressed.

qubist commented 3 years ago

Yup, looks good!