Skip to content

Conversation

KurtE
Copy link
Contributor

@KurtE KurtE commented Oct 21, 2025

Add the ability to run cameras in a logical
snapshot mode instead of always running
in video mode. In particular, the camera
starts when you ask for an image to be dequeued
and stops when it receives an image.

There are three ways to setup to run in this mode:

a) define a max of 1 buffer:
CONFIG_VIDEO_BUFFER_POOL_NUM_MAX=1

b) Tell it to do so in the overlay by giving the DCMI object the property: snapshot-mode

c) At run time: I added a video control: VIDEO_CID_SNAPSHOT_MODE Which can have the values 0 - normal, 1 - snapshot

As mentioned in snapshot mode, the HAL DMA is stopped after each image and restarted when the user asks for another image. I added a timeout default to 1 second, that handles the case where sometimes the HAL is silently not successful in retrieving an image.

Note: This is the second half of what used to be #93797

@KurtE
Copy link
Contributor Author

KurtE commented Oct 22, 2025

Update:

I added 2nd mode to snapshot to experiment with starting and stopping the video streams along with
the HAL start/stop.

Wondering if it might help or hurt on startup speed. That is in Snapshot mode:
HAL_DCMI_START_DMA with the DCMI_MODE_SNAPSHOT, the code is supposed to align itself to the start of
a frame before it starts to return data. Now if it is free running maybe it has to wait through almost a full
frame before it sees the start of next frame. Maybe if it is started up fresh, maybe it starts a a new frame...

Also was wondering if it might help reduce tearing when it gets an error. So far It looks like I at times it still
gets into tearing images if a DMA error happens. I think I am getting fewer of those errors in this mode,
but still at times showing split images and often takes until next error to maybe recover.

Note: I have not yet adding the stop/start in the DMA recovery code will try that as well.

Also wondering if maybe something with the DMA buffer should be cleared when error happens.

KurtE added 2 commits October 23, 2025 05:08
Add the ability to run cameras in a logical
snapshot mode instead of always running
in video mode.  In particular, the camera
starts when you ask for an image to be dequeued
and stops when it receives an image.

There are three ways to setup to run in this mode:

a) define a max of 1 buffer:
CONFIG_VIDEO_BUFFER_POOL_NUM_MAX=1

b) Tell it to do so in the overlay by giving the DCMI object
the property: snapshot-mode

c) At run time:  I added a video control: VIDEO_CID_SNAPSHOT_MODE
Which can have the values 0 - normal, 1 - snapshot

As mentioned in snapshot mode, the HAL DMA is stopped
after each image and restarted when the user asks for another
image.  I added a timeout default to 1 second, that handles
the case where sometimes the HAL is silently not successful
in retrieving an image.

Signed-off-by: Kurt Eckhardt <[email protected]>
Experimenting with the optional snapshot_mode=2

With this it calls off to stop the video stream after each frame
is returned and restarts it when you ask for a new frame.

Testing to see if it helps or hurts the throughput speed.

Also testing if it helps/hurts with image tearing after
DMA errors.

Signed-off-by: Kurt Eckhardt <[email protected]>
@KurtE KurtE force-pushed the camera_snapshot_snap branch from 8ab99bc to 8a69c9e Compare October 23, 2025 12:08
@sonarqubecloud
Copy link

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Mainly minor comments.

struct dma_config cfg;
};


Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove this extra empty line?

struct video_stm32_dcmi_data *data = dev->data;
const struct video_stm32_dcmi_config *config = dev->config;

LOG_DBG("dequeue snapshot: %p %llu\n", data->vbuf, timeout.ticks);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpipcking: \n not neded.
Here and below.

return 0;
}
data->vbuf = k_fifo_get(&data->fifo_in, K_NO_WAIT);
LOG_DBG("\tcamera buf: %p\n", data->vbuf);
Copy link
Contributor

Choose a reason for hiding this comment

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

Heading tab \t expected?

*vbuf = k_fifo_get(&data->fifo_out, timeout);
if (*vbuf == NULL) {
uint32_t time_since_start_time =
(uint32_t)(k_uptime_get_32() - data->snapshot_start_time);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add an empty line below this variable definition?

}
/* verify it did not come in during this procuessing */
*vbuf = k_fifo_get(&data->fifo_out, K_NO_WAIT);
if (*vbuf) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (*vbuf) {
if (*vbuf != NULL) {


LOG_INF("Set Snapshot: %d\n", value);
if ((CONFIG_VIDEO_BUFFER_POOL_NUM_MAX == 1) && (value == 0)) {
LOG_DBG("Only one buffer so snapshot mode only");
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth a LOG_ERR()?

.def = data->snapshot_mode ? 1 : 0});
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove this extra empty line.

Comment on lines 597 to 599
return video_init_ctrl(&ctrls->snapshot, dev, VIDEO_CID_SNAPSHOT_MODE,
(struct video_ctrl_range){.min = 0, .max = 1, .step = 1,
.def = data->snapshot_mode ? 1 : 0});
Copy link
Contributor

Choose a reason for hiding this comment

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

Requires an extra indentation space char:

Suggested change
return video_init_ctrl(&ctrls->snapshot, dev, VIDEO_CID_SNAPSHOT_MODE,
(struct video_ctrl_range){.min = 0, .max = 1, .step = 1,
.def = data->snapshot_mode ? 1 : 0});
return video_init_ctrl(&ctrls->snapshot, dev, VIDEO_CID_SNAPSHOT_MODE,
(struct video_ctrl_range){.min = 0, .max = 1, .step = 1,
.def = data->snapshot_mode ? 1 : 0});

Comment on lines 716 to 720
data->snapshot_mode = config->snapshot_mode;
if (CONFIG_VIDEO_BUFFER_POOL_NUM_MAX == 1) {
LOG_DBG("Only one buffer so snapshot mode only");
data->snapshot_mode = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer

	if (CONFIG_VIDEO_BUFFER_POOL_NUM_MAX == 1) {
		LOG_DBG("Only one buffer so snapshot mode only");
		data->snapshot_mode = true;
	} else {
		data->snapshot_mode = config->snapshot_mode;
	}

};


#define SNAPSHOT_STOP_STREAM_MODE 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer have all macros defined before the struct's.

Could you remove the extra empty line below?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants