peterhinch / micropython-nano-gui

A lightweight MicroPython GUI library for display drivers based on framebuf class
MIT License
508 stars 87 forks source link

driver implementation for waveshare 4.2" rev2.2 #64

Closed surdouski closed 4 months ago

surdouski commented 6 months ago

Couldn't find a working driver anywhere online for micropython (or any other language) for partial refresh for the waveshare 4.2" (rev2.2), so I wrote one. In all the partial refresh code I can find anywhere, there is a bug in it (writes the waveform command twice instead of the second one needing to be the data entry command). All the materials I could find copy the things written verbatim from the C-source, and I suppose no one tested it either, so all the materials I could find have that bug.

I did not test the fast or 4gray, so they are commented out below. I might have use for them in the future, but I am a software developer and I seem to get caught up really easily on small things that I'm guessing wouldn't be a problem for actual firmware developers.

Also, just wanted to say thanks for all your work on the micropython libraries/utilities.

UPDATE: There was some weird timing bug inside of the partial refresh, which I was unable to get rid of because I don't know how it's being introduced. The current version works though, confirmed that the partial refresh on the aclock demo successfull refreshes without a full screen refresh on each tick of the hand moving.

peterhinch commented 6 months ago

I'm not clear what hardware you are using. The reference in the code seems to point to a bare EPD. Are you using this version with a built-in controller?

Please can you explain the benefits of the hardware you're using compared to the already supported 4.2" display. This is an excellent unit with low ghosting and is compatible with a wide range of hosts. At present my recommendation is to use this unit.

I have a difficulty accepting a PR for hardware which I don't possess - it makes it hard or impossible for me to deal with requests for support. If you can make a good case for the hardware you are using I might consider buying one.

surdouski commented 6 months ago

The reason I bought this hardware initially is because I thought it was fully supported by your library. Unfortunately the listings are confusing, and as someone that has only briefly entered the hardware space, I was convinced that I was receiving the model mentioned in your current driver. Also, I made this mistake[?] twice; two different suppliers both shipped me this version instead of the version I expected to get.

The link that you mentioned, "this version," appears to be the one. On the back of the device you can see the "Rev2.2" on the back of the screen underneath the word "Module" and above the "v2" sticker.

I do respect the fact that you don't want to accept a PR when you can't test it yourself; it's part of the reason I probably find your repo's so reliable. I would have a hard time making a case for it with my experience -- the best case for supporting it would be that it is confusing trying to find the device you do currently support.

Edit: figured i'd an include an image of this one's ghosting since you mentioned ghosting of the other one. this is almost a full rotation's worth (you can see the small area where no ghosting yet). image

peterhinch commented 6 months ago

For future reference this doc lists supported displays with links to manufacturers' sites.

Ghosting is a perennial problem with these displays. The one I reference above has the lowest level I've yet seen. In the early days ghosting was awful and refresh times were long, so I designed this clock display. It has the attribute that changes are additive so ghosting cannot occur. Once per hour a full refresh takes place. It's still available as part of nano-gui eclock.py, if only because every engineer has to invent a clock at some point in their life :)

As for the PR, I'm afraid it's "thanks but no thanks", for the reason stated and because the 4.2" display that I do support is so good.

peterhinch commented 4 months ago

In the light of https://github.com/peterhinch/micropython-nano-gui/issues/68 I'm reopening this. I'm trying to procure a V2 display to evaluate/test.

surdouski commented 4 months ago

I remembered that my local revisions were more correct than the previous iteration. Sorry that it's so messy at the moment, I haven't cleaned it up at all; I'll see if I can get around to that soon.

peterhinch commented 4 months ago

The code has the following comment at the start which I don't understand. It seems to imply that partial refresh doesn't work, while your photo shows that it clearly does work. If the Waveshare code is broken, where did you find information to correct it?

# Note, at the time of writing this, none of the source materials have working
# code that works with partial refresh, as the C code has a bug and all the other
# materials use that reference material as the source of truth.
peterhinch commented 4 months ago

How would you like to proceed? I've had a preliminary look at the code and there are a few things that I'd query. The usual approach is for me to do a detailed code review with you implementing any agreed changes. Do you have the time and inclination to do this or would you rather I take the code as written and develop from there?

surdouski commented 4 months ago

The code has the following comment at the start which I don't understand. It seems to imply that partial refresh doesn't work, while your photo shows that it clearly does work. If the Waveshare code is broken, where did you find information to correct it?

This comment is in not in reference to the code I had written. The C code and any libraries that used that C code as reference (including any python code) were all incorrect. I believe I had to look at the datasheet to determine what the correct commands were. I could investigate further, but my memory is failing to give me more detail at the moment.

How would you like to proceed? I've had a preliminary look at the code and there are a few things that I'd query. The usual approach is for me to do a detailed code review with you implementing any agreed changes. Do you have the time and inclination to do this or would you rather I take the code as written and develop from there?

I would be happy to take the approach of review and me implementing any agreed upon changes. I'm still early on in my learnings of firmware, but I'm unemployed at the moment so I have free time to spare. If you'd prefer, I can try to clean things up a bit more first to make it a bit more coherent -- the recent push I made were changes made after you had declined the PR, so I hadn't expected anyone to see them (but they are working).

peterhinch commented 4 months ago

I would be happy to take the approach of review and me implementing any agreed upon changes.

Excellent.

I have ordered a V2 board direct from Waveshare. This struck me as the only way to be sure of getting a V2 board, but I've no idea how long delivery is likely to take. So I can't contribute much other than general observations. Here are some initial thoughts based on a fairly cursory examination.

  1. I agree with your comments re the commented-out init_fast code. Speed of instantiation is not an issue. I think we can lose that code.
  2. It would be good if the greyscale code worked based on a 2-bit framebuffer as per pico_epaper_42_gs.py. You'd need a constructor arg to specify greyscale mode. I wonder if partial updates work in greyscale mode?
  3. _as_show needs attention. Currently the PR yields after every line, so a full refresh will yield 300 times. If you had another task that blocked for 20ms it would push the refresh time out to 6s. I used to yield every 16 lines to mitigate this, but I have just pushed an update that measures the blocking time and only yields when a threshold is exceeded. See new code. I suggest we adopt this approach as it's more platform-independent and is user configurable.
  4. The do_refresh method is needed for micro-gui, complete with its unused arg.

What chip does the display use? I assume it's not a UC8176 judging by the lower baudrate, I'd like to grab a datasheet.

peterhinch commented 4 months ago

Re greyscale I can see three options:

  1. Specify greyscale as a constructor arg.
  2. Subclass the main driver.
  3. Create separate 1-bit and 2-bit drivers as per V1.

I'll leave it up to you to decide - I'd be happy with any of these options. Or you may want to ignore greyscale and just write a 1-bit driver.

In terms of the general way forward I'll step back until you have some code ready for review. Feel free to continue the discussion if there are any issues that crop up.

peterhinch commented 4 months ago

Re greyscale I can see three options:

  1. Specify greyscale as a constructor arg.
  2. Subclass the main driver.
  3. Create separate 1-bit and 2-bit drivers as per V1.

I'll leave it up to you to decide - I'd be happy with any of these options. Or you may want to ignore greyscale and just write a 1-bit driver.

In terms of the general way forward I'll step back until you have some code ready for review. Feel free to continue the discussion if there are any issues that crop up.

surdouski commented 4 months ago

What chip does the display use? I assume it's not a UC8176 judging by the lower baudrate, I'd like to grab a datasheet.

I'm seeing... YE08 01K AOJK ? Attaching image because I'm not sure which are the significant numbers or if I'm reading them correctly. image

