joukos / PaperTTY

PaperTTY - Python module to render a TTY or VNC on e-ink
946 stars 101 forks source link

Fixes for 7in5v2 #51

Closed colin-nolan closed 4 years ago

colin-nolan commented 4 years ago

Unfortunately, the 7in5v2 configuration added by @badrihippo didn't work when I tried it on my device; the reset inherited from WaveshareFull does not seem to play with wait_until_idle.

This PR changes the reset used for this device to mirror that of the reference implementation: https://github.com/waveshare/e-Paper/blob/702def06bcb75983c98b0f9d25d43c552c248eb0/RaspberryPi%26JetsonNano/python/lib/waveshare_epd/epd7in5_V2.py#L48-L54. A quick scan of the other reference implementations indicates that the reset varies across devices so should prehaps not be common (it's unclear whether there is good reason for device specific differences or if the developer is just being inconsistent most of the time...).

I have also matched the wait_until_idle with the device specific implementation in the reference, as it is significantly different to the one that is inheritted.

Lastly, I have removed an undocumented, extraneous "VDHR" data send, which is not in the reference implementation: https://github.com/waveshare/e-Paper/blob/702def06bcb75983c98b0f9d25d43c552c248eb0/RaspberryPi%26JetsonNano/python/lib/waveshare_epd/epd7in5_V2.py#L86-L89. I am very willing to accept that there is good reason for this but I can't work it out!

joukos commented 4 years ago

It was nice to wake up with a bunch of pull requests waiting, thanks for your contributions!

I'll try to test this out as soon as I have some spare time, since I happen to have a 7in5v2 which is happy with the current code (as opposed to the ESP32 image viewer code which did not work even though I had the V2 selected in the interface...). Maybe the underlying reason has to do with this reset thing, I didn't look at the ESP32 code much, just used it to test if the display works, so result was disappointing :)

Now that I look at it, I think the sleep in the original method might be a bit too long (if not completely unnecessary in practice) - I think I originally put it there because the full refresh displays are so slow anyways and I didn't want to spend extra cycles while waiting.

Anyway, I'll test it and we'll see if it works for both of our displays.

colin-nolan commented 4 years ago

Great and thanks for your work on this project - it's really useful!

joukos commented 4 years ago

This seems to work with my display too, so we'll merge it (and hope that there's not some third variety that works with the other code and not this).

I have a feeling that instead of no sleep at all, having even a very small sleep might be better. I don't have time to benchmark if it makes any noticeable difference right now though, but mostly I'm thinking about the wait after the display refresh - won't it basically busy-loop the RPi until the display is done refreshing? Does it still work with your display if you put, say, 20ms of sleep there?

colin-nolan commented 4 years ago

I'm thinking about the wait after the display refresh...

I'm sorry: I don't understand what wait (or lack of wait) you're referring to?

joukos commented 4 years ago

I'm sorry: I don't understand what wait (or lack of wait) you're referring to?

I mean the wait_until_idle method - earlier it had a 100ms sleep in the loop:

    def wait_until_idle(self):
        while self.digital_read(self.BUSY_PIN) == 0:  # 0: busy, 1: idle
            self.delay_ms(100)

But now that it was changed, for the 7in5v2, it's (comments omitted):

    def wait_until_idle(self):
        self.send_command(0x71)
        while self.digital_read(self.BUSY_PIN) == 0:  # 0: busy, 1: idle
            self.send_command(0x71)

I assume this results in it being more busy during the wait, but without profiling it, can't really say for sure whether it's significant (for example with a measly RPi Zero). FWIW, I tried adding a 20ms delay there and it works for me, but my display worked with the earlier code too so it doesn't mean much :)

colin-nolan commented 4 years ago

Thanks for elaborating. If I understand correctly, your view is that the command send in the 7in5v2 has no functional purpose other than to achieve a busy wait? This could very well be correct - I have no idea what the result of repeatidly sending 0x71 to the device is. It is possible (although prehaps unlikely) that sending > n commands in a given timeframe has a functional purpose?

If I were to guess, I would assume the IO overhead of doing the send and read operations means that it's not going to stress the processor. I'm afraid I've not confirmed this suspicion yet though.

joukos commented 4 years ago

Well, I very quickly and naïvely tried adding a simple counter to the loop to just see how many iterations it does using A) 20ms delay B) no delay, and let it draw a couple of frames. With the delay, it did 191 iterations and without it, 17991 and 17934. So technically it does something about a hundred times more, but because of the overall complexity of scheduling and low-level details of how the GPIO library works that I'm not going to delve into right now, I'm not sure if it's noticeable in practice.

That said, if it works with the sleep, I feel that I'd prefer having it, since with these frame rates the potential delay in finishing an update is negligible and I'd like to minimize any idle overhead.

colin-nolan commented 4 years ago

With the delay, it did 191 iterations and without it, 17991 and 17934.

Wow! It seems very unlikely that there is a requirement to send the hardware that 0x71 command quite so often in order for it to function!

I will change to put in the sleep and confirm it works on my device (it seems likely it will).

colin-nolan commented 4 years ago

The 20ms works fine for me so I've added it in 512eda9.

joukos commented 4 years ago

Nice, thanks.

The build related files are work in progress, I presume (shouldn't the drivers and rest of the files be included too)? I think it would be better to add them as a separate PR to keep things orderly, as they're unrelated to this particular fix.

colin-nolan commented 4 years ago

Oh sorry, I did not mean to include those files! I will remove and re-push after work.

colin-nolan commented 4 years ago

Fixed, apologies again.

joukos commented 4 years ago

No worries :) I'll merge this now. Thanks for your work!