search

tinue / apa102-pi

Pure Python library to drive APA102 LED stripes; Use with Raspberry Pi.
GNU General Public License v2.0
202 stars 72 forks source link

Small bugs in the code #2

Closed pcjacobse closed 8 years ago

pcjacobse commented 8 years ago

Hi,

I'm starting a simple Raspberry + APA102 project myself. And found your code, it's a really nice starting point for me, tnx for that. I played a bit with it last night, and found 2 small bugs in your code:

  1. In the clearstrip method (https://github.com/tinue/APA102_Pi/blob/master/apa102.py#L102) you forgot to send the clock startframe self.clockStartFrame(). The leds didn't clear for me without this.
  2. In the paint loop (https://github.com/tinue/APA102_Pi/blob/master/colorcycletemplate.py#L65) you increment the counter before checking, so you should do a check only on greater than. not equal: if (currentStep > self.numLEDs): I guess this isn't really noticable on a led strip. However I'm working with single leds (making a ledmatrix myself) and when I tested the code with 1 led the update method didn't work.
  3. The same issue is with the check on the cycles (https://github.com/tinue/APA102_Pi/blob/master/colorcycletemplate.py#L69): if (currentCycle > self.numCycles): break, however I didn't check it.

If you'd like I can create a pull request tonight.

pcjacobse commented 8 years ago

I just checked out sebastian's fork. You should also merge in his commits, it looks like they fix a couple of other issues :)

tinue commented 8 years ago

Thanks for your comments! This explains why the LEDs sometimes did not clear up when I hit Ctrl-C ;-) Usually they did, but this obviously depends on when in the entire cycle Ctrl-C is being hit.

The new samples are not quite debugged yet, I suspect that there will be some more errors present. For example I don't check at all for overflow, so for example stuff like "currentCycle*numLEDs+currentStep" is bound to break after a few hours, and with a lot of LEDs.

As for the fork: I don't understand the change. What error does it prevent? The clearStrip method is only to clear the strip e.g. before exiting the application (the comment explains this). Why would one want to release and reallocate the memory for the strip buffer?

-> Anyway, I'll have a more detailed look at your comments on the weekend.

pcjacobse commented 8 years ago

The clearing of the memory help if you only want to set specific leds after clearing the strip. I guess its not really usefullI for exiting the application, but when you built out some of the samples and want to clear the strip midrun, and after that just set a couple of pixels, you dont have to set all other leds to black manually.

And the other commit he did was swap the green and blue byte, since those are in the wrong order in your code. For example your colorwheel starts on blue instead of green.

tinue commented 8 years ago

Coloring is a different issue, that I haven't tackled yet: There are different strips out on the market. Some are rgb, others are bgr, and there are possibly even more. It will get tricky if strips of different makes are coupled together. For example the strandtest would suddenly change color on the boundary of different types. It looks like I have a different strip than you have, because on mine the colors are correct.

tinue commented 8 years ago

I think I fixed the reported issues, so I am closing it. Feel free to reopen if you disagree.