surdouski commented 4 months ago
  1. I agree with your comments re the commented-out init_fast code. Speed of instantiation is not an issue. I think we can lose that code.

:+1:

  1. _as_show needs attention. Currently the PR yields after every line, so a full refresh will yield 300 times. If you had another task that blocked for 20ms it would push the refresh time out to 6s. I used to yield every 16 lines to mitigate this, but I have just pushed an update that measures the blocking time and only yields when a threshold is exceeded. See new code. I suggest we adopt this approach as it's more platform-independent and is user configurable.

Updated to use this. I have noticed in my code that I have code that is fairly split between asynchronous updates vs full updates. I think my issue is that my "partial" and "full" code are separated in a way that is very confusing. I will see if I can reason out the logic for updates vs setting and displaying the updates -- although I do remember this being hard for me last time, as whenever I tried changing the way I ran the commands, the epaper did not display correctly.

  1. The do_refresh method is needed for micro-gui, complete with its unused arg.

:+1: Hopefully I did this right, I only have it using _as_show_full until I figure out the thing in the previous comment.

surdouski commented 4 months ago

Re greyscale I can see three options:

  1. Specify greyscale as a constructor arg.
  2. Subclass the main driver.
  3. Create separate 1-bit and 2-bit drivers as per V1.

I'll leave it up to you to decide - I'd be happy with any of these options. Or you may want to ignore greyscale and just write a 1-bit driver.

I haven't touched this yet because I think the split between the "full" and "partial" logic and it's resolution could change my opinion on what the best way to do this is. However, my current thought is that "create separate 1-bit and 2-bit drivers as per V1" is probably best because it's already the established paradigm.

surdouski commented 4 months ago

My previous comment on confusion over partial vs full refresh hopefully is mostly subdued with this recent commit. I'll wait here for any feedback, otherwise I can see about getting greyscale to work.

Side note: I'll remove any excess print statements at the end, for now I find them very helpful in debugging and I'd have to keep removing and including them otherwise.

peterhinch commented 4 months ago

Having thought about this some more, I fully agree with separate drivers for 1-bit and 2-bit modes. I suggest we get the 1-bit driver released first. On this basis the commented out 4gray code can be eliminated from the 1-bit driver. Have you considered using my _linv method (or similar) for fast inversion? An aim is to minimise latency when a micro-gui user performs an operation like pressing a button.

I am rather puzzled by set_full. Firstly why do we need the two args (absent in the V1 driver)? Secondly, resetting the chip seems rather drastic. The Waveshare code doesn't seem to do this. Resetting the chip may cause the mode change to be slow.

I'm also puzzled by the partial=False constructor arg. The V1 driver initialises the display in full mode. In general applications clear a display on initialisation - which implies full mode. On startup micro-gui does this automatically. I don't object to it given the default - but can you foresee an application which would want old display content to be retained on power-up?

Re the chip, I'm 99% sure that the chip you photographed is a level shifter for the white connector. The wiki indicates that a level shifter is provided for (5V) Arduino, and the PCB traces show links to the white connector. I think the controller chip is on the flexible PCB out of sight. The wiki includes a spec for the UC8176 plus another unspecified chip spec but neither seem to correspond with their own code. I'll try raising a request with Waveshare.

surdouski commented 4 months ago

I am rather puzzled by set_full. Firstly why do we need the two args (absent in the V1 driver)? Secondly, resetting the chip seems rather drastic. The Waveshare code doesn't seem to do this. Resetting the chip may cause the mode change to be slow.

Removed the args. I was confused on my first go when writing this driver, as I hadn't done so before. The fact that things are working as expected in this current iteration of PR means that some of my previous assumptions were incorrect and that I now have a better understanding.

Also, I updated it so that set_full doesn't do a hardware, nor software, reset. Should the software reset ever be used? I'm not entirely sure, and if so, when?

