SenseHat LED matrix text rendering model

[logicaloud]

• Support text rendering into the LED matrix
• Apply device model to commands and properties

A critical eye would be good to judge whether the applied device interface, command and property attributes are in keeping with the intent of the device model.

Microsoft Reviewers: Open in CodeFlow

[Ellerbach]

Nice work, couple of quick comments

[Ellerbach]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[krwq]

I do like the fact that we're extending this to support text scrolling etc but I'd prefer this logic was reusable to any LED matrix. Here is how I'd imagine this would work:

• this class remains mostly untouched (unless you need to add extra API or whatever to make text work then those changes are ok but nothing directly text related)
• text scrolling / setting is an abstraction which takes abstract display (or couple of Action/Func for each needed operation)

the usage would be more or less like this (you might have better ideas but this is bare minimum to isolate this utility):

SenseHatLedMatrix matrix = ...;
LedMatrixTextScroller textScroller = new(width, height, (x, y, color) => matrix.DrawPixel(x, y, color));
textScroller.Start();
textScroller.SetText("hello");
textScroller.Stop();
// Start could also happen automatically and Dispose could make it stop - don't care much about specific design choice

Note: I don't expect you to integrate this utility anywhere else (so there is no need to move it outside of SenseHat project but it might be a good idea to do so if you feel like it) but if anyone else asks or tries to add something similar I'll point them in this direction so they can re-use this code. Ideally I'd imagine this utility somewhere next to BdfFont

[krwq]

The callback could also draw entire buffer or whatever, pick a design which feels easiest to use from user perspective but is not unreasonably slow - I think for small matrix like this it doesn't matter if you draw by pixel or screen but for larger one by pixel might be a bit too slow but if library is not meant for large screens then that argument goes away - I think reasonably largest screen would be 64x64 so that gives you about 4k calls per 20ms which is around 200k operations per second - that might be good enough for Raspberry Pi 3. If we pick DrawPixel now worst case scenario we can always add extra ctor overload to the scroller in the future with entire screen callback and for existing ctor just translate DrawPixel into DrawBuffer callback (basically you'd wrap user passed callback with your own which utilizes that to draw a screen). If we do reverse it will be much harder to simplify this later and DrawPixel is so much easier to use than DrawBuffer

[logicaloud]

The "text scroller" in this instance is very simple: It increments a value (in the timer events) that is the horizontal offset position of the pre-rendered text (in SenseHatTextRenderMatrix) from where bits should be copied to the LED matrix display. It may be possible to generalize the use for small, single-line LED displays. Larger displays will probably lead to different design choices. My suggestion would be to possibly add an issue "generalize text rendering and scrolling for single line LED displays" or similar, that could be picked up by someone who has such a device, otherwise there may be a bit of guessing of what is feasible. This pull request could go into a different branch in the mean time, but I think it would not be too much work to refactor for generalization later. I'd be happy to test the generalized solution on SenseHat if/when available.

[logicaloud]

There was a noticeable difference (smoother scrolling) when setting the pixels with a single Write operation vs multiple SetPixel (Pi 4, 8GB), although I can't recall whether I have visually compared in Debug mode only. Maybe optimization here could also be left until implementation for another LED display is attempted?

[krwq]

note: if you're isolating this class but would like to utilize PropertyAttribute (although I don't ask you to do it in this PR) with the class you'd need to create a wrapper for the TextScroller, i.e. SenseHatLedMatrixTextScroller which would be just a tiny class which allow you to only do text writing (drawing and text scroller shouldn't be used together anyway because it will just look like bogus pixels showing up). For now I'd prefer to not add that though.

[logicaloud]

The design objective was to keep things simple for the user, and text simply starts scrolling as soon as the there is more than one character to display, using the default TextScrollPixelsPerSecond (1 pixel per second). Separating the scrolling out is a possibility of course. Something to consider for a possible generalization maybe, that it should be possible to retain "automatically scroll long text" for tiny LED displays, which could be a property set in a base class with whatever default makes sense for the LED display at hand.

[krwq]

it's ok if someone disposes matrix prematurely as long as you get reasonable exception - we do not want to pollute the code just because someone might do weird things with the hardware. If possible do avoid locks (see my comment about alternative to timer design)

[logicaloud]

It's the disposal of the I2C bus that interferes in this case. Currently this works without exception:

using (var sh = new SenseHat()) 
{
    sh.LedMatrix.SetText("abc");
    // ... do something
}

In that case the text scrolling automatically starts because the text is long. At the end of the using, the SenseHat disposes the I2C bus. Without precautions, the text rendering could attempt to access the disposed I2C bus because it is in the middle of rendering a new frame. The Lock around Write in SenseHatLedMatrix prevents access to a disposed I2C device.

With the current API design (text automatically scrolls) it would not be obvious to the user that something needs to be reset before disposing the SenseHat. One could give up simplicity and ask the user to explicitly enable and disable text scrolling. Or demand that text is set to 'empty' before disposing. Or any exception in the text rendering method could be ignored. My preference would be to retain the lock for the simplest use without surprises; this would be for SenseHat on Pi with its small LED matrix. It is possible that generalizations for other LED displays require a different rendering method or make other design choices, eliminating the lock in their approach. - What do you think?

[krwq]

Rather than timer consider allowing user pick when drawing happens. I.e.:

TextScroller scroller = ....;

while (true)
{
  scroller.DrawFrameIfNecessary(); // note that you should keep track of time separately inside of this method so it doesn't move inconsistently when user calls this at not equal intervals
  // also this could return true/false depending if it actually draw or skipped this frame (because i.e. update wasn't necessary for whatever reason)
  Thread.Sleep(20);
}

this kind of design is usually more flexible and will also help us port this to nano framework which might not have timer implementation.

This design also allows you to easily pause text independently of drawing (you can still draw frame it will just no-op)

[krwq]

And note - if you make this design the way I described above you can still use Timer in your app but it won't be necessary for scroller to have dependency on it - you'd just call DrawFrameIfNecessary inside of the timer callback

[krwq]

(of course pick better names if you have any idea but ideally it should be straight forward to understand what the method is doing by just looking at the name)

[logicaloud]

Maybe this could be an extended feature in the future, where the user can toggle between automatic scrolling and manual scrolling? For SenseHat, there is probably not much gain in pausing text, because only one character (or parts of two) are visible at a time, but it would be a good thing to have for longer displays.

[krwq]

@logicaloud thanks for this work, it's really awesome! I added comments how we can leverage this outside of SenseHat as well and sorry for a bit late review

[logicaloud]

@krwq Thank you for your review. I'll go through each point and comment separately.

[joperezr]

@krwq can you take another look here? Looks like @logicaloud has replied to your comments.