New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GPIO.cleanup() in camera.close() #35

Closed
martinohanlon opened this Issue Dec 24, 2013 · 10 comments

Comments

Projects
None yet
2 participants
@martinohanlon

martinohanlon commented Dec 24, 2013

Hi,

I'm getting a problem as my calling program uses the GPIO, but Camera.close() calls GPIO.cleanup() which resets the GPIO state and make my program crash.

As an example, I want to use a button to start the camera, the code for my button exists in my calling program, I run Camera and stop it when I press the button, the next time I want to use the GPIO I cant because camera.close() has called cleanup(), resetting GPIO.

A possible solution could be to pass an optional useLed (which could default to True) on init of the PiCamera class? I have made the change in my fork of picamera. https://github.com/martinohanlon/picamera/blob/master/picamera/camera.py

To replicate the issue, you can run the following:

import picamera
import RPi.GPIO as GPIO

set gpio mode

GPIO.setmode(GPIO.BCM)

setup gpio pin as output

GPIO.setup(17, GPIO.OUT)

turn gpio on

GPIO.output(17, True)

with picamera.PiCamera() as camera:
camera.resolution = (800, 600)
camera.framerate = 25
camera.start_recording("my_video.h264")

camera.wait_recording(1)

camera.stop_recording()
camera.close()

turn gpio off - error occures here!

GPIO.output(17, False)

@ghost ghost assigned waveform80 Dec 24, 2013

@waveform80

This comment has been minimized.

Owner

waveform80 commented Dec 24, 2013

Ahhh ... a global state problem. Well, that certainly needs fixing before 1.0. The slight issue I see with your suggestion of a useLed parameter to the camera's init is what if someone wants to use the camera, control the LED, and still use GPIO for a button as you're doing here? Sadly I'm not that experienced with GPIO stuff yet (although I have a suspicion there's certain pi-related gifts that might just be waiting under the christmas tree tomorrow - so that might change in the near future!).

Anyway, I suspect the first thing to do will be to make the GPIO setup "lazy" - i.e. instead of doing it in __init__ we should do it in _set_led the first time it's called and set a flag to remember that someone's used the LED and we should call GPIO.cleanup in close. That'll mean the PiCamera class never touches GPIO if it doesn't need to.

That takes care of people not using the LED property and using GPIO (without any code changes on their part, which is always nice), and doesn't affect people using LED and not using GPIO, but still leaves the question of people who want to use LED and use GPIO. For them I suspect the only option is indeed to add a parameter to __init__ - but one that implies "don't cleanup GPIO on close, even if I've used the LED, I'll take responsibility for doing that myself". Something like no_gpio_cleanup (which should default to False).

Anyway, I'll set this as milestone 1.0 and see if I can get it sorted out sometime in the holidays!

@waveform80

This comment has been minimized.

Owner

waveform80 commented Dec 24, 2013

Hehe - no need to close this yet! Or not until I've finished implementing it anyway :-)

@waveform80 waveform80 reopened this Dec 24, 2013

@waveform80

This comment has been minimized.

Owner

waveform80 commented Dec 24, 2013

Hmm, after a bit more reading it appears that the only thing GPIO.cleanup is set everything back to an input (for safety). However, as we're only messing around with GPIO5 which isn't exposed on the board there's no safety reason to reset it to an input. If we simply remove the cleanup call in close that'll sort things out for all GPIO users, regardless of whether they use the LED or not.

That said, it's still worth modifying the LED initialization to be lazy. This will allow GPIO users to use the library in BOARD mode and still use the camera (as long as they don't touch the led property).

@martinohanlon

This comment has been minimized.

martinohanlon commented Dec 24, 2013

I think its worth changing the led initialisation. The only problem with no calling cleanup is that, RPi.GPIO gives a warning the next time you set the mode if it wasn't previously 'cleanedup', although picamera does set warnings to false so this would suppress it.

Just an idea:

What about giving the calling program the accountability of calling the gpio initialisation and cleanup code? That way you could use the concept of use_led (or similar) and let the user set their own gpio pin number and gpio numbering mode (BCM/BOARD) as well.

e.g.
LEDGPIOPIN = 4
GPIO.setmode(BOARD)
camera = picamera.Camera()
camera.initLed(4)
camera.startrecording()
....
camera.stoprecording()
GPIO.cleanup()

@waveform80

This comment has been minimized.

Owner

waveform80 commented Dec 25, 2013

That's interesting - is the camera LED controllable from BOARD mode? (I was under the impression it was only controllable when GPIO was in BCM mode, with pin 5). I'll have a play and see if I can get it working - if I can, then picamera probably shouldn't be calling setmode at all - just detect what mode the user has set (if any) and go with that.

@martinohanlon

This comment has been minimized.

martinohanlon commented Dec 26, 2013

I was only using BOARD as an example, sorry if I caused confusion, I dont know if the camera LED is controllable from BOARD mode.

I was just trying to get across though that if you give the calling program the accountability for setting the mode and pin you dont need to worry about it in picamera.

@waveform80

This comment has been minimized.

Owner

waveform80 commented Jan 1, 2014

