camera config classes #73

joeyhlu · 2024-12-04T02:52:07Z

No description provided.

maxlou05

Reviewed

maxlou05 · 2024-12-04T02:55:10Z

modules/camera/__init__.py

This should be an empty file. Also, we don't import classes directly, we only import the modules then use module.Class later on in the code. Manually include the imports in each file, and only 1 module per line

maxlou05 · 2024-12-04T02:55:55Z

modules/camera/camera_config.py

+class OpenCVCameraConfig:
+    """
+    Placeholder
+    """


Add a newline at the end of the file. Also, are empty classes are not allowed in Python, you need to add pass to it otherwise it throws error.

modules/camera/camera_factory.py

maxlou05 · 2024-12-04T02:58:40Z

modules/camera/camera_factory.py

-    camera_option: CameraOption, width: int, height: int
-) -> tuple[True, base_camera.BaseCameraDevice] | tuple[False, None]:
+    camera_option: CameraOption, width: int, height: int, config: None
+) -> tuple[True, base_camera.BaseCameraDevice | None] | tuple[False, None]:


You should never return True if returning Camera device failed to instatiate, that's why it was setup like this. No need to change the output type

maxlou05 · 2024-12-04T03:03:19Z

modules/camera/camera_factory.py

 def create_camera(
-    camera_option: CameraOption, width: int, height: int
-) -> tuple[True, base_camera.BaseCameraDevice] | tuple[False, None]:
+    camera_option: CameraOption, width: int, height: int, config: None


config type should be [camera_config.PiCameraConfig | camera_config.OpenCVCameraConfig]

maxlou05 · 2024-12-04T03:14:29Z

tests/integration/test_camera_factory.py

+
+def test_opencv_camera_config() -> None:
+    config = OpenCVCameraConfig()
+    assert config is not None


Add empty line at end of file

maxlou05 · 2024-12-04T03:18:19Z

modules/camera/camera_picamera2.py

-            config = camera.create_still_configuration(
+            camera_config = camera.create_still_configuration(
                {"size": (width, height), "format": "RGB888"}
            )
-            camera.configure(config)
+            camera.configure(camera_config)


Can you change this portion to create_preview_configuration(). If you don't want to make a new name, you can also combine the whole thing together into a line

maxlou05 · 2024-12-04T03:20:26Z

modules/camera/__init__.py

+    "OpenCVCameraConfig",
+    "CameraOpenCV",
+    "CameraPiCamera2",
+]


Add newline to the bottom of the file

maxlou05 · 2024-12-04T03:21:23Z

modules/camera/camera_config.py

+        """
+        Dictionary containing camera controls.
+        """
+        controls: dict[str, int | float | None]  = {}


It doesn't need to have a None type, as it just won't exist in the dictionary

modules/camera/camera_config.py

maxlou05

Reviewed

maxlou05 · 2024-12-04T16:20:33Z

modules/camera/camera_config.py

+            controls["Contrast"] = self.contrast
+        if self.lens_position is not None:
+            controls["LensPosition"] = self.lens_position
+            controls["AfMode"] = controls.AfModeEnum.Manual


I think you need to import the library at the top of this file:

try: from libcamera import controls # This is a pre-installed library on the Rpi5 except: pass

But, this means you probably should rename your dictionary to something else like camera_controls or settings

maxlou05 · 2024-12-04T16:23:44Z

tests/integration/test_camera_factory.py

I think you don't need this file, just modify test_camera_picamera2.py, test_camera_opencv.py and test_camera_qr_exmaple.py (all in the tests/integration folder) to use the camera configs.

maxlou05 · 2024-12-04T16:24:41Z

tests/integration/test_camera_picamera2.py


 from modules.camera import camera_factory

+from modules.camera.camera_config import PiCameraConfig


No space above this line, and also should be from modules.camera import camera_config since we import modules

maxlou05 · 2024-12-04T16:27:56Z

