GIGA/Portenta H7 - Camera code with the new deferred init setup...

[KurtE]

There are probably issues with the Camera code with the current defer init settings. Similar but more complex than the Giga Display touch issue.

For about the last maybe 8 months, I have been running the camera code in a deferred init manor, with the Pull Request #195, which I am trying to address the issues with the newly introduced deferred stuff.

Note: In the Device tree we have:

&dcmi {
	status = "okay";
	/* ext-sdram = <&sdram1>; */
	pinctrl-0 = <&dcmi_hsync_ph8 &dcmi_pixclk_pa6 &dcmi_vsync_pi5
				&dcmi_d0_ph9 &dcmi_d1_ph10 &dcmi_d2_ph11 &dcmi_d3_pg11
				&dcmi_d4_ph14 &dcmi_d5_pi4 &dcmi_d6_pi6 &dcmi_d7_pi7>;
	pinctrl-names = "default";
	dmas = <&dma1 0 75 (STM32_DMA_PERIPH_TO_MEMORY | STM32_DMA_PERIPH_NO_INC |
			STM32_DMA_MEM_INC | STM32_DMA_PERIPH_32BITS | STM32_DMA_MEM_32BITS |
			STM32_DMA_PRIORITY_HIGH) STM32_DMA_FIFO_1_4>;

	port {
		dcmi_ep_in: endpoint {
			remote-endpoint-label = "gc2145_ep_out";
			bus-width = <8>;
			hsync-active = <0>;
			vsync-active = <0>;
			pclk-sample = <0>;
		};
	};
};
...
&i2c4 {
	status = "okay";
	zephyr,deferred-init;
	gc2145: gc2145@3c {
		compatible = "galaxycore,gc2145";
		reg = <0x3c>;
		reset-gpios = <&gpiod 4 GPIO_ACTIVE_LOW>;
		pwdn-gpios = <&gpioa 1 GPIO_ACTIVE_LOW>;

		port {
			gc2145_ep_out: endpoint {
				remote-endpoint-label = "dcmi_ep_in";
			};
		};
	};
};
...
&pwm1 {
	/* Use the pwmclock node to start the clock generation */
	pwmclock: pwmclock {
		status = "okay";
		compatible = "pwm-clock";
		clock-frequency = <0>;
		#clock-cells = <1>;
		pwms = <&pwm1 3 PWM_HZ(12000000) PWM_POLARITY_NORMAL>;
	};
};

	chosen {
		zephyr,camera = &dcmi;
		zephyr,log-uart = &log_uarts;
		/* zephyr,console = &board_cdc_acm_uart; */
	};

The order of these sections may not be what is in the file...

Main issues:

1. Note: neither DCMI nor GC2145 objects are marked as deferred, yet GC2145 is under
   the i2c4 object which is deferred.

2. Currently at startup, there is code in the loader/fixups.c:

#if defined(CONFIG_BOARD_ARDUINO_GIGA_R1) && defined(CONFIG_VIDEO)
#include <zephyr/kernel.h>
#include <zephyr/device.h>
#include <zephyr/drivers/clock_control.h>
#include <zephyr/logging/log.h>

int camera_ext_clock_enable(void) {
	int ret;
	uint32_t rate;
	const struct device *cam_ext_clk_dev = DEVICE_DT_GET(DT_NODELABEL(pwmclock));

	if (!device_is_ready(cam_ext_clk_dev)) {
		return -ENODEV;
	}

	ret = clock_control_on(cam_ext_clk_dev, (clock_control_subsys_t)0);
	if (ret < 0) {
		return ret;
	}

	ret = clock_control_get_rate(cam_ext_clk_dev, (clock_control_subsys_t)0, &rate);
	if (ret < 0) {
		return ret;
	}

	return 0;
}

SYS_INIT(camera_ext_clock_enable, POST_KERNEL, CONFIG_CLOCK_CONTROL_PWM_INIT_PRIORITY);

#endif

That gets called at startup and has to setup before the camera code can begin as many of these cameras I2C communications will fail if there is no clock signal.

Both DCMI and the GC2145 will try to start at boot.
The GC2145 startup code will fail to start.

