Added support for TM1637 displays

[GioCC]

Replaced stock LedControl library with dual MAX/TM library; increased display number for smaller Arduinos from 1 to 3

Description of changes

Fixes #251
Adding these units as new, independent devices is almost unfeasible because of memory (mainly flash) constraints on the smaller (Arduino) boards.
This implementation extends the standard MAX72xx LedControl driver with a dual low-level layer, keeping perfect compatibility in the upper interface layer; this way, it doesn't call for the overhead of an additional device type, and also it doesn't require changes in the message set.

[github-actions]

Firmware for this pull request:
Firmware.zip

[elral]

After my vacation I now started to have a look on this PR.

Just a short questions, a TM1637 is identified if no CS pin is defined? /edit: see below

Up to now we have a "own" library for the MAX segments. Do we change the library or do we keep the new files under src/MF_Segment/...? In the second case we don not need anymore lib_deps = https://github.com/MobiFlight/LedControl#1.1.0

I haven't build my own opinion for now...

[elral]

Oh, and another one. Which value is coming from the connector for a undefined pin? I was thinking about this when working on the custom device. I thought about 0x-1 or 0xFF. 0xFF could be a problem as some(all?) pins are handled as 8bit integer, but I have to check this again.

/edit: However, it's a clever idea to handle TM1637 displays this way!

/edit2: OK, I guess I got it. If pin CS has a valid pin (< 0xFE) than it is a MAX display. If CS is 0xFE it is a 6digit and if 0xFD it is a 4 digit TM display. But why is the function bool    isMAX(void) { return (IO_CS + 1) < TM_6D; } set up this way? Wouldn't it better/more clear to define bool    isMAX(void) { return (IO_CS) < TM_4D; } ?

[GioCC]

Hi @elral, hope your vacation went great!
Thanks for taking the time for having a look at the code.

Just a short questions, a TM1637 is identified if no CS pin is defined? /edit: see below

More precisely, if it has a "magic" value - see next reply

Up to now we have a "own" library for the MAX segments. Do we change the library or do we keep the new files under src/MF_Segment/...?

The stock LedControl library is replaced by the new LedControl_Dual library that manages both devices (the MAX part is identical to the previous one).
Files under MF_Segment/ are a higher layer; they are intentionally 99% unchanged (except for an include). The name of the class has remained LedControl (as opposed to LedControl_Dual) for compatibility.
Since the previous LedControl library was out of our code space - and indeed not in src/MF_Segment/, as no other library is - but now it has become proprietary (meaning "belonging to the project"), I saw fit to place it in src/. Of course it can be moved elsewhere.
Of course this is already all set up in the PR.

In the second case we don not need anymore lib_deps = https://github.com/MobiFlight/LedControl#1.1.0

Exactly, as per the modified platformio.ini.

[GioCC]

Oh, and another one. Which value is coming from the connector for a undefined pin? I was thinking about this when working on the custom device. I thought about 0x-1 or 0xFF. 0xFF could be a problem as some(all?) pins are handled as 8bit integer, but I have to check this again.

