Add officially supported camera modules #1213

vivien · 2023-08-15T20:22:40Z

This patchset adds support for the camera modules listed in the official documentation and simplify the configuration with a single RASPBERRYPI_CAMERA variable (and predefined aliases) to describe if and which camera model is used.

At the same time, add an empty config.txt template file to the rpi-config package, because relying on a commented out config file from a user github repository is error prone and only a few keys are added to the configuration anyway. Prefer to point the user to the official documentation instead.

vivien · 2023-08-21T13:14:04Z

This PR also fixes #1177 and #1197.

conf/machine/include/rpi-base.inc

recipes-bsp/bootfiles/rpi-config_git.bb

agherzan · 2023-08-24T14:11:47Z

recipes-bsp/bootfiles/rpi-config.bb

@@ -2,18 +2,16 @@ DESCRIPTION = "Commented config.txt file for the Raspberry Pi. \
               The Raspberry Pi config.txt file is read by the GPU before \
               the ARM core is initialised. It can be used to set various \
               system configuration parameters."
+HOMEPAGE = "https://www.raspberrypi.com/documentation/computers/config_txt.html"


I'm not sure what to say about this commit. I feel like users are used to the format and will end up with a pretty confusing change. @kraj what do you think?

I really don't think that a commented out file was adding any value to the user. On the contrary, this was just cluttered and error prone. The only useful addition to the config file would be to echo the homepage URL as the first line so that the user knows where to look for documentation if they want to tweak the config, not scrolling a subjectively commented out file.

I see both sides - I'm unsure how to proceed.

@kraj your thoughts on this?

config.txt has detailed comments for an option, in-place sed keeps that intact. Non yocto users are familiar with this as well, since thats the first place people go to debug weird issues. So keeping the format has advantages. Sed is as reliable as echo, both are shell commands. They can fail in same ways. If sed can go wrong so can echo emit an older option and render system problems. So I am not buying echo is better than sed argument :)

I want to clarify that there isn't an "original" config.txt file that we are overwriting with this PR. The initial recipe fetched an unofficial file from GitHub.

Different distributions come with their own config.txt files. For instance, RPI-Distro (formerly Raspbian) ships this one, while Arch Linux ARM ships this version. The choice of config.txt is determined by the distribution, not the recipe.

In fact, a minimal image can successfully boot with either no config.txt file or with the following content:

# Empty file # Some settings may impact device functionality. Refer to the official documentation below for details. # https://www.raspberrypi.com/documentation/computers/config_txt.html

As a BSP layer, our goal should be to provide a clean default configuration with minimal adjustments. Distributions have the flexibility to further customize the configuration using conditional filters, additional files like autoboot.txt, or any configuration fragments enabled with the include directive, as outlined in the documentation.

Following best practices, it would be advisable to list rpi-config in MACHINE_EXTRA_RDEPENDS to prevent core-image-minimal from installing the file by default, while allowing other core images to include it. In the meantime, I kindly request that @kraj and @agherzan carefully consider this comment and refrain from advocating for the use of an unofficial text file on the internet, as it may not align with established best practices.

I would recommend this being split in another PR. It does not directly support the camera modules and we can have more public visibility on this in this way.

I would personally prefer a minimal config.txt that isn't filled with every option imaginable.

@vivien I started to lean into that, too - with the reoccurrence of the config.txt size issues in RPI5. @kraj I think we should start considering this as a cleaner solution forward.

yes documentation can be moved out of config file and perhaps not a bad idea, one problem I do see though is it may be out of sync with whats installed on the system and that could be confusing, so there is value in capturing the comments but I think we can make that compromize. We hardly edit this on device.

recipes-bsp/bootfiles/rpi-config.bb

Sort kernel device tree overlays alphabetically. Signed-off-by: Vivien Didelot <[email protected]>

Add the device tree overlays for the supported cameras as listed on https://www.raspberrypi.com/documentation/computers/camera_software.html#if-you-do-need-to-alter-the-configuration Signed-off-by: Vivien Didelot <[email protected]>

The rpi-config package uses a mixture of sed -i $CONFIG and echo >> $CONFIG commands in order to customize the config.txt file. Because relying on commented out option is error prone, replace all sed -i occurrences with the echo alternative. Signed-off-by: Vivien Didelot <[email protected]>

Relying on a commented out config file fetched from a user Github repository is likely error prone and misleading. The best practice is to direct the user to the official documentation describing the complete list of supported configuration keys. Only a few keys are added to the config.txt file anyway, so start from an empty file that the user may override if necessary. Move the config.txt tweak code into do_compile for clarity and to ensure that the 80 character wide check is done once the file is modified, not in between appended modifications. Signed-off-by: Vivien Didelot <[email protected]>

recipes-bsp/bootfiles/rpi-config.bb

agherzan · 2023-09-19T12:04:02Z

recipes-bsp/bootfiles/rpi-config.bb

+RASPBERRYPI_CAMERA ??= "auto"
+RASPBERRYPI_CAMERA_V1 = "ov5647"
+RASPBERRYPI_CAMERA_V2 ?= "imx219"
+RASPBERRYPI_CAMERA_HQ = "imx477"


What uses some of these variables?

These variables describe the common values and are optionally used by the user to assign RASPBERRYPI_CAMERA if they wish not to explicit the actual model name.

Can you give me an example?

