hzeller / rpi-rgb-led-matrix

Controlling up to three chains of 64x64, 32x32, 16x32 or similar RGB LED displays using Raspberry Pi GPIO
GNU General Public License v2.0
3.67k stars 1.17k forks source link

Fill() not respecting RGB sequence #679

Closed mikepparks closed 6 years ago

mikepparks commented 6 years ago

I noticed when testing my 64x64 panel using the newer Adafruit HAT that it seems the framebuffer Fill() method is not respecting the RGB sequence. I switched to "RBG" because it seemed that SetPixel wasn't working as expected in "RGB" mode, but noticed that doing this did not affect the output of Fill for the B & G colors at all. Is this happening to anyone else, or is this the expected behavior? I'm currently poking around for a fix, but would love to make sure that I'm not chasing windmills first in case there's something I missed.

hzeller commented 6 years ago

So it works for SetPixel(), but not Fill() ?

mikepparks commented 6 years ago

Correct.

hzeller commented 6 years ago

Alright, now it should work :)

flavio-fernandes commented 6 years ago

@hzeller sorry to bother you, but on my setup, the commit ba3226b873dcef93f8ff64a1836023d9d4b6a844 really broke python binding.

To reproduce, simply use:

options.led_rgb_sequence = "RBG"

Let me know if you need more details, but it seems that seq is pointing to the wrong string, or to a string that has been freed up.

#  Framebuffer::GetGpioFromLedSequence  is very unhappy
$ sudo ./screen.py
LED sequence '
�t' does not contain any 'R'.
Aborted
hzeller commented 6 years ago

Interestng, did it even work before then @flavio-fernandes ? Because the lifetime assumptions about this string didn't change at all.

You haven't showed your code. Are you by any chance using cppinc.Options class in Python ? That won't work as it does not copy the string locally, while RGBMatrixOptions does.

flavio-fernandes commented 6 years ago

heh, sorry for the confusion. I will make a PR to give you an example on how it breaks. I have actually made no changes at all to this repo. Simply using Adafruit's install script and hacking the commit it pulls from your repo 1. I only see this issue if I use commit ba3226b.

hzeller commented 6 years ago

For a baseline test, I compiled the library and did a fresh compilation and install of the python binding and the following works on my Pi with Python 2.7

  sudo bindings/python/samples/rotating-block-generator.py --led-rgb-sequence=BRG

What is different in your set-up ?

flavio-fernandes commented 6 years ago

Sorry for delay, @hzeller .

I will come up with a well defined set of steps to see if I can consistently reproduce it. Likely this weekended.

cd ~/bedclock.git
export PYTHONPATH=~/bedclock.git
if [ ! -e ~/bedclock.git/env ]; then time ~/bedclock.git/bedclock/bin/create-env.sh ; fi
source ~/bedclock.git/env/bin/activate
cd ~/bedclock.git/bedclock
sudo ./screen.py

Please know that this is id a WIP, but was working fine before I re-ran rgb-matrix.sh to pick up ba3226b873dcef93f8ff64a1836023d9d4b6a844

Best,

-- flaviof

hzeller commented 6 years ago

Did you compare if the problem started to happen between 0d89e01d562d4a303ab4107e0be9088370ff9c9c (previous last) and ba3226b873dcef93f8ff64a1836023d9d4b6a844 (latest) or just between the last submit the Adafruit script has 58830f7bb5dfb47fc24f1fd26cd7c4e3a20f13f7 (somewhere February) and latest ? Maybe there is something else that broke in-between.

So the Python binding internall attempts to actually create a copy of the string, so that it is not garbage collected before passing on to the library, here are the relevant parts:

https://github.com/hzeller/rpi-rgb-led-matrix/blob/master/bindings/python/rgbmatrix/core.pxd#L18 https://github.com/hzeller/rpi-rgb-led-matrix/blob/master/bindings/python/rgbmatrix/core.pyx#L159

Can you trace on your system how this is behaving ? I don't know circuit python, is there anything different there ? Maybe the trick with the local copy stopped working ? I don't know enough about Python internals to be of much help here, but since you're using it, you might be able to find the issue quickly.

hzeller commented 6 years ago

Ok, so looking at your code https://github.com/flavio-fernandes/bedclock/blob/master/bedclock/screen.py#L70 you create the options struct on the stack, so the string content would actually be destroyed; then if you later create a frame, it will choke there.

Fixed. With the latest commit, it should now work.

flavio-fernandes commented 6 years ago

ah... works great. Made a PR for Adafruit, so they pick up all the enhancements you added since 58830f7bb5dfb47fc24f1fd26cd7c4e3a20f13f7. Thank you @hzeller !