I'm also puzzled by the partial=False constructor arg. The V1 driver initialises the display in full mode. In general applications clear a display on initialisation - which implies full mode. On startup micro-gui does this automatically. I don't object to it given the default - but can you foresee an application which would want old display content to be retained on power-up?

The only thing I could think of wanting to retain old content was if the thing controlling the epaper device had to reset for some reason unrelated to graphics, but didn't want user operation to be affected by it. That being said, I'm not sure it's worth the additional complexity, so I changed this.

Have you considered using my _linv method (or similar) for fast inversion? An aim is to minimise latency when a micro-gui user performs an operation like pressing a button.

I may need further assistance on this; I'm not sure what I would need to do.

surdouski commented 4 months ago

In regards to the most recent commit: the display was sitting on my desk and noticed that it wasn't updated after the first couple times. Updated some things and let it run for a few minutes on async and sync this time; seems to be working correctly now.

peterhinch commented 4 months ago

Should the software reset ever be used?

My display drivers only do this on initialisation. The Waveshare drivers I've ported are similar.

The _linv function is a fast way of doing a bit inversion and copy of a buffer. It takes a source bytearray and a destination (typically a memoryview slice of the display buffer) and copies the contents of the source to the destination, inverting the bits. This is done 32 bits at a time: the length arg is thus the length of the source buffer // 4. Paste the following to see how it works:

@micropython.viper
def _linv(dest: ptr32, source: ptr32, length: int):
    n: int = length - 1
    z: uint32 = int(0xFFFFFFFF)
    while n >= 0:
        dest[n] = source[n] ^ z
        n -= 1

a = bytearray(20)  # Source
b = bytearray(40)  # Destination
m = memoryview(b)  # Memoryview into destination
_linv(m[12:], a, len(a) >> 2)
print(b)

Viper code can be a little unfamiliar but it is astonishingly fast! It can be comparable to Assembler. [EDIT] Re latest change it sounds as if they have changed the sense of the busy pin. See my code comment - arguably the V1 display was wrong.

surdouski commented 4 months ago

In regards to the viper code, I think I'm understanding now, thanks. I read up on some documentation as well.

I added _linv and updated the reset to issue a sw reset command.

The only way I could think to re-use the code for the checking between ticks was to have it be a generator function. However, I'm not entirely happy with my approach and think it is probably more confusing than helpful.

peterhinch commented 4 months ago

I see what you're doing with the generator function: you want to avoid repeating code, yet the function needs to retain state.

I have a doubt over the use of generators in asynchronous code, because yield has a specific meaning to asyncio - namely an exit to the scheduler. This is something I've never investigated in detail, but it would need careful testing to be sure it does what you expect. There is an alternative approach which is to use a closure which avoids this hazard. Something like:

def _sendbytes(self):
    fbidx = 0  # Index into framebuf
    nbytes = len(self._ibuf)  # Bytes to send
    nleft = len(self._buf)  # Size of framebuf
    def inner():
        nonlocal fbidx
        nonlocal nbytes
        nonlocal nleft
        self._bsend(fbidx, nbytes)  # Invert, buffer and send nbytes
        fbidx += nbytes  # Adjust for bytes already sent
        nleft -= nbytes
        nbytes = min(nbytes, nleft)
        return nbytes > 0
    return inner

# calling:
self._command(b"\x24")
sb = self._sendbytes()  # Instantiate closure
while sb():
    pass

This could perhaps be improved by having the closure do the timing:

def _sendbytes(self):
    fbidx = 0  # Index into framebuf
    nbytes = len(self._ibuf)  # Bytes to send
    nleft = len(self._buf)  # Size of framebuf
    def inner():
        nonlocal fbidx
        nonlocal nbytes
        nonlocal nleft
        ts = time.ticks_ms()
        while nleft > 0:
            self._bsend(fbidx, nbytes)  # Invert, buffer and send nbytes
            fbidx += nbytes  # Adjust for bytes already sent
            nleft -= nbytes
            nbytes = min(nbytes, nleft)
            if time.ticks_diff(time.ticks_ms(), ts) > EPD.MAXBLOCK:
                return nbytes > 0  # Probably not all done: quit and call again
        return 0  # All done
    return inner

This would be called as above in synchronous code. In async code it would be

sb = self._sendbytes()  # Instantiate closure
while sb():
    await asyncio.sleep_ms(0)

Obviously all this is untested :)

There is an apparent muddle in the way the driver initialises self._partial. The constructor sets it dependent on the passed partial arg. It then calls .init which calls .set_full which unconditionally sets it False. Unless you can see a use case for the constructor arg, I suggest losing it and letting .init behave as at present.

surdouski commented 4 months ago

I hadn't considered using a closure, that's nifty. I like the clever return; I've implemented this with the small difference of having it return nbytes instead of nbytes > 0, as 0 is the only falsy value.

There is an apparent muddle in the way the driver initialises self._partial. The constructor sets it dependent on the passed partial arg. It then calls .init which calls .set_full which unconditionally sets it False. Unless you can see a use case for the constructor arg, I suggest losing it and letting .init behave as at present.

Agreed, must have missed that.

Additionally, I've added a commit which adds some print statements to the two demos I've been using to ensure the visual matches the expectation.

surdouski commented 4 months ago

Added the driver to the package.json as well.

Although... the way this is setup, wouldn't this be installing all of the drivers with the mip install? Maybe there is some caveat I'm missing.

peterhinch commented 4 months ago

Added the driver to the package.json as well.

Thanks. The install does pull in all the drivers is a directory; this is documented here. Laziness on my part, but drivers are fairly small and Flash space is not usually critical. If the number of ePaper drivers becomes large I could split them into more than one directory.

I'm happy with the code. I'll revisit it in a few days to see if there's anything I've missed and update the docs - then we can go ahead.

Have you been able to test with micro-gui? If not, I'm willing to release it with an "untested on micro-gui" warning until my display arrives.

surdouski commented 4 months ago

I have not yet tested with micro-gui. I just took a quick look, but I don't have enough time this morning to figure out how to set it up. I might have some time tonight, possibly.

peterhinch commented 4 months ago

I think testing can be minimal. You could define pins for the buttons without bothering to implement the hardware. If the epaper demo runs, with the screen updating, we can consider it passed. We're testing for the display driver API, response to controls can be taken as read.

However it's entirely up to you whether you want to do this. I have a test rig set up and once my display arrives I will be able to do this easily.

surdouski commented 4 months ago

The first time I tried to run it, it had a messed up screen. This is the first time I had tried to run it on this waveshare (I have 2 of them, as previously mentioned), and the last time I had used this one I had probably sent some commands that were persistent for whatever reason.

While this does worry me slightly, I ran it a second time without changing anything and it appeared to work fine. I'm unsure what the reason for this is and I have now re-ran it multiple times in an attempt to reproduce the behavior, to no avail. However, because I don't know what initial state the waveshare device was in beforehand, I suspect I won't get that behavior again.

image

peterhinch commented 4 months ago

Thanks for testing. I'll have a final look through the code over the weekend and update the drivers doc. We should be good to go.

peterhinch commented 4 months ago

My V2 display arrived today! With your driver the micro-gui demo worked perfectly. The display seems good wit fast updates and minimal ghosting. I have finished the project which has been occupying my time, so I can give this full attention. I'm keen to get this merged soon. I don't know if you want to submit a new PR with just the driver and the package.json file? I'm no expert on Git but this is my approach to achieving a clean git history, but if you know a better way then go ahead.

There are a few - hopefully final - code comments:

The documented .ready method is used by applications and needs to be implemented. It should take account of the ._busy bound variable because there are times when ._busy is True yet the pin will almost certainly be False. I suggest:

def wait_until_ready(self):
    while not self.ready():
        time.sleep_ms(100)
