Need Critique of Automatic Neopixel library

1

I’d like a critique of my automatic conversion of the Adafruit NeoPixel library. It’s a proof-of-concept, so what should be done differently/better?

It’s published, so “Add Library…”, or examine code.

Keep in mind that this used an automatic process, to produce a minimally useful library for XOD.

My goals:

  • The naming should reflect the orginal Arduino IDE library, so people can find it, and know what it is.
  • Do a directy translation of the class and it’s methods as patches.
  • Usable, but may need some polishing.

I try to credit the original library. See discussion.

I tried to name the library close to the original, though I made a mistake with this one. It was supposed to be awgrover/adafruit-neopixel-ll, but shows up on the library list as awgrover/adafruiteneopixel. Would that have been a good name?

The name is supposed to reflect that it is a low-level translation from some other library. I couldn’t think of anything better than “ll” for low-level. Is there a better signal?

I assumed that this library would get fixed, wrapped, etc., to make it friendlier for XOD.

I assumed the library was a single class, so I produced patches for the constructors and each method. A direct conversion seemed the most obvious way to get something useful.

I had to change the spelling of the methods, from intercapped to dashed.

Some other libraries use a x-device name for the constructors. I used the class name, which is almost the same as the library name. Does that make it easier to find the constructor? Or confusing by not following the conventions?

From another discussion it seemed like the patches should be controlled by pulses. They cause state-change, so aren’t dataflow or functional.

The object is an input/output to every method, and I assumed that buses would get used a lot. Does that seem right?

There are no public instance-variables. I was going to try generating the equivalent of a getter & setter for them. Again, with pulses.

I did a simple mapping of all numeric types to Number. I think this is not ideal, and may cause some problems eventually. You can see the uint16_t, etc. What do you think of this loss of type?

I wrote some examples, to test the library out a bit. That’s hand writing, not mechanical. Examples seem like a good idea?

It looks like libclang's cindex is not reporting all of the declarations to me. So, a few methods, and some arguments are missing. This is the sort of thing that would need to be fixed by hand. Cf. the static Color, the fill(), etc.

It seems necessary, sometimes, to change data to a pulse. For example, if a counter was updating a set-pixel-color, and choosing the pixel (like the example-simple). But, set-pixel-color only acts on a pulse. So, somehow, we have to generate a pulse. The only way I know to do that is with something like awgrover/conversions/data-to-pulse. How should we bridge the functional/impure worlds?

The Neopixels need to know which order the RGB is in, which is handled by a list of values in the Arduino library (not even an enum!). I should use an enum for those when it becomes available. But, that also requires some manual intervention to get the values out of the Arduino library.

And, finally, look over the library. Should it have been done differently?

1 Like
2

To put in summary: it is cool :heart:

TL;DR

Seeing the library page I have no chance to overlook that Adafruit is tightly connected to the work.

We use the simple name without the -device suffix for quick start nodes which are more accessible for newcomers and encapsulate the main function of a thing. Whereas a xxx-device is a dumb description which requires an action node (method) to do something useful. Used in advanced cases. It’s a pure convention but seems to work quite well.

This aspect requires more practical usage experience, more pain.

All values of uint16_t are directly representable with Number. There’s a performance penalty, but, well… we have to sacrifice something to make XOD possible.

However, uint32_t / int32_t is not directly representable with Number because values over 17M start to skip some integers. If it is important to a library, we have a problem. This case should be investigated.

They always are :slight_smile:

IMO the pulse generation should be left up to a user. He could send the trigger pulse from done of an upstream node, from pulse-on-change, or whatever he wants. The implementation of such node should be done in a way which short-circuits the evaluation to quick skip if the pulse is not set:

void evaluate(Context ctx) {
    if (!isInputDirty<input_trigger>(ctx)) {
        // The eval is provoked by a change of a functional value
        return;
    }

    // ...
    myDevice->myMethod(myArgs);
    emitValue<output_DONE>(ctx, 1);
}
1 Like