static int gc2145_init(const struct device *dev)
{
	/* set default/init format VGA RGB565 */
	struct video_format fmt = {
		.pixelformat = VIDEO_PIX_FMT_RGB565,
		.width = RESOLUTION_VGA_W,
		.height = RESOLUTION_VGA_H,
	};
	int ret;
	const struct gc2145_config *cfg = dev->config;
	(void) cfg;

	if (!i2c_is_ready_dt(&cfg->i2c)) {
		LOG_ERR("Bus device is not ready");
		return -ENODEV;
	}

With the Bus not ready error.

I am trying to address this stuff in the PR mentioned. (Marked both DCMI and GC2145 as deferred-init.
Note: I removed the code from the loader and instead add it to the Camera::begin code, as why should we startup up the clock and camera objects if the user code does not use them...

Probably need to call:
(void)zephyr::arduino::init_dev_apply_pinctrl(cam_ext_clk_dev);
On the clock object to make sure?

Now the need to startup the DCMI object

#if DT_HAS_CHOSEN(zephyr_camera)
	this->vdev = DEVICE_DT_GET(DT_CHOSEN(zephyr_camera));
#endif
...
	if ((ret = zephyr::arduino::init_dev_apply_pinctrl(this->vdev)) != 0) {
		printk("Camera::begin failed = vedev: %d\n", ret);
		return false;
	}

Now need to start the actual camera. Did not know any other way to get hold of it, so it was suggested by Josuah, I added a reference to it in chosen and reference it:

#if DT_HAS_CHOSEN(zephyr_camera_sensor)
	const struct device *camera_sensor = DEVICE_DT_GET(DT_CHOSEN(zephyr_camera_sensor));
	//struct i2c_dt_spec i2c;
	//i2c = I2C_DT_SPEC_GET(device);
	//printk("*** i2c.bus = %p\n", i2c.bus);


	if ((ret = zephyr::arduino::init_dev_apply_pinctrl(camera_sensor)) != 0) {
		printk("Camera::begin failed = sensor: %d\n", ret);
		return false;
	}

#endif

But this current fails as the I2C object is not ready...
With the GIGA display touch code, the I2C object was passed into the begin, and as @pillo79 mentioned in the PR arduino-libraries/Arduino_GigaDisplayTouch#18

This is an ad-hoc solution that doesn't scale, but it works for now. Moving forward I think we should use the Zephyr-provided dependency information to support this kind of scenarios.

So main question is, how best to get a handle to the I2C object to startup first?
The GC2145 object gets it from:

#define GC2145_INIT(n)										\
	static struct gc2145_data gc2145_data_##n;						\
	static const struct gc2145_config gc2145_cfg_##n = {					\
		.i2c = I2C_DT_SPEC_INST_GET(n),							\

Not sure of best way to get it at run time from the object:
const struct device *camera_sensor = DEVICE_DT_GET(DT_CHOSEN(zephyr_camera_sensor));

Suggestions: the hacker in me may start with a quick and dirty hack:

struct  camera_config {
	struct i2c_dt_spec i2c;
};
const struct camera_config *cfg = camera_sensor ->config;

But don't think that is the proper way. 😆

Suggestions?

[KurtE]

Actually first question is... How do I start the PWMClock?

That is:

&timers1 {
	status = "okay";
	st,prescaler = <0>;

	pwm1: pwm {
		status = "okay";
		zephyr,deferred-init;
		pinctrl-0 = <>;
		pinctrl-1 = <>;
		pinctrl-2 = <&tim1_ch3_pj9 &tim1_ch1_pk1 &tim1_ch2_pj11>;
		pinctrl-names = "default", "sleep", "arduino";
	};
};

&pwm1 {
	/* Use the pwmclock node to start the clock generation */
	pwmclock: pwmclock {
		status = "okay";
		compatible = "pwm-clock";
		clock-frequency = <0>;
		#clock-cells = <1>;
		pwms = <&pwm1 3 PWM_HZ(12000000) PWM_POLARITY_NORMAL>;
	};
};

		pwms = <&pwm1 3 PWM_HZ(12000000) PWM_POLARITY_NORMAL>,
			   <&pwm2 4 PWM_HZ(500) PWM_POLARITY_NORMAL>,
			   <&pwm2 3 PWM_HZ(500) PWM_POLARITY_NORMAL>,
			   <&pwm8 1 PWM_HZ(500) PWM_POLARITY_NORMAL>,
			   <&pwm3 2 PWM_HZ(500) PWM_POLARITY_NORMAL>,
			   <&pwm4 2 PWM_HZ(500) PWM_POLARITY_NORMAL>,
			   <&pwm3 1 PWM_HZ(500) PWM_POLARITY_NORMAL>,
			   <&pwm4 3 PWM_HZ(500) PWM_POLARITY_NORMAL>,
			   <&pwm4 4 PWM_HZ(500) PWM_POLARITY_NORMAL>,
			   /* D10-D13 in arduino_header order */
			   <&pwm1 1 PWM_HZ(5000) PWM_POLARITY_NORMAL>,
			   <&pwm8 2 PWM_HZ(500) PWM_POLARITY_NORMAL>,
			   <&pwm1 2 PWM_HZ(5000) PWM_POLARITY_NORMAL>,
			   <&pwm12 1 PWM_HZ(500) PWM_POLARITY_NORMAL>,
			   <&pwm1 1 PWM_HZ(5000) PWM_POLARITY_NORMAL>;

The above code either new/old probably does not work. As well it is timer1 is deferred. Also
not sure if someone needs to select a different pin state to get to pinctrl 2?
...

Verified that my code is not properly starting up the clock:
Only pins being modified are the reset/pwdn and I2C with my above hack and/or I manually started Wire and Wire1...

[KurtE]

Quick notes: Wanted to make sure my changes did not break it.
First off with current RELEASED zephyr/giga board:
Running the highly complicated Sketch (Blink)

With current sources:

Nothing...

[KurtE]

@dani-clo @pillo79 @per1234 I believe that the idea #415 is great, however it is unclear to me the proper fix for starting the camera pwm clock, as I mentioned in the first several posts in this issue.

I may try a couple of hacks to get it running again on GIGA
a) on GIGA somehow pass through the pin number that the PWM is on and see if I do a PWM write... But that will probably overwrite the settings that are defined for the clock, and currently won't work on Portenta H7, as the only pins that are currently defined for Arduino are the pins on the exterior of the H7, none of the other expansion pins.

b) see what happens if I define that one clock signal still on pinctrl_0...