def ready(self):
    return not (self._busy or self._busy_pin())  # 1 == busy

The ._show_full method should end with

time.sleep_ms(2000)  # Demo mode: give time for user to see result

The reason for this is documented here.

Rather than add a new display_on_partial method, why not check the status when .display_on() is issued?

def display_on(self):
    if self._partial:
        self._command(b"\x22")
        self._data(b"\xFF")
        self._command(b"\x20")
    else:
        self._command(b"\x22")
        self._data(b"\xF7")
        self._command(b"\x20")

Lastly the .clear method is unused. I have no objections to it being retained if you wish, but the normal way of clearing a display in nano-gui is to issue an ssd.fill(0 followed by ssd.show().

surdouski commented 4 months ago

My V2 display arrived today! With your driver the micro-gui demo worked perfectly.

Wonderful.

The documented .ready method is used by applications and needs to be implemented. It should take account of the ._busy bound variable because there are times when ._busy is True yet the pin will almost certainly be False. I suggest:

def wait_until_ready(self):
    while not self.ready():
        time.sleep_ms(100)
def ready(self):
    return not (self._busy or self._busy_pin())  # 1 == busy

:+1: Updated.

The ._show_full method should end with

time.sleep_ms(2000)  # Demo mode: give time for user to see result

Updated in both _show_full as well as _show_partial, as I believe it was needed there as well.

Rather than add a new display_on_partial method, why not check the status when .display_on() is issued?

def display_on(self):
    if self._partial:
        self._command(b"\x22")
        self._data(b"\xFF")
        self._command(b"\x20")
    else:
        self._command(b"\x22")
        self._data(b"\xF7")
        self._command(b"\x20")

Agreed, that's much more clean.

Lastly the .clear method is unused. I have no objections to it being retained if you wish, but the normal way of clearing a display in nano-gui is to issue an ssd.fill(0 followed by ssd.show().

I have no qualms with removing it; it has been done.

I don't know if you want to submit a new PR with just the driver and the package.json file?

I force pushed the branch so that the only commit is the most recent.

peterhinch commented 4 months ago

Excellent! Now merged. I'll push a revised DRIVERS.md later.

What's your view on a V2 greyscale driver? Do you want to do this?

peterhinch commented 4 months ago

I have pushed some changes to the V2 driver. Some are just general code tidying. The last are in response to a bug I found in micro-gui.

Code changes

  1. display_on renamed _display_on (it's not part of the API).
  2. Modify .send_bytes and calls to it (see below).
  3. Move the demo_mode code to .show (instead of copies in ._show_full and ._show_partial). Remove duplication.
  4. Code comment improvements.

send_bytes
I have changed the code so that the following is no longer needed.

while sb():
    pass

A simple sb() call now suffices in synchronous code because the timeout only occurs in asynchronous code. (An idea which only occurred to me later). Please see code comments in _send_bytes.

API changes

  1. Class variable MAXBLOCK is now instance variable maxblock.
  2. New instance variable blank_on_exit.
  3. New method .shutdown (see below for reasons for this and (2)).

.shutdown and blank_on_exit
I discovered a bug in micro-gui whereby, with the EPD, the shutdown procedure did not work. The GUI now calls .shutdown which optionally clears the display. This behaviour is determined by the blank_on_exit bound variable. Micro-gui now calls .sleep() on application exit.

peterhinch commented 4 months ago

One (hopefully final) change. The default _DC_PIN has now reverted to 8.

While I understand your reservation about this pin, the purpose of the defaults is to match the physical wiring of the Pico socket. Thus, in the case where the Pico is fitted to the socket, the setup can simply be

# Using the onboard socket connection default args apply
ssd = SSD()

Where a host is connected via a cable these defaults may be overridden.

surdouski commented 4 months ago

One (hopefully final) change. The default _DC_PIN has now reverted to 8.

Fair enough. I suppose I should update my other PR as well to align with that.