Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to Interstate / HUB 75 for panels with different color orders #660

Merged
merged 3 commits into from
Feb 10, 2023

Conversation

ZodiusInfuser
Copy link
Member

This follows the same approach as used for Plasma. However, plasma is a lower number of pixels, so this may make rendering slower. I am unsure how to properly test this at the moment though.

@ZodiusInfuser ZodiusInfuser added enhancement New feature or request [- interstate75 -] https://shop.pimoroni.com/products/interstate-75 micropython This issue or request relates to micropython (either code or bindings) c++ This issue or request relates to C++ code labels Jan 31, 2023
@helgibbons
Copy link
Contributor

I can't tell a visual speed difference running the rainbow.py or balls_demo.py examples, but rainbow.py claims it's 776ms between updates on v1.19.12 MicroPython and 832ms between updates with this branch.

@Gadgetoid
Copy link
Member

Which resolution display @helgibbons? At 32x32 pixels that would be around 54μs per pixel extra.

This could potentially be optimised by doing the switch in Hub75::update() rather than per pixel. But the resulting code would be rather horrifying and would get worse if we supported additional pen types.

I can't see any obvious (at least non-horrible) ways to optimise the colour order otherwise- you can't beat a good ol' compiler-optimised switch.

I have noticed that Hub75::clear() is complete nonsense, though, and that mashing all this bit-shifting, swapping and lookup stuff through probably-not-inline function calls is likely not doing us any favours.

I think I could probably see ways to optimise this -

  • set_colour does bounds checking we don't need if we're calling from update
  • set_colour does some y coordinate translations and checks we could probably do more efficiently in update
  • Pixel hides a per-channel conversion from 8-bit to 10-bit colour via a gamma lookup table
  • update calls set_pixel which calls set_colour aaaaaaa

It might be worth exploring those. Making clear not total absolute horrifying nonsense would be a good and low-hanging start that could earn us the cost of colour order conversion back if it's use in our examples.

@helgibbons
Copy link
Contributor

64 x 64!

@Gadgetoid
Copy link
Member

Okay rainbow.py doesn't call clear but we should probably still something something memset(back_buffer, 0, sizeof(Pixel) * width * height)

Oh at 64*64 that would be more like 14μs per pixel.

Should probably just delete the bounds checking from Hub75::set_color and see if that gets us it back.

AFAIK it's impossible to call set_pixel or set_color from MicroPython, since it uses the PicoGraphics versions of those functions.

We could almost certainly handle the interleaving better than a per pixel:

    if(y >= height / 2) {
        y -= height / 2;
        offset = (y * width + x) * 2;
        offset += 1;
    } else {
        offset = (y * width + x) * 2;
    }

By stepping through the source frame buffer in two separate fields and incrementing an offset rather than including multiplication.

That code above basically ensures that the bottom and top half of a given display are interleaved so that row:

1, 2, 3, 4

Becomes:

1, 3, 2, 4

And they can be blasted out more efficiently.

Basically this change as-is is fine, there are bigger performance issues - I think - than what's introduced here. This code could really use the eye someone with a head for optimisation and some spare time! 😆

@ZodiusInfuser
Copy link
Member Author

Thanks for looking this over @Gadgetoid . I had hoped to be smart be changing the pin order in the Constructor, but sadly the bin defines don't seem to be used beyond the initial init. The PIO assumes a fixed pin order regardless.

Basically this change as-is is fine, there are bigger performance issues - I think - than what's introduced here. This code could really use the eye someone with a head for optimisation and some spare time! 😆

That first part can certainly be me. If only the second part were true 😭

@Gadgetoid Gadgetoid merged commit de8cb95 into main Feb 10, 2023
@Gadgetoid
Copy link
Member

The code here is very similar to Cosmic/Galactic which I have miserably failed to make any faster. Since I don't want to have to set up a counter for "times the compiler has totally humbled my attempts to optimise something" I'm just going to merge this one and worry about performance later.

The fact of the matter is- very little we can do behind the scenes will have as big an impact as the user's drawing code.

I should totally port my fast numpy-powered FX demos to Interstate75 though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[- interstate75 -] https://shop.pimoroni.com/products/interstate-75 c++ This issue or request relates to C++ code enhancement New feature or request micropython This issue or request relates to micropython (either code or bindings)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants