Add image capture from a Raspberry Pi camera. #46
Conversation
| """Creates a new camera wrapper instance. | ||
|
|
||
| Args: | ||
| folder_path: String of the path name of the folder where pictures |
There was a problem hiding this comment.
For this I don't think we need to say "String of the." Path name implies it's a string.
|
|
||
| def capture(self): | ||
| """Captures an image from the camera.""" | ||
| filename = self._local_clock.now().strftime('%Y-%m-%d-%H') |
There was a problem hiding this comment.
I think we should do the full timestamp to seconds-level granularity so that we can support taking many pictures per hour or minute. Also, since we're using the filename to store the metadata about the time the picture was taken, I think we should include the UTC offset so that we can parse it back later as a timezone-aware datetime.
One thing to be aware of is that : is an illegal character on some filesystems, so we don't want to use that in our filename.
Also I don't think picamera will add the extension for us, so I think we need to append '.jpg'.
| @@ -0,0 +1,27 @@ | |||
| class Camera(object): | |||
There was a problem hiding this comment.
I'd say it's more like a CameraManager or a PhotoSaver because the only thing the caller can do with it is tell it to save a picture to the filesystem.
| self._camera = camera | ||
|
|
||
| def capture(self): | ||
| """Captures an image from the camera.""" |
There was a problem hiding this comment.
Suggests "Captures an image from the camera and saves it to the destination folder." Maybe rename the function to something like save_photo to make the purpose a little clearer.
| @@ -190,3 +190,23 @@ def _poll_once(self): | |||
| if ml_pumped > 0: | |||
| self._watering_event_store.store_water_pumped( | |||
| self._local_clock.now(), ml_pumped) | |||
|
|
|||
|
|
|||
| class CameraPoller(SensorPollerBase): | |||
There was a problem hiding this comment.
Oh this is clever! It hadn't occurred to me that it would just plug into our pollers.
| def capture(self): | ||
| """Captures an image from the camera.""" | ||
| filename = self._local_clock.now().strftime('%Y-%m-%d-%H') | ||
| self._camera.capture(self._folder_path + filename, format='jpeg') |
There was a problem hiding this comment.
We always want to use os.path.join to combine file paths. There's a lot of subtle things that can go wrong if we do it by hand (especially if we want to work well across platforms). A common bug is to call the function and forget the trailing slash like C:\foo\bar instead of C:\foo\bar\ and then it puts the file at C:\foo\bar2016-01-05.jpeg.
|
If you mention issue #44 in the commit description, Github will cross reference the issue here (but we can also do it in PR comments like this). |
| @@ -0,0 +1,25 @@ | |||
| import unittest | |||
| import datetime | |||
| camera.Camera(folder_path, mock_local_clock, mock_camera).capture() | ||
| mock_camera.capture.assert_called_once_with( | ||
| 'C:\\Users\\Jeet\\2016-07-23-10', | ||
| format='jpeg') |
There was a problem hiding this comment.
Suggest using contextlib.closing here. It calls close automatically at the end of the with block, even if there's an exception.
That's what we'll want actual callers to use.
with contextlib.closing(camera.Camera(folder_path, mock_local_clock,
mock_camera)) as camera_manager:
camera_manager.capture()
mock_camera.capture.assert_called_once_with(
'C:\\Users\\Jeet\\2016-07-23-10',
format='jpeg')
mock_camera.close.assert_called()
| folder_path, mock_local_clock, mock_camera)) as manager: | ||
| manager.save_photo() | ||
| mock_camera.capture.assert_called_once_with( | ||
| 'C:/Users/Jeet/2016-07-23T10-51-09.928000+00-00.jpg') |
There was a problem hiding this comment.
I think this should be just one test, like we do the mock_camera.close.assert_called() outside the with. I feel like in this case, it's still all logically part of the same single scenario of taking a photo and releasing the camera manager.
This change is