GPIO.cleanup() in camera.close() about picamera

[waveform80]

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]

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

[waveform80]

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]

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]

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]

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]

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]

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]

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]

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