RASPBERRYPI_CAMERA = "${RASPBERRYPI_CAMERA_V2}", it's optional, but convenient in order to avoid case sensitive errors or other typos. I'll update the doc to reflect that as well. One may even append overlay parameters if they wish to, e.g. RASPBERRYPI_CAMERA = "imx219,rotation=0".

I propose keeping this simple with only RASPBERRYPI_CAMERA and giving documentation examples of its values. Having indirect definitions and keeping them updated with new variables is a situation that might cause more confusion.

Instead of using many inconsistent variable flags like VIDEO_CAMERA, RASPBERRYPI_CAMERA_V2, RASPBERRYPI_CAMERA_V3 or RASPBERRYPI_HD_CAMERA, use a single RASPBERRYPI_CAMERA variable, defaulting to "auto" for automatic camera detection, which can be set to a specific camera module name or alias as described in https://www.raspberrypi.com/documentation/computers/camera_software.html#if-you-do-need-to-alter-the-configuration For convenience, provide variables for predefined values describing the officially supported camera devices listed in the above documentation. Signed-off-by: Vivien Didelot <[email protected]>

Reword the camera module section to describe the usage of the new RASPBERRYPI_CAMERA variable. Signed-off-by: Vivien Didelot <[email protected]>

vivien · 2023-09-26T14:45:24Z

See this comment for explanation behind the motivation for a clean config file.

conf/machine/include/rpi-base.inc

recipes-bsp/bootfiles/rpi-config_git.bb

agherzan · 2023-10-10T12:48:36Z

recipes-bsp/bootfiles/rpi-config.bb

@@ -2,18 +2,16 @@ DESCRIPTION = "Commented config.txt file for the Raspberry Pi. \
               The Raspberry Pi config.txt file is read by the GPU before \
               the ARM core is initialised. It can be used to set various \
               system configuration parameters."
+HOMEPAGE = "https://www.raspberrypi.com/documentation/computers/config_txt.html"


I would recommend this being split in another PR. It does not directly support the camera modules and we can have more public visibility on this in this way.

agherzan · 2023-10-10T12:50:17Z

recipes-bsp/bootfiles/rpi-config.bb

+RASPBERRYPI_CAMERA ??= "auto"
+RASPBERRYPI_CAMERA_V1 = "ov5647"
+RASPBERRYPI_CAMERA_V2 ?= "imx219"
+RASPBERRYPI_CAMERA_HQ = "imx477"


I propose keeping this simple with only RASPBERRYPI_CAMERA and giving documentation examples of its values. Having indirect definitions and keeping them updated with new variables is a situation that might cause more confusion.

agherzan · 2023-10-10T12:52:14Z

docs/extra-build-config.md


-Similarly, the Raspberry Pi Camera Module v3 also has to be explicitly enabled in local.conf.
+    RASPBERRYPI_CAMERA = "${RASPBERRYPI_CAMERA_V3}"


There are more values possible here. I would only keep RASPBERRYPI_CAMERA and have examples of direct assignments instead of having to document indirect variables.

agherzan requested changes Aug 24, 2023

View reviewed changes

vivien force-pushed the camera branch from 6554a59 to f4c700d Compare September 14, 2023 02:04

vivien added 4 commits September 13, 2023 22:10

rpi-base: sort overlays

71d47ee

Sort kernel device tree overlays alphabetically. Signed-off-by: Vivien Didelot <[email protected]>

rpi-base: add overlays for all supported cameras

e70003c

Add the device tree overlays for the supported cameras as listed on https://www.raspberrypi.com/documentation/computers/camera_software.html#if-you-do-need-to-alter-the-configuration Signed-off-by: Vivien Didelot <[email protected]>

agherzan requested changes Sep 19, 2023

View reviewed changes

vivien added 2 commits September 21, 2023 10:40

docs: document the RASPBERRYPI_CAMERA variable

2615a1e

Reword the camera module section to describe the usage of the new RASPBERRYPI_CAMERA variable. Signed-off-by: Vivien Didelot <[email protected]>

vivien force-pushed the camera branch from f4c700d to 2615a1e Compare September 21, 2023 14:42

vivien requested a review from agherzan October 6, 2023 16:31

agherzan requested changes Oct 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add officially supported camera modules #1213

Add officially supported camera modules #1213

vivien commented Aug 15, 2023

vivien commented Aug 21, 2023

agherzan Aug 24, 2023

vivien Aug 25, 2023

agherzan Aug 29, 2023 •

edited

Loading

agherzan Sep 19, 2023

kraj Sep 21, 2023

vivien Sep 26, 2023

agherzan Oct 10, 2023

djr-spectrummfg Nov 17, 2023

agherzan Nov 23, 2023

kraj Nov 23, 2023

agherzan Sep 19, 2023

vivien Sep 21, 2023

agherzan Sep 21, 2023

vivien Sep 21, 2023 •

edited

Loading

agherzan Oct 10, 2023

vivien commented Sep 26, 2023

agherzan Oct 10, 2023

agherzan Oct 10, 2023

agherzan Oct 10, 2023


		Similarly, the Raspberry Pi Camera Module v3 also has to be explicitly enabled in local.conf.
		RASPBERRYPI_CAMERA = "${RASPBERRYPI_CAMERA_V3}"

Add officially supported camera modules #1213

Are you sure you want to change the base?

Add officially supported camera modules #1213

Conversation

vivien commented Aug 15, 2023

vivien commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agherzan Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivien Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivien commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agherzan Aug 29, 2023 •

edited

Loading

vivien Sep 21, 2023 •

edited

Loading