If57 update #7997

bablokb · 2023-05-18T18:35:30Z

Update port for Pimoroni InkyFrame 5.7".

Some notes:

fix frozen modules (remove unused module, add support for builtin hardware)
use standard pico-w board-LED as board.LED: having an additional LED is always useful (I use it for technical information)
configure enable-pin as DigitalInOut. This is to keep compatibility with the Badger2040W port and to how Pimoroni uses this pin in their MicroPython version
optimize parameters: refresh_time adds a useless wait after _clean_area, and seconds_per_frame of 30 is not enough
support the buttons of the InkyFrame by providing board.KEYCODES.SW_A etc. for keycodes returned by keypad.ShiftRegisterKeys

tannewt

Thank you for the improvements! I have a couple concerns though.

tannewt · 2023-05-19T16:08:53Z

ports/raspberrypi/boards/pimoroni_inky_frame_5_7/board.c

@@ -100,10 +111,10 @@ void board_init(void) {
        false,  // color_bits_inverted
        0x000000,  // highlight_color
        refresh_sequence, sizeof(refresh_sequence),
-        28.0,  // refresh_time
+        2.0,  // refresh_time


I don't think this is correct. It needs to be long enough for the sequence to complete before we send it more commands. Usually the busy pin would minimize this but it isn't used here.

I timed it at just under 28 seconds.

Currently, I need to do this:

call display.refresh()

_clean_area()

wait until refresh_time expires

update areas

return immediatly

call time.sleep(until refresh is really finished)

set board.ENABLE_DIO = 0 (shutdown and cut power)

During the wait-time after _clean_area (3) the display does nothing, it just wastes battery with a tight loop. (6) is necessary regardless of the value of refresh_time, since there is no wait after update areas.

So although the refresh time is really about 30 seconds, in the current implementation it makes more sense to set it to something short and wait after display.refresh() returns (I currently wait seconds_per_frame).

The better solution would be that display.refresh() returns after refresh is finished and power can be cut without problems. But until then, a long refresh time has negative impact.

We could make 3 use sleep and that'd save power. I don't think we want to update the displays RAM while it is refreshing though.

I don't think anything happens during (3) even when the refresh_time is 28 secs. The display flashes two times after sending the clean-area bytes (dimming all colors), but that is it. The real refresh flashes much more often and the display is still busy way after the refresh() method returns.

And I don't get the point of waiting half a minute after _clean_area, but not waiting that time after the real screen update. This does not make sense. And in all my testing I haven't seen any problems with the two seconds refresh_time.

To repeat the point: I do believe the necessary refresh time is about 30 seconds. But this is the time after the refresh() method returns and the display is still busy refreshing. But the refresh_time parameter has nothing to do with the refresh behavior.

tannewt · 2023-05-19T16:09:31Z

ports/raspberrypi/boards/pimoroni_inky_frame_5_7/mpconfigboard.h

@@ -4,7 +4,7 @@
 #define CIRCUITPY_DIGITALIO_HAVE_INVALID_PULL (1)
 #define CIRCUITPY_DIGITALIO_HAVE_INVALID_DRIVE_MODE (1)

-#define MICROPY_HW_LED_STATUS   (&pin_GPIO6)
+#define MICROPY_HW_LED_STATUS   (&pin_CYW0)


I'd rather not change this because the built in LED isn't visible from the front.

ports/raspberrypi/boards/pimoroni_inky_frame_5_7/pins.c

bablokb · 2023-05-20T11:30:13Z

@tannewt Can you post some test-code that demonstrates the need for a refresh_time value of 28? All my tests show that 2 is enough, but I might not test every possible situation. And this really makes a difference from the user's side. A complete cycle (including connecting to an AP, fetching data from the net and updating the display) takes about 60s with my implementation and 90s with yours.

Another question: will you consider the changes from #7996 to EPaperDisplay.c? I.E. supporting a shiftregister pin as a busy pin? This would help since with a busy pin the value of refresh_time is ignored anyhow.

tannewt · 2023-05-22T20:36:16Z

Here is my display if I let it have the 28 seconds for the clear step.

trim.E4B3CCA4-6C93-4154-83BA-D617286AAF9B.MOV

tannewt · 2023-05-22T20:39:43Z

Another question: will you consider the changes from #7996 to EPaperDisplay.c? I.E. supporting a shiftregister pin as a busy pin? This would help since with a busy pin the value of refresh_time is ignored anyhow.

Supporting shift register would be ok but I doubt it buys you much time. My understanding is that the refresh sequence is fixed by the LUTs and will take the same amount of time each time. So, the busy pin will just get you a slightly more precise delay time.

bablokb · 2023-05-23T12:41:59Z

Here is my display if I let it have the 28 seconds for the clear step.

This behavior is definitely different to what I see. I'm currently not at home but when I am back end of the week, I try to post a video of what I experience. BTW: my test-code is here: https://github.com/bablokb/circuitpython-examples/blob/master/inkyframe57/main.py

tannewt

One more comment. The clean refresh time is still incorrect.

ports/raspberrypi/boards/pimoroni_inky_frame_5_7/pins.c

bablokb · 2023-06-27T17:21:20Z

One more comment. The clean refresh time is still incorrect.

I still disagree on that. I have two of these Inkys running here with my patch without any problems. It saves me 26s of update time without any problems.

Can you please post some test-code that demonstrates the need for a refresh_time value of 28? If there is no code that fails with 2s then there is no reason to use 28s.

bablokb · 2023-06-27T18:23:23Z

Maybe your PSR-setting is the reason why you need such a long refresh_time? You tell the display in the init-sequence to rotate the screen by 180, and then you rotate the content yourself within CircuitPython before sending it to the display.

tannewt · 2023-06-28T16:33:50Z

One more comment. The clean refresh time is still incorrect.

I still disagree on that. I have two of these Inkys running here with my patch without any problems. It saves me 26s of update time without any problems.

I also have one running here with the 28s clean. Just like I posted a video of.

Can you please post some test-code that demonstrates the need for a refresh_time value of 28? If there is no code that fails with 2s then there is no reason to use 28s.

It's the currently checked in code. Your 2s setting is interrupting the process. I checked the state of the busy pin and it takes 27.67 seconds each time. So, the 28 setting is best.

The 2 seconds may work fine for you, but I don't want to check it in.

bablokb · 2023-06-29T05:22:01Z

It's the currently checked in code.

I would like a code.py example that demonstrates failure.

Your 2s setting is interrupting the process.

Have you tested the busy-pin with the updated PSR setting? And what consequences does "interrupting the process" have? Visually, I see no difference between 2s and 28s: the display is doing nothing until the real update starts. After the clean, during the real update, this is different: the display is indeed flashing for about that time-period (i.e. it is busy).

tannewt · 2023-07-05T21:22:51Z

It's the currently checked in code.

I would like a code.py example that demonstrates failure.

As far as I know, they all should. The clean code is part of the C module and every refresh.

Your 2s setting is interrupting the process.

Have you tested the busy-pin with the updated PSR setting?

No. I have no idea why rotation would effect the refresh time.

And what consequences does "interrupting the process" have?

I don't know what the consequences are but there is a reason the example code for the display does it. I'd rather be safe by default and do it.

Visually, I see no difference between 2s and 28s: the display is doing nothing until the real update starts.

Please post a video. I posted one already showing you that mine does flash for the full 28 seconds of the clean. (Since my CP build doesn't interrupt it.)

bablokb · 2023-10-10T17:48:31Z

After additional study of the codebase I must admit that changing the refresh_time is not the right thing to do. So I will update my pull-request to get the other stuff in.

But I would still like to get rid of the useless time spent in _clean_area. Can we add an option to make this optional? The only reference I found regarding this cleaning is in a document from Waveshare and they state it is necessary because of ghosting. But they give no reference for this claim.

Pimoroni, which uses the same displays, does no cleaning in there code either. I am a regular on their forums and I haven't seen any complains about ghosting there. So there is a good chance that this is not a normal issue.

Another option is to remove cleaning all together, since it can be done from application code if necessary.

tannewt · 2023-10-11T17:00:11Z

After additional study of the codebase I must admit that changing the refresh_time is not the right thing to do. So I will update my pull-request to get the other stuff in.

Thank you!

But I would still like to get rid of the useless time spent in _clean_area. Can we add an option to make this optional? The only reference I found regarding this cleaning is in a document from Waveshare and they state it is necessary because of ghosting. But they give no reference for this claim.

Yup, feel free to add a kwarg clean_acep to the EPaperDisplay constructor that defaults to True. You can then set it to False for your use.

tannewt

Great! Thank you!

bablokb added 6 commits May 16, 2023 18:54

fixed frozen modules

ed1b9a4

use board-LED as status-LED (sic!)

bfa536f

use board-LED as status-LED (sic!)

957d011

support enable-pin as DigitalInOut (Badger2040W compatibility)

3c10409

optimize parameters

123aad7

add support for keycodes to make inky-buttons usable

2d30d6d

tannewt requested changes May 19, 2023

View reviewed changes

use GPIO6 as status led and and add PICO_LED for onboard-led

767be33

bablokb added 2 commits June 23, 2023 15:05

Merge branch 'main' into if57_update

9f6a518

added defines for VOLTAGE_MONITOR/A3

0af689d

dhalbert requested a review from tannewt June 27, 2023 13:21

tannewt requested changes Jun 27, 2023

View reviewed changes

ports/raspberrypi/boards/pimoroni_inky_frame_5_7/pins.c Outdated Show resolved Hide resolved

add board.LED

d905ac0

jepler added the board New board or update to a single board label Aug 22, 2023

bablokb added 3 commits October 20, 2023 18:28

Merge branch 'main' into if57_update

74ec5c1

merged main

acfb18d

changed refresh_time as requested

9f91eb4

dhalbert requested a review from tannewt April 3, 2024 13:33

tannewt approved these changes Apr 3, 2024

View reviewed changes

tannewt merged commit ab7f506 into adafruit:main Apr 3, 2024

bablokb deleted the if57_update branch April 4, 2024 08:37

If57 update #7997

If57 update #7997

Uh oh!

Conversation

bablokb commented May 18, 2023

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

tannewt May 19, 2023

Choose a reason for hiding this comment

Uh oh!

bablokb May 19, 2023

Choose a reason for hiding this comment

Uh oh!

tannewt May 19, 2023

Choose a reason for hiding this comment

Uh oh!

bablokb May 19, 2023

Choose a reason for hiding this comment

Uh oh!

tannewt May 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bablokb commented May 20, 2023

Uh oh!

tannewt commented May 22, 2023

Uh oh!

tannewt commented May 22, 2023

Uh oh!

bablokb commented May 23, 2023

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bablokb commented Jun 27, 2023

Uh oh!

bablokb commented Jun 27, 2023

Uh oh!

tannewt commented Jun 28, 2023

Uh oh!

bablokb commented Jun 29, 2023

Uh oh!

tannewt commented Jul 5, 2023

Uh oh!

bablokb commented Oct 10, 2023

Uh oh!

tannewt commented Oct 11, 2023

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!