Skip to content

video: esp32: initialize format #95890

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −0
Open

video: esp32: initialize format #95890

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions drivers/video/video_esp32_dvp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,16 @@ static int video_esp32_init(const struct device *dev)
return -ENODEV;
}

if (!device_is_ready(cfg->source_dev)) {
LOG_ERR("Source device not ready");
return -ENODEV;
}
Comment on lines +389 to +392
Copy link
Contributor

@ngphibang ngphibang Sep 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@epc-ake epc-ake Sep 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ngphibang ngphibang Sep 15, 2025

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@epc-ake epc-ake Sep 17, 2025

Choose a reason for hiding this comment

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

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


if (video_get_format(dev, &data->video_format) < 0) {
LOG_ERR("Failed to get default format from source device");
return -EINVAL;
}
Comment on lines +394 to +397
Copy link
Contributor

@ngphibang ngphibang Sep 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@epc-ake epc-ake Sep 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ngphibang ngphibang Sep 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@epc-ake epc-ake Sep 17, 2025

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@josuah josuah Sep 17, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@ngphibang ngphibang Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)


return 0;
}

Expand Down