c) ???

Suggestions?

EDIT: I tried b) like:

int camera_ext_clock_enable(void) {
	int ret;
	uint32_t rate;
	const struct device *cam_ext_clk_dev = DEVICE_DT_GET(DT_NODELABEL(pwmclock));

	if (!cam_ext_clk_dev) {
		return -ENODEV;		
	}

	(void)zephyr::arduino::init_dev_apply_pinctrl(cam_ext_clk_dev);

	ret = clock_control_on(cam_ext_clk_dev, (clock_control_subsys_t)0);
	if (ret < 0) {
		return ret;
	}

	ret = clock_control_get_rate(cam_ext_clk_dev, (clock_control_subsys_t)0, &rate);
	if (ret < 0) {
		return ret;
	}

	return 0;
}
#else
#ERROR "CONFIG_VIDEO is not defined for this variant"
#endif

with:

&timers1 {
	status = "okay";
	st,prescaler = <0>;

	pwm1: pwm {
		status = "okay";
		zephyr,deferred-init;
		pinctrl-0 = <&tim1_ch3_pj9>;
		pinctrl-1 = <>;
		pinctrl-2 = <&tim1_ch3_pj9 &tim1_ch1_pk1 &tim1_ch2_pj11>;
		pinctrl-names = "default", "sleep", "arduino";
	};
};

I did not get an error. from my calling of this, but debug output:

Before cam.begin
*** i2c.bus = 0x809db2c
[00:00:05.776,000] <err> video_common: failed to write-read to I2C register
                                       f0                                               |.          
[00:00:05.793,000] <err> video_common: Failed to read from register 0xf0
Camera::begin failed = sensor: 5

And Logic Analyzer verified that there the clock pin is not doing anything.

EDIT 2
Hacked test app and added:

  Wire.begin();
  Wire1.begin();
  analogWrite(57, 64);
  printk("Before cam.begin\n");
  if (!cam.begin(CAMERA_WIDTH, CAMERA_HEIGHT, CAMERA_FORMAT, true)) {
    fatal_error("Camera begin failed");
  }

Hacked the camera start code

#if 1
	if ((ret = zephyr::arduino::init_dev_apply_pinctrl(camera_sensor)) != 0) {
		printk("Camera::begin failed = sensor: %d\n", ret);
		//return false;
	}
#endif

Did not error it out, but ret = -2

Now I am actually getting an image...