Pins are managed internally as uint8_t, so 0xFF is suitable while 0x-1 could cause issues.
The connector does never pass an "undefined" pin: as you correctly write below, for TM1637s only it transmits the "magic values" 0xFD for a 4-digit or 0xFE for a 6-digit. (ints are then used for the serial transmission, but that's irrelevant).

/edit2: OK, I guess I got it. If pin CS has a valid pin (< 0xFE) than it is a MAX display. If CS is 0xFE it is a 6digit and if 0xFD it is a 4 digit TM display. But why is the function bool    isMAX(void) { return (IO_CS + 1) < TM_6D; } set up this way? Wouldn't it better/more clear to define bool    isMAX(void) { return (IO_CS) < TM_4D; } ?

That's a safeguard: it also covers the case of CSpin = 0xFF (theoretically never happening, at least currently). That's because uint8_t 0xFF + 1 = 0x00, which - albeit now meaningless - would correctly identify as not a TM1637.
To be also semantically clean (and not just correct), the function should better read
isNotMAX(void) { return (IO_CS + 1) < (TM_4D+1); }

[elral]

Hi @GioCC,

Hi @elral, hope your vacation went great!

Yes, it was! Thanks!

---

Since the previous LedControl library was out of our code space...

This library is forked and is therefore under our own control. Compared to the original one there are some modifications done. So the changes could be implemented in our fork, but as there are now new functions I would keep it as it is now. Compared to other devices the naming of the files is not really consistent. We should take the chance to have it like the other devices. LED in the filename is not required anymore as the functions to control a single LED on the MAX are all deleted (instead of not used per #define up to now).

• LedSegment.cpp/.h into Segments.cpp/.h (fits to MFSegments.cpp/.h )
• LedControl_dual.cpp/.h into e.g. ControlSegments.cpp/.h (I am not good in naming )and move it to /lib ? Or start the file name with an underline?

---

I haven't looked in detail to the charTable[128] . This table is modified compared to the original MAX library. Did you used our modified one?

---

Exactly, as per the modified platformio.ini.

There is no modified platformio.ini file in this PR.

---

Pins are managed internally as uint8_t ....

Unfortenutely not always, see void MFSegments::attach(int dataPin, int csPin, int clkPin, byte moduleCount, byte brightness) . And I found in Servos.cpp void Add(int pin) . For the first one it could be changed within this PR. For the second I can add it to my planed issue/PR with same small modifications to save a little RAM and Flash usage.

That's a safeguard: it also covers the case of CSpin = 0xFF ....

If CSpin is 0xFF this pin is uninitialized according your definition enum { TM_4D = 0xFD, TM_6D = 0xFE, UNINITIALIZED = 0xFF}; and shouldn't occur. So it's not a TM1637 display and not a MAX display. To be on the absolute safe side this should be checked first, but I think it's overkill.  So for better understanding for other users and for working on it in the future I would prefer to have bool isMAX(void) { return (IO_CS) < TM_4D; } 

---

#ifndef __LEDCONTROL_DUAL__H__
#define __LEDCONTROL_DUAL__H__

Shouldn't we use #pragma once like in the other .h files?

---

In LedControl_dual.cpp line 391 and 397, I can remember from another PR that Sebastian asks me to change this to an #else .

Regards

Ralf

[github-actions]

Firmware for this pull request:
Firmware.zip

[GioCC]

Hi @elral, see my replies below.

Since the previous LedControl library was out of our code space...

This library is forked and is therefore under our own control.

Yes, but I said "out of our code space" (meaning of the current project), not "out of our control".

Compared to the original one there are some modifications done. So the changes could be implemented in our fork, but as there are now new functions I would keep it as it is now.

By "as it is now" I gather you mean you mean "as in the PR"?
In this case I'd obviously agree. Despite being substantially unchanged for the MAX part, I believe this should be considered a different library, not an extension of the original LedControl.

Compared to other devices the naming of the files is not really consistent. We should take the chance to have it like the other devices.
LED in the filename is not required anymore as the functions to control a single LED on the MAX are all deleted (instead of not used per #define up to now).

It is a library specifically for 7-segments LED displays, so I don't see anything wrong by referencing LEDs. But of course neither does that prevent from naming it differently.

• LedSegment.cpp/.h into Segments.cpp/.h (fits to MFSegments.cpp/.h )
• LedControl_dual.cpp/.h into e.g. ControlSegments.cpp/.h

This could be an option. But then to me it would make more sense to leave LedSegment.* as it is and rename MFSegments.* to MFLedSegments.*.
Or, while we are at it, give them a correct name, like:

• LedSegment.* -> LedDisplay.*
• MFSegment.* -> MFLedDisplay.*

Anyway, this name change would not really have to do with any change made by this PR, and if anyone is inclined to do it, it could be done as a separate PR, as is usually the case.

and move it to /lib ?

That would be the most meaningful place IMHO. I would have placed it there first, had I not expected a discussion on these topics anyway.

Or start the file name with an underline?

Ugly. If something meaningful is desired, it should be on the lines of "DualLEDDisplayDriver.*".
(Even the original "LedControl" was hardly a meaningful name.)

I haven't looked in detail to the charTable[128] . This table is modified compared to the original MAX library. Did you used our modified one?

Yes, as I said, the MAX part is basically unchanged from the stock one (except also for unused parts removed for clarity and possible interference).
A similar table was used by the TM library I took as a model, but I rewrote it to be able to use the same set of data, which was a must in order to fulfill space constraints.

There is no modified platformio.ini file in this PR.

You're right, I inadvertently didn't include it because I had also temporarily disabled the Pico part. Fixed now.

Pins are managed internally as uint8_t ....

Unfortenutely not always, see void MFSegments::attach(int dataPin, int csPin, int clkPin, byte moduleCount, byte brightness).

That's another layer, and it simply passes the values transmitted (also as generic int) through the CommandMessenger. In the library (and once they are finally, actually interpreted as pin values), they are uint8_ts, as pins are currently required to be.

And I found in Servos.cpp void Add(int pin). For the first one it could be changed within this PR. For the second I can add it to my planed issue/PR with same small modifications to save a little RAM and Flash usage.

First, I don't think that's appropriate; functions at this level are agnostic with respect to values passed, and a wider type is necessary to accomodate different value ranges, both present and future.
Besides, I don't think the gain would be even worth it; they are just a few arguments in a few function calls, and they aren't even stored values (not as ints anyway).
There are other places where changes like this would be be a little beneficial (see for example GioCC@a5a7464), but even there, when I tried to make similar changes they were rejected because they apparently "did not add anything" to the code.

If CSpin is 0xFF this pin is uninitialized according your definition enum { TM_4D = 0xFD, TM_6D = 0xFE, UNINITIALIZED = 0xFF}; and shouldn't occur. So it's not a TM1637 display and not a MAX display.

Exactly. But since the check is used anywhere only to detect TM displays, the most correct version (regardless for the naming) is the one that ONLY "passes" TM codes.

To be on the absolute safe side this should be checked first, but I think it's overkill.

Agreed. And unfortunately checks cost Flash space, so it would not be the only place were we want to rely on a certain degree of correctness of other code.

So for better understanding for other users and for working on it in the future I would prefer to have bool isMAX(void) { return (IO_CS) < TM_4D; }

Which, as I explained above, looks nicer but it would be even less correct and in the end more dangerous.
If a better naming is desired, the alternate one I suggested could be changed to isTM(void) { return (IO_CS + 1) >= (TM_4D+1); }.

#ifndef __LEDCONTROL_DUAL__H__
#define __LEDCONTROL_DUAL__H__

Shouldn't we use #pragma once like in the other .h files?

We could. It was that way in the original LedControl.

In LedControl_dual.cpp line 391 and 397, I can remember from another PR that Sebastian asks me to change this to an #else .

In this case I disagree. These are not alternate versions of the same code; these are two distinct pieces of code, each made available in a different build configuration.

BTW, on a more substantial note (which I think are more relevant topics to discuss):
at this stage I left these compilation options available for completeness, in order to allow a better overview of the code.

The LEDCONTROL_EXTENDED switch adds a few functions to allow displaying strings and numbers; these functions are currently neither required nor useful (except for testing), because the peculiarity of the MAX dictates that anything is sent digit-wise.
But I wanted to make known that these functions are there, and working, should the opportunity arise in the future.

The LEDCONTROL_NO_BUF switch (if false) allows the TM part to use the mandatorily present MAX buffer to possibly speed up the transfer of a block of data to the TM, by initially filling the buffer (without needlessly transmitting) and delay transmission until after the last digit.
Again, this is currently a moot point because the MAX forces us to send data bytewise (and LEDCONTROL_NO_BUF is true), unless we want to change the Connector and extend the command set; however, with the TM, this results in a noticeable "flicker" in the digit refresh, which might result uncanny. If a later improvement should be desired, these functions are available and ready.
For completeness: LEDCONTROL_NO_BUF=false requires that every instance of display has its own buffer (otherwise, partial buffer writes could overlap between instances). Since the MAX configuration requires the buffer to be 16-byte long, the RAM usage would increase sensibly. An improvement could be to use a shared (static) 16-byte buffer for MAXes and a buffer only 6-byte long for each instance.

Thanks again for your feedback, I'm looking forward to further observations!

[GioCC]

For reference, see alco comment MobiFlight/MobiFlight-Connector#1252 (comment)
in the corresponding Connector PR.

[github-actions]

Firmware for this pull request:
Firmware.zip

[github-actions]

Firmware for this pull request:
Firmware.zip

[github-actions]

Firmware for this pull request:
Firmware.zip

[DocMoebiuz]

We could also save some space if we don't use the first 4 lines. We would just to make sure that we are always requesting actual chracters > 32