Hi Martin - sorry for the long delay in replying - we've been dashing round the UK doing our xmas tour of our families! Anyway, to the business of the ticket!

I've played a bit with RPi.GPIO and it does seem as though the camera's LED is only controllable when the library is in BCM mode; it's impossible from BOARD mode (though I suspect it could be made to work if the library could be tweaked to add a fake pin in BOARD mode for BCM pin 5). If it worked in both modes, I would absolutely agree with your suggestion that we should leave GPIO initialization and cleanup outside picamera entirely and just use whatever mode the user configures.

Worse still, it appears there's no way to query the RPi.GPIO library for the mode it's set to (at least I haven't found one?). If we could query what mode the RPi.GPIO library was in I would also agree we should leave GPIO init to the user and throw an exception if they attempt to control the camera LED while the RPi.GPIO library is in BOARD mode.

Unfortunately, neither is true, and that leaves us with a rather nasty edge case: assume that we leave GPIO init to the user and they set the RPi.GPIO library to BOARD mode. Now the user imports the picamera library and attempts to set the camera's LED. The picamera library configures pin 5 as an output and proceeds to fiddle with its value. In BOARD mode, pin 5 is an actual GPIO pin so it's possible (assuming certain wiring setups) that as a consequence, the user fries their Pi (or some other component) simply by virtue of setting the camera's LED - ouch!

So, long story short: in principal I think you're absolutely right and init shouldn't be handled by picamera. However, while that edge case exists without a workaround I'm reluctant to change to such a setup. In the meantime, I'll work on a patch for RPi.GPIO to allow reading the library's mode and send them a pull request. If it gets accepted then I'll re-open this and move init out of picamera.

On the subject of the warning that gets thrown if RPi.GPIO doesn't get cleaned up: that's certainly an issue on picamera's side. I'd be tempted to fix it with the original suggestion of a no_gpio_cleanup flag to picamera's __init__. However, if in future we moved GPIO init out of picamera there'd be a backwards incompatible change necessary to remove that flag. Given that the only effect of the lack of a cleanup call at the moment is a warning from the RPi.GPIO library (i.e. nothing actually breaks, it's just annoying, and there's a simple workaround of the user calling cleanup themselves) I'm tempted to leave things as they are for now, as it'll ease any future change.

Anyway, hopefully that explains my thinking on the current implementation. It's not perfect, but I think it's a reasonable compromise until I can figure out something better!

@martinohanlon

This comment has been minimized.

martinohanlon commented Jan 2, 2014

Ok I get all that. It still cause me a couple of problems because the gpio mode is set at init of picamera, so if I am using BOARD, it gets changed to BCM.

Going right back to the start how do you feel about implementing the mod I have done which, because I want full control over the GPIO from my calling program is to create an optional property on init called use_led. use_led is then stored as a property and used in _init_led to set GPIO to None if its False.

def __init__(self, use_led=True):
        global _CAMERA
        if _CAMERA:
            raise PiCameraRuntimeError(
                "Only one PiCamera object can be in existence at a time")
        _CAMERA = self
        self._camera = None
        self._camera_config = None
        self._preview = None
        self._preview_connection = None
        self._null_sink = None
        self._video_encoder = None
        self._splitter = None
        self._splitter_connection = None
        self._exif_tags = {
            'IFD0.Model': 'RP_OV5647',
            'IFD0.Make': 'RaspberryPi',
            }
        # persist use_led property
        self.use_led = use_led
        self._init_led()
        try:
            self._init_camera()
            self._init_defaults()
            self._init_preview()
            self._init_splitter()
        except:
            self.close()
            raise
def _init_led(self):
        global GPIO
        if GPIO:
            #if use_led = True, set it up, otherwise set GPIO to None
            if self.use_led:
                try:
                    GPIO.setmode(GPIO.BCM)
                    GPIO.setwarnings(False)
                    GPIO.setup(5, GPIO.OUT, initial=GPIO.LOW)
                except RuntimeError:
                    # We're probably not running as root. In this case, cleanup and
                    # forget the GPIO reference so we don't try anything further
                    GPIO.cleanup()
                    GPIO = None
            else:
                GPIO = None

Using this approach the user of picamera has the choice about whether to let picamera manage the cam led and gpio or not.

I think this is a struggle due to the balance of making picamera functionally rich but at the same time giving flexibility to the user.

Thanks a lot for your help.

@waveform80

This comment has been minimized.

Owner

waveform80 commented Jan 2, 2014

Hi Martin,

Actually the commit last used to close the ticket, 28f5af7, should fix that issue for you as well; _init_led is no longer called unconditionally on startup - instead it's called on the first attempt to set the led attribute, so provided you don't attempt to set led your BOARD-based GPIO code should "just work" (at least, once I get release 1.0 out of the door, which should be sometime this month!)

Cheers,

Dave.

@martinohanlon

This comment has been minimized.

martinohanlon commented Jan 2, 2014

I see, makes sense now. Looking forward to 1.0!

waveform80 added a commit that referenced this issue Jan 6, 2014

Fix mock tests for led
After #35 lazy init is used for the GPIO library so calls should be
checked after first call to ``_set_led`` instead of immediately after
camera init
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment