Added NeoPixel FeatherWing

[makermelissa]

Ok, I finished the NeoPixel FeatherWing, which is based on my DotStar FeatherWing work.

[kattni]

This looks great! Thank you for extending the library the way you did, I really like that you didn't duplicate a lot of code.

I have a suggestion that affects a number of lines, but I won't comment on each one as there are many.

[kattni]

I would consider naming this matrix instead of display as we have a lot of display libraries within which we use that term. There are some FeatherWings with displays (or maybe only one?) and it might be a little confusing.

[caternuson]

Interesting inheritance. How about refactoring everything into a third generic class that both DotStar and NeoPixel could derive from?

[makermelissa]

Thanks @kattni. Matrix sounds good. I was just trying to use a generic term. @caternuson I started going down that path but there realized there were less disables required for pylint this way. Either way would work though if you'd prefer I change it to that.

[makermelissa]

Added @kattni's suggested change

[caternuson]

@makermelissa What's an example of one of the pylint disables?

[makermelissa]

I can't remember the exact one, but I put all of the functionality in the base class and because it self._display (now matrix) hadn't been assigned until the extended classes were initialized, Pylint was complaining that the methods/properties of display that I was calling in the base class didn't exist.

[caternuson]

Did you have a stubbed out placeholder in the base class for self._matrix? I'm thinking just set it to None in the base class's __int__() and then set it to a DotStar/NeoPixel instance in the derived classes.

Something like pixels in BaseClass here:

class DotStar:
    """DotStar class"""
    def __init__(self, clock, data):
        self.clock = clock
        self.data = data

    def show(self):
        """Show stuff."""
        print("I'm a DotStar with clock = {} and data = {}".format(self.clock, self.data))

class NeoPixel:
    """NeoPixel class"""
    def __init__(self, data):
        self.data = data

    def show(self):
        """Show stuff."""
        print("I'm a NeoPixel with data = {}".format(self.data))

class BaseThing:
    """Base class."""
    def __init__(self):
        self.pixels = None

    def show(self):
        """Show stuff."""
        self.pixels.show()

class DotStarThing(BaseThing):
    """DotStar class."""
    def __init__(self, clock, data):
        super().__init__()
        self.pixels = DotStar(clock, data)

class NeoPixelThing(BaseThing):
    """NeoPixel class."""
    def __init__(self, data):
        super().__init__()
        self.pixels = NeoPixel(data)

Test usage:

Adafruit CircuitPython 3.1.2 on 2019-01-07; Adafruit ItsyBitsy M4 Express with samd51g19
>>> import class_test
>>> d = class_test.DotStarThing(1,2)
>>> n = class_test.NeoPixelThing(8)
>>> d.show()
I'm a DotStar with clock = 1 and data = 2
>>> n.show()
I'm a NeoPixel with data = 8
>>> 

[makermelissa]

I tried setting it to None and when I tried to set it in the child class I got pylint errors such as

E:117,25: Value 'self._display' is unsubscriptable (unsubscriptable-object)
E:119,16: 'self._display' does not support item assignment (unsupported-assignment-operation)

[caternuson]

Can you put the full code up in a branch in your repo and link it here.

[makermelissa]

You can see it in one of my above commits. Link is b862de5

[caternuson]

Hmmm. Quick scan of that, yah, that's what I'm thinking. Seems OK. Wonder if you're hitting this?
pylint-dev/pylint#1498

What if you ignore pylint and just try running it? Was it functional?

[makermelissa]

Yeah, it was functional. That's why I mentioned pylint disables.

[makermelissa]

And yes, that sounds like the issue I ran into.

[caternuson]

If you'd rather have gone with that class arrangement, I'd say it's OK to go ahead and add the disables. Seems like you only went with the current arrangement because of what appears to be a still open pylint bug.

You can add the disable globally, so you don't have to pepper it all over the place.

[makermelissa]

That's correct. I'll go ahead and make the changes and get them pushed.

[makermelissa]

Ok, it is now using the base class, has @kattni's suggestion, passes pylint, and functions perfectly.

[caternuson]

I'd suggest keeping all the methods public in the PixelMatrixFeatherWing class (no leading _). Then your two subclasses would only need to override __init__ to plumb in an actual _matrix object.

Also, move PixelMatrixFeatherWing to a separate file and then import it in the other two.

[makermelissa]

Yeah, the main reason I did it with private methods was to be able to provide FeatherWing specific examples for each function for purposes of RTD. However, I can make is so the docstrings reflect one wing or the other.

[makermelissa]

I moved PixelMatrix to a separate file. However, I'm waiting on a response about suggestions for code examples and RTD. In other words, if I remove all the public function calls from NeoPixel and DotStar then it will show RTD generated comments from the docstring of PixelMatrix on both files. (i.e. If I use the DotStar example it will show up under NeoPixel wing or vice versa) I would think most people probably won't have both wings. Plus PixelMatrix was never intended to be used on it's own since it's a base class. I could remove the example, but I feel like it would be more helpful to the end user to have those.

[tannewt]

RTD comments on both sounds good to me. Why would you need to remove the example?

[makermelissa]

Unless there's a way of doing it that I'm not aware of, I believe RTD will split off whatever example is in the base class to the extended classes.

[tannewt]

I'd move the full function examples into external example files. Then, in the function just include the related code rather than the full example and refer to the full example.

[makermelissa]

Ok @tannewt, thanks. I'll go ahead and do that. I think including the examples in the comments seems to only work well if classes don't share code. I'm doing something similar with the Alphanum/7-segment displays too so I'll apply the same approach.

[tannewt]

Great! Happy to help unblock you. Thank you for all of the awesome changes.

[makermelissa]

I went ahead and removed all of the overrides except shift_up and shift_down in the neopixel featherwing library since they are laid out vertically in reverse of each other and shift_up on the neopixel being the same direction as shift_down on the dotstar.

[caternuson]

Nice! Looks good. I like it from a code layout / inheritance stand point.

You've tested this with hardware on both Wings?

[makermelissa]

Yes @caternuson, I tested all examples on both wings.

[caternuson]

@kattni I approved for CP Librarians. You good with changes too?

[kattni]

Looks good! Thanks again!