modules/camera/camera_factory.py

    """
    match camera_option:
        case CameraOption.OPENCV:
            return camera_opencv.CameraOpenCV.create(width, height)


In camera_opencv.py just make create take in a config Object as well, and then write a comment #TODO: apply camera configs to camera here to indicate it will be done sometime in the future

maxlou05

Reviewed

maxlou05 · 2024-12-04T21:11:36Z

modules/camera/base_camera.py

    @abc.abstractmethod
    def create(
-        cls, width: int, height: int
+        cls, width: int, height: int, config: object


Type of config should be camera_config.PiCameraConfig | camera_config.OpenCVCameraConfig

maxlou05 · 2024-12-04T21:12:16Z

modules/camera/camera_config.py

+try:
+    from libcamera import controls  # This is a pre-installed library on the Rpi5
+except ImportError:
+    pass  # pylint: disable=bare-except


Are you sure you still need the bare-except disable? If not, then remove it

maxlou05 · 2024-12-04T21:13:48Z

modules/camera/camera_opencv.py

 import numpy as np

 from . import base_camera
+from .camera_config import OpenCVCameraConfig


Just from . import camera_config

maxlou05 · 2024-12-04T21:14:25Z

modules/camera/camera_opencv.py

+    def create(
+        cls, width: int, height: int, config: OpenCVCameraConfig
+    ) -> "tuple[True, CameraOpenCV] | tuple[False, None]":
+        # TODO: apply camera configs to camera here


Can you move this comment to line 32, where you wrote the placeholder?

maxlou05 · 2024-12-04T21:15:14Z

modules/camera/camera_opencv.py

    @classmethod
-    def create(cls, width: int, height: int) -> "tuple[True, CameraOpenCV] | tuple[False, None]":
+    def create(
+        cls, width: int, height: int, config: OpenCVCameraConfig


Make config an optional argument defaulted to None

maxlou05 · 2024-12-04T21:18:45Z

tests/integration/test_camera_opencv.py


-from modules.camera import camera_factory
-
+from modules.camera import camera_factory, camera_config


Please separate into 2 lines, and also keep 2 spaces between the imports and global constants.

from modules.camera import camera_factory from modules.camera import camera_config IMAGE_LOG_PREFIX = ...

maxlou05 · 2024-12-04T21:20:31Z

tests/integration/test_camera_opencv.py

+    try:
+        assert config is not None
+    except AssertionError as e:
+        print(f"Configuration test failed: {e}")
+        return -1


It's fine to just keep the assert line. We don't need to catch exception here, as this is an integration test.

maxlou05 · 2024-12-04T21:21:12Z

tests/integration/test_camera_picamera2.py

+    try:
+        assert config.exposure_time == 250
+        assert config.contrast == 1.0
+        assert config.analogue_gain == 64.0
+        assert config.lens_position is None
+    except AssertionError as e:
+        print(f"Configuration test failed: {e}")
+        return -1


Same here, just keep the assert lines, no need to catch the exception since this is a test

maxlou05 · 2024-12-04T21:22:22Z

tests/integration/test_camera_picamera2.py


-from modules.camera import camera_factory
-
+from modules.camera import camera_factory, camera_config


Same here: separate into 2 lines and make sure 2 spaces between imports and global constants

maxlou05 · 2024-12-04T21:22:31Z

tests/integration/test_camera_qr_example.py

 import cv2

-from modules.camera import camera_factory
+from modules.camera import camera_factory, camera_config


Separate into 2 lines

maxlou05

Reviewed

maxlou05 · 2024-12-04T22:23:47Z

modules/camera/camera_picamera2.py

+from . import camera_configurations
+


Can you move these to line 15 (so that our libraries are grouped together, and 2 lines before constants)

maxlou05

Approved!

maxlou05

Approved

camera config classes

54b57d2

maxlou05 reviewed Dec 4, 2024

View reviewed changes

joeyhlu added 2 commits December 3, 2024 23:29

camera config classes updated

204f5a0

updated camera_comfig

019215b

maxlou05 reviewed Dec 4, 2024

View reviewed changes

joeyhlu added 3 commits December 4, 2024 15:30

updated camera config

8378e29

updated config

6df9452

updated camera config

53eebb7

maxlou05 reviewed Dec 4, 2024

View reviewed changes

joeyhlu added 5 commits December 4, 2024 16:53

styling errors fix

00d4fce

styling fixes

6752930

fixed errors

418c91e

styling issues

fe3d915

fixed styling

3358be3

maxlou05 reviewed Dec 4, 2024

View reviewed changes

joeyhlu added 2 commits December 4, 2024 17:36

fixed styling:

2e8c668

fixed styling

b0f5f89

maxlou05 reviewed Dec 4, 2024

View reviewed changes

maxlou05 approved these changes Dec 4, 2024

View reviewed changes

joeyhlu merged commit 1c31b5c into main Dec 4, 2024
1 check passed

joeyhlu deleted the fixed-camera-config-classes branch December 4, 2024 22:48


		from modules.camera import camera_factory

		from modules.camera.camera_config import PiCameraConfig

camera config classes #73

camera config classes #73

Uh oh!

Conversation

joeyhlu commented Dec 4, 2024

Uh oh!

maxlou05 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maxlou05 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxlou05 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxlou05 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxlou05 left a comment

Choose a reason for hiding this comment

Uh oh!

maxlou05 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!