Skip to content
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

Move camera configurations close to camera classes #74

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

Xierumeng
Copy link
Contributor

@Xierumeng Xierumeng commented Dec 8, 2024

Improve locality of code (LOC) by moving the configuration classes close to the camera classes. The tradeoff is that users are required to import all camera classes for configurations. A possible mitigation is to generate the configuration from a common data structure (e.g. dictionary from YAML).

@Xierumeng
Copy link
Contributor Author

Xierumeng commented Dec 8, 2024

Before:

configurations
 ↑    ↑    ↑
 |    |  camera0, camera1, ...
 |    |    ↑
 |  factory
 |    ↑
user -+
import camera_configurations
import camera_factory
...
if camera_selection == 0:
    config = camera_configurations.ConfigCamera0()
elif camera_selection == 1:
    config = camera_configurations.ConfigCamera1()
elif ...
...
camera_device = camera_factory(..., config)

After:

camera0, camera1
 ↑    ↑
 |  factory
 |    ↑
user -+
import camera0
import camera1
...
import camera_factory
...
if camera_selection == 0:
    config = camera0.ConfigCamera0()
elif camera_selection == 1:
    config = camera1.ConfigCamera1()
elif ...
...
camera_device = camera_factory(..., config)

@Xierumeng Xierumeng force-pushed the camera-configuration-refactor branch from 9f88f1a to 43542cb Compare December 8, 2024 04:07
@Xierumeng Xierumeng changed the title Move configurations close to camera classes Move camera configurations close to camera classes Dec 8, 2024
Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed

timeout: Getting image timeout in seconds.

exposure_time: Microseconds.
analogue_gain:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A part of ISO, analogueGain * DigitialGain * 100 = ISO. From 0 to suggested max is 16, but can go up to 64.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the 2nd part. The acceptable range of values is 0 to 64, but the suggested range to use is 0 to 16 ? Why is the default argument 64 then?


if self.maybe_lens_position is not None:
camera_controls["LensPosition"] = self.maybe_lens_position
camera_controls["AfMode"] = picamera2.controls.AfModeEnum.Manual
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs an import: from libcamera import controls, then it's just controls.AfModeEnum...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

camera_controls["AfMode"] = picamera2.controls.AfModeEnum.Manual
else:
camera_controls["LensPosition"] = 0.0
camera_controls["AfMode"] = picamera2.controls.AfModeEnum.Auto
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

exposure_time: Microseconds.
analogue_gain:
contrast: 0.0 to 32.0 . 0.0 is no contrast, 1.0 is normal contrast, higher is more contrast.
lens_position: Position of the lens is dioptres (reciprocal of metres: 1/m ).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(0 means infinity)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Xierumeng Xierumeng requested a review from maxlou05 December 13, 2024 01:10
Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@Xierumeng Xierumeng merged commit 9b10a33 into main Dec 13, 2024
1 check passed
@Xierumeng Xierumeng deleted the camera-configuration-refactor branch December 13, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants