Protomatter: Integrate with CircuitPython

[jepler]

This pull request:

• Adds displayio.FramebufferDisplay
• Adds _protomatter.Protomatter
  It anticipates that a new module adafruit_protomatter will be used to combine the two into a displayio-using led matrix driver.

Current known problems:

• Does not survive soft reset
• Does not auto-refresh (these first two are related, enabling auto-refresh makes it crash at soft reset)
• Rotation did not work
• The console only gives 1 full row of text on the 64x32 panel, seems like it should be more (it's not really usable anyway)

The code I've been using to drive it is an adaptation of the "1D Wave Simulation" ulab demo: https://gist.github.com/jepler/5b11e292193f1fcb602900ff50e8944d

[jepler]

It's still not ready for inclusion, but I've heavily reworked this PR. See the initial comment for much more information, including a checklist of known problems. The one I'm worried about is the problems with auto_refresh / surviving soft reset, because the design requires this Python Protomatter class to work. This design may simply not be workable, so hopefully @tannewt has some ideas about how to restructure it so it can work. Rotation is "probably a shallow bug" but I haven't looked at it yet.

[jepler]

The 64x32 display isn't very useful for showing the console -- you get only 2 rows by 8 chars.
Making the display persist over soft resets is proving difficult. Instead, it resets itself at soft reset.

Because of the less than useful nature of the console, I wonder if the work could be accepted in this state; and later on we can come back and work on it when we have framebuffer lcds large enough for it to matter. @tannewt ?

[tannewt]

@jepler What did you try when getting it to work across soft resets? Seems like you are close and having it will be useful when we support multiple panels in the future.

[jepler]

I'm aware that I need to preserve

• all the involved pins (existing never-reset code should cover this, though)
• the _protomatter.Protomatter object with its own internal allocations (these can't be moved without additions to protomatter's C API)
• the 565-format bytearray that is used as the target for the FramebufferDisplay and converted to protomatter's internal format
• the callback object which lets FramebufferDisplay notify the underlying framebuffer that it's been changed
  Because I identified the limitation in the protomatter C API, I wanted to not go too far down a road we may or may not want to travel.

The hardest parts of preserving across a reset are specific to Protomatter and don't seem to be things that apply for true hardware framebuffers.

[jepler]

I presently define _PM_allocator to call m_malloc. Here are its uses within protomatter:

core.c:    if((core->rgbPins = (uint8_t *)_PM_ALLOCATOR(rgbCount * sizeof(uint8_t)))) {
core.c:        if((core->addr = (_PM_pin *)_PM_ALLOCATOR(addrCount * sizeof(_PM_pin)))) {
core.c:    // _PM_ALLOCATOR() by definition always aligns to the longest type.
core.c:    if(!(core->screenData = (uint8_t *)_PM_ALLOCATOR(screenBytes + rgbMaskBytes))) {

[tannewt]

Did you try moving the three protomatter allocations? If that works, we could add an api or simply go with hacking it.

[jepler]

This branch is going back into a heavily unstable state, I'm pushing some clearly not working code as a shortcut to transfer it between a couple of my computers.

[jepler]

actually the unstable version went in a branch that is not pull-requested yet. it's https://github.com/adafruit/circuitpython/compare/master..jepler:protomatter-displayio-v2

What's changed so far is:

• I created an allocation/deallocation function which can use heap if available and supervisor allocation if needed
  • this involved exposing a new function in the supervisor allocator to check if a block was allocated by it, not by the heap
• I placed a protomatter_protomatter_obj_t in primary_display_t, but do not use it yet
• I am in the process of eliminating the need for the "userspace" class between FramebufferDisplay and Protomatter. This involves starting a framebuffer protocol in C.

Next steps:

• Move the record of the used pins into protomatter_protomatter_obj_t, rather than into subordinate allocations. This means fixing the max number of RGB and address pins.
• At reset, move the protomatter_obj into the fixed storage (if not there already), and then initialize a fresh Protomatter object using the supervisor allocator if necessary
  • this will cause a short period where the display is blanked
• Similar to moving the pin info directly into the protomatter_obj, I may do the same thing in the Protomatter object to reduce allocations

I am going to choose a fixed limit of 30 RGB pins + 6 address pins, which fits in a multiple of 4 bytes. For the breakouts that adafruit sells, 6+4 is the actual number used.

This is more work than I anticipated initially, but I feel like the pieces are starting to come together and my understanding of displayio and the allocators is improving as an additional benefit.

[jepler]

I believe that generally speaking the code works at this point and it is ready for style review, though before it's done-done we need to get the protomatter changes merged upstream and add support for nRF on our end.

[jepler]

I should explain that the test changes are not really related, see micropython#4000 for background.

[tannewt]

We should have those test changes integrated already. Do you think we just messed up the merge?

[tannewt]

(I still have a bunch of review to do on this.)

[tannewt]

Thanks for taking this on! I've add a number of comments. I like the directions you are going by using a "framebuf as a bus". I think there is some work to do on the Framebuf stuff to slim it down and simplify it. I think having brightness on it makes sense but it should then delegate to the framebuffer provider to implement it. A 24bit TTL display will have a backlight pin but a protomatter display will need to adjust what it outputs.

[jepler]

As far as the backlight, it can delegate to the underlying framebuffer (a protocol function that is permitted to be NULL), or it can use a pin as now as a fallback. protomatter uses 0 to mean pause, nonzero to mean not paused; it doesn't scale RGB values to emulate intermediate brightness levels. I have a feeling that adding a fixed off time might give better results, so if that's important we should talk to @PaintYourDragon about it.

If we thought at least 2 framebuffers would do backlight via RGB pin as regular Displays do, then having the code in FramebufferDisplay will be a net reduction of copypaste backlight code.

[jepler]

@tannewt I've made changes for many of your review comments, and addressed some of the others inline. Please take another look when you have a chance.

[tannewt]

As far as the backlight, it can delegate to the underlying framebuffer (a protocol function that is permitted to be NULL), or it can use a pin as now as a fallback. protomatter uses 0 to mean pause, nonzero to mean not paused; it doesn't scale RGB values to emulate intermediate brightness levels. I have a feeling that adding a fixed off time might give better results, so if that's important we should talk to @PaintYourDragon about it.

If we thought at least 2 framebuffers would do backlight via RGB pin as regular Displays do, then having the code in FramebufferDisplay will be a net reduction of copypaste backlight code.

I like the idea of FramebufDisplay having the brightness knobs (brightness and auto_brightness) but think the protocol should implement it and take the args on construction for the pin. We could have FramebufDisplay scale the RGB values like PixelBuf does but agree adding a fixed off time would give a better result. So, I'd removed backlight_pin for now.

[tannewt]

How about calling this refresh() to match the other display types? I get that this would be needed if using protomatter without displayio.

[tannewt]

This sounds similar to what common_hal is used for, C data structures rather than Python ones. Could you use the common_hal variants in the protocol?

These wrapper functions don't make much sense to me because the user of the protocol could just throw away the return too.

[jepler]

@tannewt I think I fixed all the issues you raised, except for this one. For some reason there's not a spot to reply inline, so ...

Protocols vs common_hal: Protocols take mp_obj_t as the first argument, while common_hal takes a type-specific object pointer. On reflection, I prefer getting the compile-time type checking that the protocol function pointer is of the right type, even if it means introducing an intermediate function to do the casting of the argument. (I think I did it with casts when I did audio sample protocols)

As always, I can change it if you feel strongly.

[tannewt]

Ok, I'm fine with the wrapper functions.

One comment on the API. I glanced at your Learn guide draft and had thoughts. Is there a library I should look at too?

Thanks!

[tannewt]

I was just peeking at your guide for how I can test this. I think we want these all to be kwarg-only args because they are either numbers, pins or lists of pins. Reading the code doesn't help with what is what. The current positional arguments can be required and have no default.

I'd also take in the height even though it's implicit in the address pins. It can make the framebuffer smaller than the available address pins and can also validate enough address pins are given.

[jepler]

Yeah, I can make them kw-only.

I can make it so that height, if specified, is checked against the expected number. I'm not sure of the use case of creating a 16 line display on a 32 line panel, though.

however, the raspberry pi lib for driving these displays (I think that's the one) can drive multiple displays wired in "zigzag" fashion. In that case, the relation between the WxH of the screen is decoupled from the true width of the display. Protomatter C library doesn't support this (yet, not sure if planned).

[jepler]

There is a wrapper module, but it's not uploaded anywhere yet. It may just end up committed in the guide repo at this time, because I'm not sure there's time to get it into the library package on the timeline in which we want the guide to appear.

Looks like I can't get the current content at this computer I'm at, but it's close to this:

import displayio
import _protomatter

def protomatter_display(
    width,
    depth,
    rgb_pins,
    addr_pins,
    clock_pin,
    latch_pin,
    oe_pin,
    *,
    doublebuffer=True,
    framebuffer=None,
    rotation=0,
    backlight_pin=None,
    brightness=1.0,
    auto_brightness=False,
    auto_refresh=True,
    native_frames_per_second=60
):
    height = len(rgb_pins) // 3 * 2 ** len(addr_pins)
    pm = _protomatter.Protomatter(
        width,
        depth,
        rgb_pins,
        addr_pins,
        clock_pin,
        latch_pin,
        oe_pin,
        doublebuffer,
        framebuffer=framebuffer,
    )
    return displayio.FramebufferDisplay(
        pm,
        width=width,
        height=height,
        rotation=rotation,
        backlight_pin=backlight_pin,
        brightness=brightness,
        auto_brightness=auto_brightness,
        auto_refresh=auto_refresh,
        native_frames_per_second=native_frames_per_second,
    )

There's also a def default_display() but I am not sure that makes sense.

[jepler]

should rename "depth" to "bit_depth". The 4 value was just in the Arduino example, gives 4096 colors (16 effective levels per primary).

[jepler]

@tannewt @ladyada Checks are green now, but since I had to "make translate" I need a fresh review before it can be merged.

[ladyada]

:)