Expose `Content` so we can manipulate it directly

[riskable]

I want to be able to iterate over all the Pixels in my matrix, modifying them slightly as I go but I can't! Instead I have to keep a separate copy of the exact same data that will exist inside the SmartLedMatrix and manipulate that first then re-write it to the matrix. This seems inefficient but... Maybe this is intentional?

If it is intentional then carry on but it sure would be nice if I could do something like for pixel in matrix.iter_mut() { /* Manipulate the pixel */ } 👍

[libesz]

Good question. I cannot remember anymore if it was intentional. For sure there is a (potential) mapping between the input data order and the internal storage to align to the display PCB layout. So if the internal storage would be exposed as-is, it would be shuffled as you get it with the current implementation.
Now the mapping is done during the drawing, so flushing gives constant execution time. We might be able to defer the mapping to flush time by loosing the constant execution time. Not sure is that would harm in certain cases.

@rfuest Would you have any thought on this?

[rfuest]

I think that most users want to access the matrix using coordinates and not indices. I would add pixel(&self, p: Point) -> Option<C> and set_pixel(&mut self, p: Point, c: C) functions to SmartLedMatrix, like we did in the new embedded-graphics Framebuffer.

But I also don't see any harm in exposing the content array to the user in case they want to apply a mutation that doesn't rely on the pixel position, like a global fading effect and want maximum performance. Adding two simple content(&self) -> &[RGB8; N] and content_mut(&mut self) -> &mut [RGB8; N] would work IMO.

[riskable]

@rfuest Points out my exact use case: I want to implement a global fading effect that just reduces the current r,g,b values by a small amount with each call of that function.

[libesz]

I am fine with either approaches. But if we expose the content as-is, we should make it clear somehow to the caller that the data order may be shuffled in any ways.

Just a question: for a global fading effect, couldn't the smart-leds built-in brightness limiter work? It is already exposed via this driver and can be changed anytime between two flushes.

[riskable]

Just a question: for a global fading effect, couldn't the smart-leds built-in brightness limiter work? It is already exposed via this driver and can be changed anytime between two flushes.

No that wouldn't work because I'm fading individual LEDs based on their current state. Trivial example: The end user presses Y O... By the time they press the O key the Y key could have faded a little bit (how much, depending on the config). Using global brightness they'd both fade at the same rate.

[rfuest]

I am fine with either approaches. But if we expose the content as-is, we should make it clear somehow to the caller that the data order may be shuffled in any ways.

IMO we should just add both approaches. They are both useful for different use cases. And I agree that we need to document that content will use the physical order of the LED string and not the logical one that is implemented by Layout.

[riskable]

I completely agree with @rfuest because having the ability to access the LEDs in their data order (as opposed to just relying on embedded-graphics primitives) would make certain operations so much easier for me. For example, to fade all lit LEDs by one step (say, reduce brightness by 10) I can just iterate .content and adjust the values accordingly. I could even apply cool filter effects very efficiently by looking for LEDs matching a specific color and modifying it in place.

So to summarize I think we need:

• Change Content to pub inside of SmartLedMatrix (and also add pub: pub struct Content).
• Add pub fn iter() and pub fn iter_mut() methods to Content (so we can matrix.content.iter() instead of matrix.content.0.iter().

[rfuest]

So to summarize I think we need: ...

I think you should take a look at the latest version which doesn't use the Content type anymore.

[riskable]

Oops! I see you've changed quite a lot... I'm converting my code now to use the latest & greatest 👍