boneskull commented 9 years ago

After trying to tweak LedControl.js to work with a 24-segment, bi-color bar-graph, I've come to the conclusion that the code needs some refactoring, because it scales poorly.

if you are attempting to use the HT16K33 controller in the context of a bar graph (since that's the chip the bar graph backpack uses), certain functions should not be available, and others (or rather, one, setBar()) should be.
it is difficult to reuse code in a controller given the common pattern of defining controllers; no "subcontrollers" are allowed
Adding new functions requires explicitly adding them to the context of the LedControl instance (which could be done via something like _.mixin() or _.assign())
controllers are not sufficiently encapsulated as to offer unique implementations for a display using a unique IC; the HT16K33 chip can drive all sorts of displays, not just traditional matrices or arrays of matrices.
Matrix dimension restrictions should be shelved except in the case of hardware-specific controllers. I don't need the code to tell me that driving six LEDs with a 74595 is not allowed!
I don't understand what a DEFAULT controller buys us, if LedControl.prototype is non-empty.

To solve this, I would expect to use either inheritance or composition to extend the LedControl object with the proper functions.

Three (3) solutions follow.

Using inheritance:

The DEFAULT controller's general-purpose methods are folded into LedControl.prototype, if any. For all intents & purposes, LedControl is abstract.
The HT16K33 pseudoclass would be a static property of LedControl, and extend LedControl, providing specific implementation for this chip. It contains no code which assumes any given matrix configuration.
Matrix, Digit (or SevenSegment), BarGraph and even Alphanumeric would be static, class-level psuedoclasses in HT16K33, containing code specific to each implementation. The Adafruit_LEDBackpack lib has implementation for all of these.
The ShiftRegister74595 (for lack of a better name) pseudoclass would do likewise.
The organization scheme here is simply for sake of organization and is not required.

Use the factory pattern to grab the class you want:

LedControl.create = function create(type, controller, opts) {
return new LedControl[controller][type](opts);
};

// alternatively:
LedControl.createMatrix = function createMatrix(controller, opts) {
 return new LedControl[controller].Matrix(opts);
};

LedControl.createBarGraph = function createBarGraph(controller, opts) {
 return new LedControl[controller].BarGraph(opts);
};
// etc

Using composition:

Fold the general-purpose functions from DEFAULT back into LedControl.prototype, if any.
Create an data structure similar to the above, but use static objects instead of classes.
Require a type string param upon instantiation, or default to 'matrix'. Require a controller param like we currently do, and default to '74595'. Or use the factory pattern above.
These two params are used to find the implementation, and within the LedControl constructor, set this value to this.imp. Since controllers and "types" would each have implementations, we probably have to mix them together.
In LedControl.prototype, for each unique function in each implementation, expose a function which delegates to the same function in this.imp.
If the function doesn't exist in the implementation, we can toss an error, or let Node throw a TypeError.

Using more mixins:

Ginsu the mixins. A controller-level mixin object would have methods common to each "type" for a given controller, and each type would have its own implementation(s). For example:

LedControl.HT16K33 = {
commonFunc: function() { 
  // ...
},
matrix: {
  matrixyThing: function() { 
    // ...
  }
},
bargraph: {
  bargraphOnlyFunc: function() { 
    // ...
  }
}
};

Change the mixin strategy. When mixing in a controller, mix in functions only, then descend into the "type" object, and mix that in too. Do this via a loop (or _.mixin()) instead of explicitly defining the function properties on the context (see LedControl constructor).

Given that prototypal inheritance is fast, I don't see either of the first two strategies having a significant performance drawback. Mixins ("parasitic inheritance"?) are slower, though I don't know if it matters.

The "leaf" level implementations ("types" within controllers) should enforce the matrix dimensions. Seven-segment displays will have a matrix of n * 8 where n is the number of displays, and if we assume bar graph displays to have 12 bars, n * 12. Perhaps this would necessitate an init() function for each "leaf" implementation, if inheritance is not used.

I'd recommend using inheritance here, because the implementations are not guaranteed to have the same set of methods--it won't scale if a new method or controller is added. Additionally, we avoid having to explicitly call an initialization function. To me, composition makes more sense when you have a strict set of functions to be implemented, and this can be enforced (think interfaces in Java).

Anyway, since I'm in here trying to get my damn bar graph to work, I can do this and at least present you the result.

rwaldron commented 9 years ago

That's a lot to read. I'll need some time to consider and respond.

rwaldron commented 9 years ago

if you are attempting to use the HT16K33 controller in the context of a bar graph (since that's the chip the bar graph backpack uses),

Why not create an Led.BarGraph class?

var graph = new five.Led.BarGraph({
  controller: "HT16K33"
});

certain functions should not be available, and others (or rather, one, setBar()) should be.

This is a nit-pick, but there is no setFoo here.

var graph = new five.Led.BarGraph({
  controller: "HT16K33"
});

// number of bars to light up
graph.bars(3);

// array of "bar" objects, index => bar
graph.bars([
 { color: "red", on: true }, // on = true by default for any bars present
 { color: "red" }, 
 null, 
 { color: green }
]);
// This would light up bars 0 and 1 with the color red, 2 would be off, 3 would be green (left to right, top to bottom)

it is difficult to reuse code in a controller given the common pattern of defining controllers; no "subcontrollers" are allowed

Controllers aren't really meant to offer re-usable code, quite the opposite: they provide the component or device specific code.

Adding new functions requires explicitly adding them to the context of the LedControl instance (which could be done via something like .mixin() or .assign())

Those literally do the same, but with the overhead of a function call and loops :P

Matrix dimension restrictions should be shelved except in the case of hardware-specific controllers. I don't need the code to tell me that driving six LEDs with a 74595 is not allowed!

I don't follow this... clarify?

I don't understand what a DEFAULT controller buys us, if LedControl.prototype is non-empty.

Its the controller used when no controller: "..." is omitted, which is an implementation that writes to a shift register. It exists only so that existing code didn't break when the system was moved to controller based design. "Default" is the one that was "here first".

instead of explicitly defining the function properties on the context (see LedControl constructor).

Yes, this is weird and I've never liked it, but had no time to refactor. Those properties should've actually been defined in the context of a descriptor, then defined on the instance as Object.defineProperties(this, controller).

...

Essentially this looks like the Motor problem. Take a look at how @dtex designed the Motor class: there are devices and controllers, with a clear distinction between them. I think you want a similar design.

ps. please don't make these: LedControl.createMatrix, LedControl.create etc.

boneskull commented 9 years ago

@rwaldron

Hmm, I think I need to be a bit more clear. The isMatrix flag, for example, causes extra conditionals within functions to check for it (see also isBicolor). This is an antipattern; it doesn't scale. Nor does checking dimensions at the constructor level, nor does explicitly mixing functions in. Basically, we're jamming too much stuff into each controller.

Let me rephrase then. If the controllers are to be "final" implementations, then I'm arguing one of two things should happen:

controllers are organized by chip and become types: "matrix", "seven-segment", "bargraph", or "alpha" implementations
controllers are organized by types "matrix", "seven-segment", "bargraph", or "alpha" and implementations are per-chip

The latter seems similar to the rest of the codebase, where controllers have a relationship to a bit of hardware. Either way, given two chips (74595 and HT16K33), there'd be eight total controllers, assuming each type had an implementation. This does not even take into account that you can have a bicolor matrix, which would cause some implementations to change; to avoid problems here, we'd want to use inheritance or composition. Adding something like support for a RGB LED matrix would possibly add another flag if we stuck with the current design; refactoring to support future expansion would make it easier to add new features and maintain.

So, to add the setBar() (as it's called in the Adafruit_LED_Backpack lib) or bar() function to the current HT16K33 controller, I'd have to add another flag, add a minimum of one function (but probably more in the spirit of DRY; the bit of led() where we're writing to the buffer would be duplicated), add more dimension checks, another class under Led, etc.

Because we'd have a lot of controllers, if we decided to use some form of prototypal inheritance here, an implementation of the GoF Factory Pattern would be appropriate; hence createBarGraph(), createMatrix(), etc. This is similar to what goes on in the LedControl constructor atm, but would happen before the object is constructed.

I'm not sure why we're aliasing LedControl in johnny-five.js either; why not just export some object from ledcontrol.js and _.assign(Led, require('ledcontrol')) in led.js? led.js is getting rather large, and perhaps we should consider splitting it up & putting it into its own folder (along with matrix.js, bargraph.js, rgb.js, array.js, etc.).

rwaldron commented 9 years ago

Hmm, I think I need to be a bit more clear. The isMatrix flag, for example, causes extra conditionals within functions to check for it (see also isBicolor). This is an antipattern; it doesn't scale

Yep, I agree. (We have a problem with isBicolor... it's already in the book)

Nor does checking dimensions at the constructor level, nor does explicitly mixing functions in. Basically, we're jamming too much stuff into each controller.

Agreed.

I'm not sure why we're aliasing LedControl in johnny-five.js either; why not just export some object from ledcontrol.js and _.assign(Led, require('ledcontrol')) in led.js

~~This is irrelevant to the task at hand (see below)~~

Sorry, I didn't mean that. Striken

...

LedControl is legacy and it's clear to me that you have a plan. I want you to rewrite the class, use the existing classes that have it right as a guideline, along with your plan. I'll contribute/advise at the highest level (API surface, etc), @BrianGenisio and @dtex will advise on the internals.

here's a few notes:

LedControl - I don't care about this class name, take it or leave it.
With the exception of anything using LedControl + isMatrix, I want all existing code to "just work". That might be hard and possibly even an insane ask, but you're a smart guy, and I'm confident you can make that happen.
I really like this: Led.Matrix, Led.Digits, Led.BarGraph, Led.Foo and so on. Put your re-write in its own file that exports an object with each of these things and then you can do this: "_.assign(Led, require('ledcontrol')) in led.js" (from your suggestion above).

rwaldron commented 9 years ago

led.js is getting rather large, and perhaps we should consider splitting it up & putting it into its own folder (along with matrix.js, bargraph.js, rgb.js, array.js, etc.).

Yeah, lib/led-components/*.js, something like that?

dtex commented 9 years ago

A quick point about controllers. We have two levels of classification: controllers and devices. Devices are the "types" referred to above ("matrix", "seven-segment", "bargraph", etc). Controllers should be read as "interfaces". They are how we communicate with or control the device.

User methods could be bound to devices and the specifics of the controller would be hidden from the user. Motor.js is the best example so far, where we have devices (motor, directional motor, CDIR motor) and controllers (Shift Register, PCA9685, etc). Global methods can be defined on the class prototype. Methods that are unique to a device (formerly "type") are defined there, and specifics to a controller implementation are defined there. So, a single instance has its methods and properties composed using defineProperties to "decorate" the prototype with the appropriate device and controller

boneskull commented 9 years ago

@rwaldron @dtex @BrianGenisio

Out of curiousity, what is the function of priv? Why do we make a state object and priv.set(this, state)?

@rwaldron Alright, I'll show you what I come up with! I'm certainly more of a JavaScript guy than a hardware guy...

rwaldron commented 9 years ago

priv is used to store instance private data—this pattern is used throughout Johnny-Five. The state object is initialized in the constructor to allow constructor-local access to that reference. The private side table can then be accessed anywhere that this is present.

boneskull commented 9 years ago

@rwaldron ...in the module only, though, right?

rwaldron commented 9 years ago

Correct—this pattern is used instead of putting icky _fakePrivate properties all over the instances.

ajfisher commented 9 years ago

So I've stumbled into this thread as I wasn't really paying attention at the time it was first discussed. I've just tried to add a new controller for the TM1640 and have run into many of the similar issues as @boneskull has raised here.

As such, what's the current state of this? I have something that approximates a working 16x8 display but it's grotty beyond belief.

If this hasn't progressed any further or no one wants to own it then I'm happy to pick it up as I think I can give it a reasonable crack as I have the existing chips available (Max and HT chips), a few matrices of different sizes and some 7-segment displays. I also want to get it working as I have a lovely new, 16x8, blue display from a kickstarter and it would be nice for it to work properly.

rwaldron commented 9 years ago

Here's a copy of my reply on gitter, just to keep it all on record...

is there a reason why 7-seg stuff and matrix stuff are so interwoven together?

The original LedControl class was actually an "almost" 1-to-1 port of the LedControl C library. That library is designed for controlling 7 Segment and Matrix displays, specifically with the MAX7219 and MAX7221 chips. The port was contributed in the first few months that Johnny-Five existed (much like the Wii classes), which obviously predates the component = controller [+ device] system. Sometime last year I attempted to update the class, but was wary of breaking changes, which resulted in a sub-par update. My hope was that someone would take ownership of the class and do a comprehensive re-write (similar to @dtex's work in Motor or Servo, and @BrianGenisio's overhaul of the sensor classes).

I get that you can control them using the same chip but just wondering why the controller interface is so interlinked when the use cases are so different?

You're not alone here—this was sort of my motivation for making the aliases LedControl => [ Led.Digits, Led.Matrix ]. I wanted to treat them as different entities so they could have documentation that clearly defined them as unique component classes.

Ideally, I'd like to see this:

Two new classes created, separate from the existing classes
- Led.Display.Digits
- controllers for N-segment digit displays
- Led.Display.Matrix
- controllers for N-dimension matrix displays
Eventually those will replace the existing classes
Must support all features currently supported.

boneskull commented 9 years ago

Two new classes created, separate from the existing classes Led.Display.Digits controllers for N-segment digit displays Led.Display.Matrix controllers for N-dimension matrix displays

for my use case, I wanted to add Led.Display.BarGraph. unfortunately I haven't had much time to play around in j5's guts lately

boneskull commented 9 years ago

but anyway, @rwaldron 's comments would address my concerns. it's difficult to maintain and extend the way it is now.

rwaldron commented 9 years ago

Led.Display.BarGraph

Sorry, forgot about that! But yes, this should be trivial to add once all refactor is complete

ajfisher commented 9 years ago

Well I have one of these to hand as well (got a mixed bag of stuff given to me the other day with matrices, bar graphs and 7-segment displays)

I'll post a design here first in terms of how I think it will work.

On Wed, Aug 12, 2015 at 5:08 AM Rick Waldron notifications@github.com wrote:

Led.Display.BarGraph

Sorry, forgot about that! But yes, this should be trivial to add once all refactor is complete

— Reply to this email directly or view it on GitHub https://github.com/rwaldron/johnny-five/issues/665#issuecomment-130022771 .

ericandrewlewis commented 8 years ago

@boneskull do you mind if I have a go at this?

ajfisher commented 8 years ago

has anyone had a crack at this as I feel like I have enough devices of the varying types as well as familiarity with the way LEDs work and J5's approach to refactor this but don't want to duplicate effort if anyone is having a go at it already.

Resseguie commented 8 years ago

@ajfisher did you see #1016 ?

ajfisher commented 8 years ago

I did - though it looks #1016 has hit the same point I did when I went to implement a controller for the mBot matrix which used a different chip again to provide control - namely that ledcontrol really needs a rework at the bottom level to stop it being so device specific.

On Fri, Feb 5, 2016 at 2:05 PM David Resseguie notifications@github.com wrote:

@ajfisher https://github.com/ajfisher did you see #1016 https://github.com/rwaldron/johnny-five/pull/1016 ?

— Reply to this email directly or view it on GitHub https://github.com/rwaldron/johnny-five/issues/665#issuecomment-180171333 .

ericandrewlewis commented 8 years ago

@ajfisher feel free to take on the refactor! You definitely have more domain knowledge than I do.

rwaldron commented 8 years ago

@ericandrewlewis you should finish up yours first, right? The devices you need are all listed on these pages:

boneskull commented 8 years ago

I am in the middle of writing a tome of a gist surrounding design considerations for this. By no means should you wait until I have something up, but wanted to say this is coming (soon).

ericandrewlewis commented 8 years ago

@ericandrewlewis you should finish up yours first, right?

I would assume the sort of refactor by @ajfisher is suggesting would supersede the need, but I'm glad to continue if not.

ajfisher commented 8 years ago

Okay - apologies for the fairly largish reply onto this but felt more usable here for comment rather than in a gist where I originally had it.

Multiple LED refactor proposal

So this has come about because the existing J5 LED controllers for things like matrix / digits etc are too closely related to their original C implementations and it's made it difficult to port these for other controllers etc.

Coming at this from the perspective of NodePixel and using these devices a lot, here's how I think a refactor could work.

My sense of this is that we have to build through the layers because there are a variety of use cases that need to be addressed and whilst you may have a 16x8 matrix you're working with, you have an immediate need to turn the LED at (2,3) on or off one minute but then scroll text across it the next. I think this can address these various cases.

Use cases I think we need to consider:

I have a 7 segment display and I want to show a countdown on it.
I have a 7 segment display and I want to make my own patterns
I have a 14 segment display and want to write letters
I have a matrix and I want to draw a game of tetris on it
I have a matrix and want to write a number or letter
I have a matrix and want to draw a custom character
I have a bar graph I'm using as a meter and want to display 37% full

An of course all of those can be multiplied by:

I have a 2 colour X and want to...
I have a 3 colour X and want to...

To accommodate all of this here's what I reckon may work, following the whole device, controller structure.

At the base level is a single LED which would have the same functionality as the current LED (noting that unless you direct wire the LED items on one of these devices you can't PWM them).

From there we have display elements which are a composition of LEDs based on what you have - so could be single, bicolour or tricolour. From an ease of use perspective you'd probably designate certain common types that reflect usual behave (eg vast majority of bicolour elements are R/G because they are cheap).

Thus you'd interact with it as:

element.r.on()
element.g.off()

etc

Now this assumes you have a controller for those elements which is where the controller side of things starts to come in. A controller could allow for being directly wired to a pin on the board or else working through one of the usual methods (shift reg, MAX7219, TM1640, HT16K33 etc). A controller at this level provides the specific handling for manipulating this element (which is really as simple as managing state and then turning it on / off etc with the appropriate io plugin messages).

Moving up a level we then have a device like normal and make that nice to interact with, thus:

Strip - used for 1D linear arrangements of elements - such as a bar graph or just a strip of LEDs. Matrix - used for 2D arrangements of elements - usual 2D matrices that we use a lot. Display - used for arrangements of elements in classic 7 and 14 segment arrangements

Again, each of these have a Controller as is appropriate and depending on what that controller allows for, would handle things like chaining etc.

All three of these devices would allow for a single element to be referenced to it could be manipulated directly (thus allowing for custom control where needed). eg:

strip.elements(0).on()
matrix.elements(0).on()
display.elements(0).on()

Would all turn on the first referenced LED regardless of the arrangement.

As you step up, each device would then give you interfaces that make sense to that type of arrangement eg:

Strip

.on() - turns the whole strip on .off() - turns the whole strip off .percent(val) - calculates length of strip and "fills" (turns on) appropriate number of LEDs. .shift(x) - shifts element values x positions (negative towards zero side of axis and positive vice versa)

Matrix

.on() - turns the whole matrix on .off() - turns the whole matrix off .clear() - blanks everything on the matrix (sets states to 0) .row(y) - gives you reference to the whole row given by y .col(x) - gives you reference to the whole column given by x .shift(x, y) - shifts elements x along x axis and y along y axis .draw([byte array]) - writes the bytes to the individual elements to draw a pattern .write(string) - writes a character to the matrix

Display

.on() - turns the display on .off() - turns display off write(string) - writes the character to the display

Representing multi-colour elements at device level

So in many cases the device is single colour (a green matrix or a red display etc). However bi and tri colour displays exist which we need to account for. In any method that draws an output (percent(), row(), col(), draw(), write() etc) you would pass an optional colour value which is up to a 3 bit value as appropriate for the type of device you have - eg:

Assume I have a 7 segment display that is 3 colour.

display.write("A", 7); // white "b111"
display.write("B", 1); // blue "b001"
display.write("C", 3); // cyan "b011"

So that's what I'm thinking - I have a stack of these devices across a range of different types and controller types as well so I feel I have a good range of things to work with and a lot of the way this works is reflected in the shape stuff that's going to drop in NodePixel too so makes sense for it to have some similarities.

If this seems like a good approach then I'm happy to just get cracking.

ajfisher commented 8 years ago

And just further to this - anything that currently "works" with respect to digits and matrix I'd see us managing through even if it's not entirely "right" into the future. But at least we can manage backward compatibility as best as possible.

boneskull commented 7 years ago

@ajfisher Did anything come of this?

Anyway, since nobody replied, and if you were waiting for approval, I would say your proposal LGTM. :wink:

boneskull commented 7 years ago

Given strip.elements(n).on() perhaps some sugar like strip.elements(n).yellow() would be appropriate for bi-color displays. However, red/green bi-color displays aren't the only variation of bi-color display...

boneskull commented 7 years ago

Insofar as HT16K33-driven bi-color bar graphs are concerned, I have this implementation. The specifics therein may be useful

stale[bot] commented 6 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

rwaldron / johnny-five

refactor LedControl #665

Multiple LED refactor proposal

Strip

Matrix

Display

Representing multi-colour elements at device level