video: esp32: initialize format

[epc-ake]

Make sure the driver initializes with the sensors default format.

[josuah]

This was missing, good catch.

Though, by directly asking source_dev, this does not set data->video_format.pitch (responsibility of the receiver).

Using video_get_format() on dev rather than source_dev will make video_esp32_get_fmt() be used, which correctly set the pitch.

Would that make sense? There might be better ways which you may prefer.

Thanks!

[epc-ake]

This was missing, good catch.

Though, by directly asking source_dev, this does not set data->video_format.pitch (responsibility of the receiver).

Using video_get_format() on dev rather than source_dev will make video_esp32_get_fmt() be used, which correctly set the pitch.

Would that make sense? There might be better ways which you may prefer.

Thanks!

Thanks, @josuah! Good catch :-)
Funny enough, I had it right initially but then thought I needed to change it...

[ngphibang]

Is there any reason to require the sensor to be initialized before the dvp ? If so, you should change the init order of the dvp (currently, they are the same order and no guarantee for that).

Otherwise, I don't see in the code any reason for that.

[epc-ake]

Because the call to video_get_format(dev, &data->video_format) will be forwarded to the source_dev i think this is needed.

The driver expects the source property set and therefore I think the initialization sequence is correct because lcd_cam depends on the sensor driver?
https://github.com/zephyrproject-rtos/zephyr/blob/5bef4a9d6264761c10d739b345eb2f7ded719099/boards/espressif/esp32s3_eye/esp32s3_eye_procpu.dts#L150C1-L150C21

Or should the driver actually be updated to make use of remote-endpoint? In that case yes we would have to handle the initialization sequence differently.

[ngphibang]

Oh, just realize that the esp32_dvp is not yet migrated to use the port / endpoint mechanism, issue here:
#80514

Could you help to do this if possible ? An example is here #88323

With the new remote-endpoint and no direct phandle reference we don't need the sensor initialized before the dvp anymore (if it is not explicitely required for some reason)

[epc-ake]

maybe I find some time tomorrow. I will do an other PR then.

[ngphibang]

Could you explain why the esp32_dvp needs to get the format from the source here in init() function as I don't see it is used after this line.
Basically, format will be set by the application after all drivers initialization and even the sensor doesn't set a format by default, it 's not a problem.

[epc-ake]

Is it intentional not to set a default format in the drivers?
In my opinion, sensor drivers should initialize with a default format.
For example, I noticed that with the video shell, the default format before this PR ends up as (fmt=0x0, width=0, height=0), which seems like an invalid state and something that should always be avoided.

[ngphibang]

It is not intentional, most of the sensor drivers set its default format. However, it is not mandatory, so we should not check this in the receiver init function and return failed as it's not an issue if it is not set at initialization, I think.

[epc-ake]

@josuah could you give your oppinion on this?
Here’s the sequence where I encounter this issue:

1. Start the Video Shell.
2. Retrieve the current format and observe that the default format from the sensor driver is already set. This leads to the assumption that setting the same format again is unnecessary.
3. Attempt to capture images, but receive strange messages indicating that a buffer of size 0 is enqueued.
4. The system either freezes or responds with an error.

The root cause is that the sensor and capture driver are not properly synchronized after startup.

[josuah]

Currently there is the assumption that a default format will be set before any intervention from the sample:

zephyr/samples/drivers/video/capture/src/main.c

Lines 144 to 149 in 1d6b675

	/* Get default/native format */
	fmt.type = type;
	if (video_get_format(video_dev, &fmt)) {
	LOG_ERR("Unable to retrieve video format");
	return 0;
	}

But this is a tricky situation: now users in a might want to modify the driver (where the default comes from) to get the resolution. pixfmt=0x0, width=0, height=0 sounds like sane default: un-configured state, refusing to start with a clear error message (i.e. shell, video protocols stacks, libraries, samples).

In the meantime, setting the default is still the best that can be done?

[ngphibang]

I think we need to add a check to the samples so that if fmt is not set correctly, we should not continue. The order should be

• first taking the format from the configs (CONFIG_VIDEO_PIXEL_FORMAT, WIDTH, HEIGHT) and check it then if nothing
• taking the default format (and check it), then if still nothing, should not continue

Anyway, checking the sensor default format here in the receiver init() and force the driver stop initializing here is not the right way I think, because user can set the format later.

Furthermore, the check :

 if (video_get_format(video_dev, &fmt)) { 
 	LOG_ERR("Unable to retrieve video format"); 
 	return 0; 
 } 

is not sufficient as get_format() can be successful but fmt can be {0}, the check should be done on the valid range (retrieved from caps)

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.