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.

Sign up for GitHub

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

SenseHat LED matrix text rendering model #2324

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

SenseHat LED matrix text rendering model #2324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fca48e2
SenseHat LED matrix text rendering model
logicaloud Jun 8, 2024
8b0c81a
refactor text render frame locking
logicaloud Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/devices/SenseHat/SenseHat.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,9 @@ public SenseHat(I2cBus? i2cBus = null, bool shouldDispose = false)
/// <inheritdoc/>
public void Dispose()
{
// Terminate text animation timer if active
LedMatrix?.Terminate();

if (_shouldDispose)
{
_i2cBus?.Dispose();
Expand Down
340 changes: 340 additions & 0 deletions src/devices/SenseHat/SenseHatLedMatrix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@
using System.Diagnostics;
using System.Drawing;
using System.Runtime.CompilerServices;
using System.Device.Model;

namespace Iot.Device.SenseHat
{
/// <summary>
/// Base class for SenseHAT LED matrix
/// </summary>
[Interface("SenseHat LED Matrix")]
public abstract class SenseHatLedMatrix : IDisposable
{
/// <summary>
Expand All @@ -32,6 +34,51 @@ public abstract class SenseHatLedMatrix : IDisposable
/// </summary>
protected const int NumberOfPixelsPerColumn = 8;

/// <summary>
/// Lazily intialized pixel font reference. Initialized when rendering text.
/// </summary>
protected SenseHatTextFont? _pixelFont = null;
Copy link
Member

@krwq krwq Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@krwq krwq Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


/// <summary>
/// Lazily initialized text render state. Initialized when rendering text.
/// </summary>
protected SenseHatTextRenderState? _textRenderState = null;

/// <summary>
/// Text color when rendering text.
/// </summary>
protected Color _textColor = Color.DarkBlue;

/// <summary>
/// Text background color when rendering text.
/// </summary>
protected Color _textBackgroundColor = Color.Black;

/// <summary>
/// Text rotation, counterclockwise.
/// </summary>
protected SenseHatTextRotation _textRotation = SenseHatTextRotation.Rotate_0_Degrees;

/// <summary>
/// Text scroll speed when the rendered text does not fit the 8x8 LED matrix
/// </summary>
protected double _textScrollPixelsPerSecond = 1;

/// <summary>
/// Timer used for text animation (scrolling). Lazily initialized as required.
/// </summary>
protected System.Timers.Timer? _textAnimationTimer = null;

// Lock object to maintain a consistent set of render parameters.
private object _lockTextRenderState = new();

// Lock object to prevent i2c disposal during render.
// "Terminate" must be called for clean termination of text animation.
private object _lockWrite = new();

// Flag set to true when no more rendering should take place because the LED matrix is about to be disposed.
private volatile bool _terminate = false;

/// <summary>
/// Constructs SenseHatLedMatrix instance
/// </summary>
Expand Down Expand Up @@ -81,12 +128,14 @@ public static int PositionToIndex(int x, int y)
/// Write colors to the device
/// </summary>
/// <param name="colors">Array of colors</param>
[Command]
public abstract void Write(ReadOnlySpan<Color> colors);

/// <summary>
/// Fill LED matrix with a specific color
/// </summary>
/// <param name="color">Color to fill the device with</param>
[Command]
public abstract void Fill(Color color = default(Color));

/// <summary>
Expand All @@ -95,9 +144,300 @@ public static int PositionToIndex(int x, int y)
/// <param name="x">X coordinate</param>
/// <param name="y">Y coordinate</param>
/// <param name="color">Color to be set in the specified position</param>
[Command]
public abstract void SetPixel(int x, int y, Color color);

/// <summary>
/// Stop animation effects if active.
/// </summary>
public void Terminate()
{
lock (_lockTextRenderState)
{
_textRenderState = null;
StopTextAnimationTimer();
}

lock (_lockWrite)
{
// Prevent further access to the underlying device (i.e. i2c bus)
_terminate = true;
}
}

/// <inheritdoc/>
public abstract void Dispose();

/// <summary>
/// Renders text on the LED display.
/// </summary>
/// <param name="text">Text to render. Set to empty string to stop rendering text.</param>
[Command]
public void SetText(string text)
{
if (string.IsNullOrEmpty(text))
{
StopTextAnimationTimer();
Fill(_textBackgroundColor);
lock (_lockTextRenderState)
{
_textRenderState = null;
}

return;
}

if (_pixelFont == null)
{
_pixelFont = new SenseHatTextFont();
}

var renderMatrix = _pixelFont.RenderText(text);
SenseHatTextRenderState renderState;
lock (_lockTextRenderState)
{
// Create a new render state containing the render matrix for the new text
_textRenderState = new SenseHatTextRenderState(renderMatrix, _textColor, _textBackgroundColor, _textRotation);
renderState = _textRenderState;
}

RenderText(renderState);

StartOrStopTextScrolling();
}

/// <summary>
/// Text color when rendering text.
/// </summary>
[Property]
public Color TextColor
{
get
{
return _textColor;
}
set
logicaloud marked this conversation as resolved.
Show resolved Hide resolved
{
_textColor = value;

// Apply new state. Lock because render state is nullable and assignment may not be atomic.
SenseHatTextRenderState? renderState;
lock (_lockTextRenderState)
{
if (_textRenderState != null)
{
_textRenderState = _textRenderState.ApplyTextColor(value);
}

renderState = _textRenderState;
}

RenderText(renderState);
}
}

/// <summary>
/// Text background color when rendering text.
/// </summary>
[Property]
public Color TextBackgroundColor
{
get
{
return _textBackgroundColor;
}
set
{
_textBackgroundColor = value;

// Apply new state. Lock because render state is nullable and assignment may not be atomic.
SenseHatTextRenderState? renderState;
lock (_lockTextRenderState)
{
if (_textRenderState != null)
{
_textRenderState = _textRenderState.ApplyTextBackgroundColor(value);
}

renderState = _textRenderState;
}

RenderText(renderState);
}
}

/// <summary>
/// Text scroll speed in pixels per second.
/// </summary>
[Property]
public double TextScrollPixelsPerSecond
Copy link
Member

@krwq krwq Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
get
{
return _textScrollPixelsPerSecond;
}
set
{
_textScrollPixelsPerSecond = value;
StartOrStopTextScrolling();
}
}

/// <summary>
/// Text rotation, counterclockwise.
/// </summary>
[Property]
public SenseHatTextRotation TextRotation
{
get
{
return _textRotation;
}
set
{
_textRotation = value;

// Apply new state. Lock because render state is nullable and assignment may not be atomic.
SenseHatTextRenderState? renderState;
lock (_lockTextRenderState)
{
if (_textRenderState != null)
{
_textRenderState = _textRenderState.ApplyTextRotation(value);
}

renderState = _textRenderState;
}

RenderText(renderState);
}
}

private void StartOrStopTextScrolling()
{
SenseHatTextRenderState? renderState;
// Lock because render state is nullable and assignment may not be atomic
lock (_lockTextRenderState)
{
renderState = _textRenderState;
}

int millisecondsPerPixel;
if (renderState == null || renderState.TextRenderMatrix.Text.Length == 0 || _textScrollPixelsPerSecond <= 0)
{
millisecondsPerPixel = 0;
}
else
{
millisecondsPerPixel = (int)(1000 / _textScrollPixelsPerSecond);
}

if (millisecondsPerPixel <= 0)
{
StopTextAnimationTimer();
}
else
{
// Calculate the number of milliseconds for one pixel shift
StartTextAnimationTimer((int)millisecondsPerPixel);
}
}

private void StartTextAnimationTimer(int intervalMs)
{
StopTextAnimationTimer();
_textAnimationTimer = new System.Timers.Timer(intervalMs);
_textAnimationTimer.Elapsed += TextAnimationTimer_Elapsed;
_textAnimationTimer.Start();
}

private void StopTextAnimationTimer()
{
if (_textAnimationTimer != null)
{
_textAnimationTimer.Stop();
_textAnimationTimer.Elapsed -= TextAnimationTimer_Elapsed;
_textAnimationTimer.Dispose();
_textAnimationTimer = null;
}
}

private void TextAnimationTimer_Elapsed(object? sender, System.Timers.ElapsedEventArgs e)
Copy link
Member

@krwq krwq Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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)

Copy link
Author

@logicaloud logicaloud Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
SenseHatTextRenderState? renderState;
// Lock because render state is nullable and assignment may not be atomic
lock (_lockTextRenderState)
{
renderState = _textRenderState;
}

if (renderState != null)
{
var renderMatrix = renderState.TextRenderMatrix;
if (renderMatrix.Text.Length > 1)
{
renderMatrix.ScrollByOnePixel();
RenderText(renderState);
}
}
}

private void RenderText(SenseHatTextRenderState? textRenderState)
{
if (textRenderState == null)
{
return;
}

// Allocate frame buffer to hold color values for one frame
var frameBuffer = new Color[NumberOfPixelsPerColumn * NumberOfPixelsPerRow];

// Render the 8x8 matrix
for (var x = 0; x < NumberOfPixelsPerColumn; x++)
{
for (var y = 0; y < NumberOfPixelsPerRow; y++)
{
int tx = x;
int ty = y;
switch (textRenderState.TextRotation)
{
case SenseHatTextRotation.Rotate_0_Degrees:
break;
case SenseHatTextRotation.Rotate_90_Degrees:
tx = y;
ty = NumberOfPixelsPerColumn - x - 1;
break;
case SenseHatTextRotation.Rotate_180_Degrees:
tx = NumberOfPixelsPerColumn - x - 1;
ty = NumberOfPixelsPerRow - y - 1;
break;
case SenseHatTextRotation.Rotate_270_Degrees:
tx = NumberOfPixelsPerColumn - y - 1;
ty = x;
break;
}

var frameIndex = PositionToIndex(tx, ty);

if (textRenderState.TextRenderMatrix.IsPixelSet(x, y))
{
frameBuffer[frameIndex] = textRenderState.TextColor;
}
else
{
frameBuffer[frameIndex] = textRenderState.TextBackgroundColor;
}
}
}

// Lock write so that SenseHat disposal does not interfere
Copy link
Member

@krwq krwq Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

lock (_lockWrite)
{
if (!_terminate)
{
Write(frameBuffer);
}
}
}
}
}
Loading