VIDEO: GC2145/stm32_dcmi support for CROP/Snapshot

[KurtE]

Added support to video_stm32_dcmi for the new
set and get selection. These implementations simply forward the message to the underlying
camera object if they support these messages.

Also added support for a snapshot mode instead of
always using continuous capture mode. Tried
to make it semi-transparent when you desire it to be

The stm32_dcmi code now also allows you to work
with only one buffer. This will force it into snap shot mode. There is also new calls added to the api for: video_get_snapshot_mode and video_set_snapshot_mode.

That allows you to set it with more than one buffer and query what mode you are in.

GC2145 was updated first to try out these changes. The camera now allows me to follow the call order
that @josuah mentioned in another pr/issue.

With this driver I also updated it to allow more or less any video resolution:
{
.pixelformat = format, .width_min = width_l, .width_max = width_h,
.height_min = height_l, .height_max = height_h, .width_step = 0, .height_step = 0,
}

static const struct video_format_cap fmts[] = {
	GC2145_VIDEO_FORMAT_CAP_HL(128, 1600, 128, 1200, VIDEO_PIX_FMT_RGB565),
	GC2145_VIDEO_FORMAT_CAP_HL(128, 1600, 128, 1200, VIDEO_PIX_FMT_YUYV),

When resolution is set, it computes the scale factor. If you then later call set_crop, the same code is
used except it uses the ratios computed from the set_resolution.

With these changes: I was able to setup a test app, for the Arduino Nicla vision and send out a 480x320 image over USB.

More to come

Note: this is a replacement for #91975

My current test sketch/app is up at:
https://github.com/KurtE/zephyr_test_sketches/tree/master/camera_capture_to_usb
built using:

west build -p -b arduino_nicla_vision//m7
west flash

I am using the Arducam viewer with this on my PC. I am using the one at:
https://github.com/mjs513/Teensy_Camera/tree/main/extras/host_app

Picture using GC2145 on Arduino Nicla Vision shown on Arducam mini viewer.

Edit: current summary of changes:
There are several changes some of which will likely change if/when code reviews happen. Things like:
a) The stm_dcmi driver handles the get/set selection APIs and if the camera has also implemented these APIs, it
forwards the messages to them, else return error not implemented.

a1) GC2145 camera implements them.

b) Currently I allow arbitrary size of the frame GC2145, that is I have one fmt (per RGB...) which sets min and max versus the
ones where it current code which has 3 arbitrary sizes (1600x1200 - ratio 1, 640x480 ratio 2, 320x240 ratio 3). I instead
compute ratio and allow you to choose for example 800x600 which is less arbitrary than the 640x480... Note 320x240
computes ratio=5, except I currently limit to max ratio=3 per seeing other implementations that do so... Maybe should
make that max configurable.

c) Setting to allow the code to run in SNAPSHOT mode, which starts the camera, waits for one frame to come back and then deactivates.

This is the way that Arduino library works at least on MBED. Note snapshot mode also has some ability to recover from
failures...

d) Allow you to configure to only have one buffer, before it required at least two, If set to 1, it set forces SNAPSHOT mode.

With this running on Zephyr, I for example was able to program a Nicla Vision, which has no SDRAM and output at
480x320 over USB to an Arducam viewer. My test sketch (not sure what to call them on Zephyr)
is up at https://github.com/KurtE/zephyr_test_sketches/tree/master/camera_capture_to_usb

Also have others that output to Portenta H7 to an ST7796 display...

[KurtE]

@josuah @mjs513 @dkalowsk @iabdalkader and all:

As @josuah mentioned in my previous PR:
#91975 (comment)
Which I closed as per his earlier comments about using the new set/get selection video stuff.

This might make more sense when considering there is only one particular order users are expected to follow:

1. Set the format with video_set_format()
2. Set the cropping region with video_set_selection(dev, VIDEO_SET_TGT_CROP)
3. Set the scaling parameter with video_set_selection(dev, VIDEO_SET_TGT_COMPOSE)
   Every step reset the values of what is below it: "select the native size, remove margins, and scale it up/down" always in this order.

Which makes sense: But now wondering about a few details. In my own test case app I have:

	LOG_INF("- Video format: %s %ux%u",
		VIDEO_FOURCC_TO_STR(fmt.pixelformat), fmt.width, fmt.height);

	if (video_set_format(video_dev, &fmt)) {
		LOG_ERR("Unable to set format");
		return 0;
	}

#if CONFIG_VIDEO_FRAME_HEIGHT || CONFIG_VIDEO_FRAME_WIDTH
#if CONFIG_VIDEO_FRAME_HEIGHT
	fmt.height = CONFIG_VIDEO_FRAME_HEIGHT;
#endif

#if CONFIG_VIDEO_FRAME_WIDTH
	fmt.width = CONFIG_VIDEO_FRAME_WIDTH;
#endif
#endif	

	/* First set the format which has the size of the frame defined */
	LOG_INF("video_set_format: %u %u", fmt.width, fmt.height);
	if (video_set_format(video_dev, &fmt)) {
		LOG_ERR("Unable to set format");
		return 0;
	}

	/* initialize the bsize to the size of the frame */
	bsize = fmt.width * fmt.height * 2;
	/* Set the crop setting if necessary */
#if CONFIG_VIDEO_SOURCE_CROP_WIDTH && CONFIG_VIDEO_SOURCE_CROP_HEIGHT
	sel.target = VIDEO_SEL_TGT_CROP;
	sel.rect.left = CONFIG_VIDEO_SOURCE_CROP_LEFT;
	sel.rect.top = CONFIG_VIDEO_SOURCE_CROP_TOP;
	sel.rect.width = CONFIG_VIDEO_SOURCE_CROP_WIDTH;
	sel.rect.height = CONFIG_VIDEO_SOURCE_CROP_HEIGHT;
	LOG_INF("video_set_selection: VIDEO_SEL_TGT_CROP(%u, %u, %u, %u)", 
			sel.rect.left, sel.rect.top, sel.rect.width, sel.rect.height);
	if (video_set_selection(video_dev, &sel)) {
		LOG_ERR("Unable to set selection crop  (%u,%u)/%ux%u",
			sel.rect.left, sel.rect.top, sel.rect.width, sel.rect.height);
		return 0;
	}
	LOG_INF("Selection crop set to (%u,%u)/%ux%u",
		sel.rect.left, sel.rect.top, sel.rect.width, sel.rect.height);
	bsize = sel.rect.width * sel.rect.height * 2;
#endif

	if (video_get_format(video_dev, &fmt)) {
		LOG_ERR("Unable to retrieve video format");
		return 0;
	}
	LOG_INF("video_get_format: ret fmt:%u w:%u h:%u pitch:%u",fmt.pixelformat, fmt.width, fmt.height, fmt.pitch);

And the CONF file has:

CONFIG_VIDEO_FRAME_WIDTH=800
CONFIG_VIDEO_FRAME_HEIGHT=600
CONFIG_VIDEO_SOURCE_CROP_WIDTH=480
CONFIG_VIDEO_SOURCE_CROP_HEIGHT=320

But if I was not using my updated fmts, which allows more resolutions, I would have done FRAME_WIDTH=640 and HEIGHT=480

So now assume:

CONFIG_VIDEO_FRAME_WIDTH=640
CONFIG_VIDEO_FRAME_HEIGHT=480

With this, the call to video_set_format would have a width=640 and height= 480
Which internally sets the ratio (scaling to 2) and crop to 640x480.

Note: If you now (first commit) call video_get_selection with VIDEO_SEL_TGT_NATIVE_SIZE, it will return 800x600

So now to do step 2) to crop it to 480x320 - Currently I ignore the passed in top and left in the crop rectangle, but that
is what I wish to update in the next commit, The current code sets the crop rectangle top=0, left=0. However internally
it actually sets the crop to (80,60, 640, 480) to center the image in the sensor.

So with this setup, I would expect on the TGT_CROP that I should be able to pass in rectangles in the range:
(0, 0, 480, 320) - Upper left area of sensor. to
(159, 139, 480, 320) - lower right (not sure about if 159 or 160...)
Which would allow you to pan over the entire sensor...

But in this case: should step 1) have set the top=0, left=0 or should it instead of set it to 80,60?
Should the sketch calling the setting the TGT_CROP compute this themself, that is currently if you set it to 0, 0
it would be at one end of the sensor? Should there be a default value?

Thanks
Kurt

[KurtE]

Having problems with this PR first commit on getting signoff to be accepted:

I actually copy/pasted the signoff line from previous PRS which worked then but not on this one?

Signed-off-by: Kurt Eckhardt <[email protected]>
I tried using:
Signed-off-by: Kurt E <[email protected]>
which I had changed my profile to have: Kurt E instead of KurtE for profile name...
I tried what: git commit -s added:
`Signed-off-by: KurtE [email protected]

All of which failed like:

  -- Run compliance checks on patch series (PR): Identity.txt#L0See https://docs.zephyrproject.org/latest/contribute/guidelines.html#commit-guidelines for more details 52ea30be81e17759c1725a3ef7e7c2c64d6ed5c0: Signed-off-by line (Signed-off-by: Kurt Eckhardt ) does not follow the syntax: First Last .  

[Run compliance checks on patch series (PR): Identity.txt#L0](https://github.com//pull/93797/files#annotation_37244444845) See https://docs.zephyrproject.org/latest/contribute/guidelines.html#commit-guidelines for more details

52ea30be81e17759c1725a3ef7e7c2c64d6ed5c0: Signed-off-by line (Signed-off-by: Kurt Eckhardt [email protected]) does not follow the syntax: First Last .

[KurtE]

More mumbling to self ;)
Reworking some of this, based on what was done elsewhere including our Teensy_camera code.

With this, the call to video_set_format would have a width=640 and height= 480 Which internally sets the ratio (scaling to 2) and crop to 640x480.

Note: If you now (first commit) call video_get_selection with VIDEO_SEL_TGT_NATIVE_SIZE, it will return 800x600

Reworking: Currently I have the crop code recalculate most of the window and crop registers. Will instead have it
only update the crop registers... As such if you passed in 640x480 on set_format, that is what you are limited to.
So VIDEO_SEL_TGT_NATIVE_SIZE will return 640x480, so I need to save that away and/or grab it from registers.

As the setting the crop updates the fmt structure:

		drv_data->fmt.width = drv_data->crop.width;
		drv_data->fmt.height = drv_data->crop.height;
		drv_data->fmt.pitch = drv_data->fmt.width
			* video_bits_per_pixel(drv_data->fmt.pixelformat) / BITS_PER_BYTE;

Why? Because the buffer management code requires the buffers to be that size:

static int video_stm32_dcmi_enqueue(const struct device *dev, struct video_buffer *vbuf)
{
	struct video_stm32_dcmi_data *data = dev->data;
	const uint32_t buffer_size = data->fmt.pitch * data->fmt.height;

	if (buffer_size > vbuf->size) {
		return -EINVAL;
	}
...

So for example if I setup for the GC2145 camera to output 480x320 the buffer size needed is: 307200 bytes
which the STM32H747 like the Nicla Vision can hold in its memory. Now if the buffer size calculation is based on:
640x480 (614400 bytes) or worse 800x600 (960000 bytes), won't fit in memory.

This also effects the range of crop LEFT and TOP to fit the 480x320 within the range of 640x480...

[avolmat-st]

@avolmat-st and @josuah @iabdalkader @mjs513 and all -

I thought before I start debugging the snapshot mode, that I should probably rebase to the latest sources, as I know from previous PRs that being out of date by a while can cause issues with merging and the tests run. So I rebased this morning.

I also hand applied the DMA fix mentioned earlier. I have not yet pushed up the rebase will do soon.

But wanted to at least build it before I did.

I am now finding none of my test sketches will build, as it looks like SMH and/or SDRAM stuff changed... That is the code that I have for initializing the SMH looks like:

stm32_sdram1_section static uint8_t __aligned(32) smh_pool[4*1024*1024];

int smh_init(void) {
    int ret = 0;
    ret = shared_multi_heap_pool_init();
    if (ret != 0) {
        return ret;
    }

    struct shared_multi_heap_region smh_sdram = {
        .attr = SMH_REG_ATTR_EXTERNAL,
        .addr = (uintptr_t) smh_pool,
        .size = sizeof(smh_pool)
    };

    ret = shared_multi_heap_add(&smh_sdram, NULL);
    if (ret != 0) {
        return ret;
    }
	return 0;
}

SYS_INIT(smh_init, POST_KERNEL, CONFIG_KERNEL_INIT_PRIORITY_DEFAULT);

} // extern c

Which is/was a copy of the ArduinoCore-zephyr loader fixups code: https://github.com/arduino/ArduinoCore-zephyr/blob/main/loader/fixups.c#L109-L137

Which is now erroring as:

Files/app.dir/src/main.cpp.obj -c D:/zephyrproject/zephyr_test_sketches/camera_to_tft/src/main.cpp
D:/zephyrproject/zephyr_test_sketches/camera_to_tft/src/main.cpp:790:1: error: '__stm32_sdram1_section' does not name a type
  790 | __stm32_sdram1_section static uint8_t __aligned(32) smh_pool[4*1024*1024];
      | ^~~~~~~~~~~~~~~~~~~~~~
D:/zephyrproject/zephyr_test_sketches/camera_to_tft/src/main.cpp: In function 'int smh_init()':
D:/zephyrproject/zephyr_test_sketches/camera_to_tft/src/main.cpp:801:29: error: 'smh_pool' was not declared in this scope
  801 |         .addr = (uintptr_t) smh_pool,
      |                             ^~~~~~~~
[68/273] Building C object zephyr/CMakeFiles/zephyr.dir/subsys/shell/modules/kernel_service/thread/resume.c.obj
ninja: build stopped: subcommand failed.

Now to see what has changed.

EDIT: Looks like you ( @avolmat-st ) removed these sections a couple of days ago with the commit: 9cc26d6

Ohh, ok this is trying to create a SMH reagion from the SDRAM. Ok, indeed, this won't work ... maybe it was a bit early to remove that section ? Wasn't aware about this usage.
A possibility, could be for example to have this commit [1] applied. It is from a PR which has been sleeping for a while and has been closed recently due to inactivity, and this allow to initialize the SMH from the SDRAM. That won't replace exactly what you need thow.

[1] 7c853e8

Maybe again, for the time being might be better to stick to the previous branch you had until we see how to address that. Or you can propose something of course.

[avolmat-st]

@KurtE

Actually, on top of the latest main branch, you should be able to do something like:

Z_GENERIC_SECTION(SDRAM1) static uint8_t __aligned(32) smh_pool[410241024];
this is equivalent to
__stm32_sdram1_section static uint8_t __aligned(32) smh_pool[410241024];

assuming that the platform you run on has the zephyr,memory-region property set. Normally I've added all of them when removing those __stm32_sdram1_section.

[KurtE]

@KurtE

Actually, on top of the latest main branch, you should be able to do something like:

Z_GENERIC_SECTION(SDRAM1) static uint8_t __aligned(32) smh_pool[4_1024_1024]; this is equivalent to __stm32_sdram1_section static uint8_t __aligned(32) smh_pool[4_1024_1024];

assuming that the platform you run on has the zephyr,memory-region property set. Normally I've added all of them when removing those __stm32_sdram1_section.

I am guessing that it is not properly set... as my test sketch I am trying it build fails now with:

src/main.cpp.obj -MF CMakeFiles\app.dir\src\main.cpp.obj.d -o CMakeFiles/app.dir/src/main.cpp.obj -c D:/zephyrproject/zephyr_test_sketches/camera_to_tft/src/main.cpp
terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc

You mentioned using:
`Z_GENERIC_SECTION(SDRAM1)** static uint8_t __aligned(32) smh_pool[4_1024_1024]; this is equivalent to __stm32_sdram1_section static uint8_t __aligned(32) smh_pool[4_1024_1024];

`
with SDRAM1 but the line replaced was for SDRAM2? Is that
Do I need to try something like adding:

/ {
	sdram1: sdram@c0000000 {
		status = "okay";
		compatible = "zephyr,memory-region", "mmio-sram";
		device_type = "memory";
		reg = <0xc0000000 DT_SIZE_M(16)>;
		zephyr,memory-region = "SDRAM1";
		zephyr,memory-attr = <DT_MEM_ARM(ATTR_MPU_RAM)>;
	};
};

I tried it, it still throws that error.

Sorry, I tried this rebase as to a comment from last week, that if I were using the current stuff, I would need to integrate a DMA PR ... So I though I should try as to maybe some of the issues I was running into might have been related to other stuff like that.

EDIT: I see that the SDRAM1 is defined in the zephyr boards ...
https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/arduino/portenta_h7/arduino_portenta_h7_stm32h747xx_m7.dts#L41-L47

But: I should have double checked the changing of line:
Z_GENERIC_SECTION(SDRAM1) static uint8_t __aligned(32) smh_pool[410241024];
versus:
__stm32_sdram1_section static uint8_t __aligned(32) smh_pool[4*1024*1024];

as the browser here converted the 4 * 1024 * 1024 in your post to: 41024104 which is > 4194304
when I converted it back to the right size, it now builds.

Thanks!

[avolmat-st]

@KurtE

Oh ok understood for the rebase / dma point. Sorry, probably my comment wasn't clear. Actually the DMA issue is on the current main branch, aka you will in fact hit it if you rebase. I was pointing this just in case you might actually rebase. On code you have been using so far there were no such DMA issue which has been introduced after you start your work.

[iabdalkader]

Ohh, ok this is trying to create a SMH reagion from the SDRAM. Ok, indeed, this won't work ... maybe it was a bit early to remove that section ? Wasn't aware about this usage.

If Z_GENERIC_SECTION(SDRAM1) works that's good enough. We will have to update that when we update our Zephyr fork. At some point it might be better to move that SMH code from the application to boards/arduino/<board>/board.c.

[KurtE]

If Z_GENERIC_SECTION(SDRAM1) works that's good enough. We will have to update that when we update our Zephyr fork. At some point it might be better to move that SMH code from the application to boards/arduino/<board>/board.c.

@avolmat-st @iabdalkader @mjs513 - Next issue
DCMI device does not start.

This was due to pulling in the PR:
#95803

Why:

Our overlay files have:

&dcmi {
	status = "okay";
	zephyr,deferred-init;
	/* ext-sdram = <&sdram1>; */
	pinctrl-0 = <&dcmi_hsync_pa4 &dcmi_pixclk_pa6 &dcmi_vsync_pi5
		     &dcmi_d0_ph9 &dcmi_d1_ph10 &dcmi_d2_ph11 &dcmi_d3_ph12
		     &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_FULL>; //FULL for 7670, default FIFO_1_4

...

The problem is that the dma number changed needed to change the dmas to:

	dmas = <&dma1 1 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_FULL>; //FULL for 7670, default FIFO_1_4

[KurtE]

One idea is actually to make the snapshot mode behave same as the continuous mode, that is, whenever getting a DMA error, the error handling would simply do a Stop / Start and that's it. No data->dma_error_detected to be set etc, simply the k_fifo_get within the dequeue function will take a bit more time to come back with a frame since the capture has been restarted.

Doing this would simply a lot the code I think since there is no more the need to consider if there has been a DMA error or so.

Makes sense, I will give it a try. I thought about this at one point, but am wondering if there should then be some configuration option like, MAX_RETRY_COUNT or the like.

[avolmat-st]

@KurtE

do you confirm that the snapshot mode is now stable again, aka working for very long period of time without stopping suddenly ?
you mentioned few days ago that you had same stability issue as I also saw on my side.

[KurtE]

@avolmat-st
It appears to be working better. Partially as I am using a simplified example sketch, that outputs the frames over USB.
I have it where it can output using processing app (ships in extras on Arduino builds) or Arducam viewer (not sure if they have non-windows version) The process sketch has never worked well for me on Windows (not zephyr specific)

I have it running on both GIGA and Portenta H7... But throwing darts on how to convert the USB code from old USB CDC-ACM to the USB Next, as the current zephyr has obsoleted the other USB code. Still trying to get it to work again with the
Arduino Nicla Vision. So far USB Port not showing up. Not sure yet what would be different between it and the Portenta H7 as they both use the OTG port which support HS, whereas the GIGA uses the other USB port which only does FS. But different subject

[KurtE]

I have it running on both GIGA and Portenta H7... But throwing darts on how to convert the USB code from old USB CDC-ACM to the USB Next, as the current zephyr has obsoleted the other USB code. Still trying to get it to work again with the
Arduino Nicla Vision. So far USB Port not showing up. Not sure yet what would be different between it and the Portenta H7 as they both use the OTG port which support HS, whereas the GIGA uses the other USB port which only does FS. But different subject

@avolmat-st @pillo79 @mjs513 - Looks like the PR #92522 solves my issues trying to update to the latest USB testing issues.
At least so far for the Nicla Vision. Will probably try updating the other boards to use your updates.

I have been running the sketch that outputs to the USB Processing sketch for probalby an hour and still running. However probably not many errors with this board as it does not have any SDRAM, so using normal memory for buffers.

[avolmat-st]

Hi @KurtE,

thanks for the update. I just gave it a try. I cannot yet fully conclude on it since I am running into issues with the STM32H747I-DISCO, I guess this is specific on it and the image is very distorded, seems like if I had an issue with the circular mode of the DMA.
Anyway, in term of stability I still have a similar status as previously in that, it is very stable (run forever) in continuous mode (even if I do see errors but recovery seems to work fine), but it still stop very very rapidly if used in snapshot.

The system isn't even locked, crashed or anything, but it just doesn't grab anymore frames.

I'll try to have a look at it in addition to see that potential DMA issue (?) I have.

[avolmat-st]

I got it working by using another sensor (ov9655 which I have on a shield). In this case I do not have anymore strides on the image as before. At the same time, I do not see DMA issue recovery anymore but this is not that surprinsing since I think this sensor sends the pixel way slower than the OV5640.
I however still have the snapshot mode not reliable and stopping very very rapidly.

[JarmouniA]

However probably not many errors with this board as it does not have any SDRAM, so using normal memory for buffers.

My guess is SDRAM instability is mainly due to its location in the bus matrix

(DCMI is on AHB2، SDRAM on FMC)

[KurtE]

@avolmat-st - I found one issue with the snapshot. If I had a timeout with one buffer and during the time between the calls it completed, I was not checking to see if it was on the output queue. I did a quick and dirty for it. might move the code that checked it up front to check it later. It got one of my test apps working better. But later still get a freeze.

Note: in the morning, I will probably move this test into the snapshot if, that if NULL, don't error out if no buffer available, but one is on output. This should remove a window of time where it could fail as the test is done and right after that instant the frame complete happens.

[KurtE]

I got it working by using another sensor (ov9655 which I have on a shield). In this case I do not have anymore strides on the image as before. At the same time, I do not see DMA issue recovery anymore but this is not that surprinsing since I think this sensor sends the pixel way slower than the OV5640. I however still have the snapshot mode not reliable and stopping very very rapidly.

@avolmat-st I just updated the last commit from yesterday to fix the issue I was running into with my test app, that has a short timeout. It ran into issue where it had not completed getting a frame, but by the next time I called, it had completed it. Code did not handle
that there was a buffer already on the output queue.

As noted: the non-snapshot mode appears to be stable and recovering.

Snapshot mode is not recovering: I believe I mentioned before that I found at times doing the start and maybe stop as part of the callback could be problematic. my WAG is that when you call the start with the snapshot mode option, the API is setup to wait until it finds the start of a new frame, which in cases could be more or less waiting a whole frame, and maybe doing that in
the callback causes issues with other things happening or not happening.

Will try temporarily undoing that part of the changes and see if it improves again.

[KurtE]

@avolmat-st @josuah @JarmouniA - I pushed up some changes to the snapshot mode, that if it finds it already
has started but not received an image, it sees how long that image has been pending. I put in a hard coded
5 second timeout, which if expires, stops and then restarts. This appears to handle the cases where the snapshot
just stops.

I believe I Should not hard code, but should allow the program to set it. Not sure which was is best to do this.
Could add property to DCMI? Maybe add the runtime controls?

Should I also change to init controls for setting being in snapshot?

Thanks
Kurt

EDIT: Been running now for a few hours, >45K frames received and displayed on display.
I do get timeouts, but it recovers.

[03:42:24.909,000] <wrn> video_stm32_dcmi: Snapshot: Timed out!
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
[03:43:50.875,000] <wrn> video_stm32_dcmi: Snapshot: Timed out!
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
[03:44:12.296,000] <wrn> video_stm32_dcmi: Snapshot: Timed out!
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
ERROR: Unable to dequeue video buf
[05:12:47.326,000] <wrn> video_stm32_dcmi: Snapshot: Timed out!

Note: I think my sketch semi paused when my computer went to sleep, probably because it could not output to the USB
That is outputting data every 10 frames:

45569 10 RD: 661396 66139 WR: 2096874 209687
45579 10 RD: 661633 66163 WR: 2097455 209745
45589 10 RD: 662042 66204 WR: 2096214 209621
45599 10 RD: 661693 66169 WR: 2096815 209681
45609 10 RD: 661843 66184 WR: 2096722 209672
45619 10 RD: 661123 66112 WR: 2097720 209772

[KurtE]

@avolmat-st - I redid the timeouts code mentioned yesterday. I now do it at the end of the dequeue if it did
not receive a buffer. Also added a setting for what the timeout is in MS.

Note: I split the dequeue up into two functions. The main one checks if it is snaphot and if so calls off to different function.
Leaves the non-snapshot as very simple.

I have run both ways this morning and both are recovering from errors now. The nice this is having the setting and checking
at the end has an added ability. You can set that value very small and you have it so it only times out once.. Or you can set
it to what you really want as a timeout, and you can call with no wait and poll.

I am thinking maybe time to squash back to just two commits.

Thoughts?

[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

[KurtE]

@avolmat-st @mjs513 and all. I believe that it is working reasonably well. Both continuous and snapshot. Today I ported it back
to the Arduino version of zephyr: In particular to some of the DMA changes.

In: stm32_dma_init
Newer stuff has:

	hdma.Instance			= STM32_DMA_GET_INSTANCE(config->dma.reg,
								 config->dma.channel);

switched back to:

#if defined(CONFIG_SOC_SERIES_STM32F7X) || defined(CONFIG_SOC_SERIES_STM32H7X)
	hdma.Instance = __LL_DMA_GET_STREAM_INSTANCE(config->dma.reg,
						config->dma.channel);
#elif defined(CONFIG_SOC_SERIES_STM32L4X) || defined(CONFIG_SOC_SERIES_STM32U5X)
	hdma.Instance = __LL_DMA_GET_CHANNEL_INSTANCE(config->dma.reg, config->dma.channel);
#endif

And in the newer stuff it has:
ret = dma_config(config->dma.dma_dev, config->dma.channel, &dma_cfg);
switched back to:

	/* Because of the STREAM OFFSET, the DMA channel given here is from 1 - 8 */
	ret = dma_config(config->dma.dma_dev,
			config->dma.channel + STM32_DMA_STREAM_OFFSET, &dma_cfg);

With these changes I am able to read in frames and display them on the GIGA display shield. Without these changes
it would die after maybe a few frames.

With them it has run for a long period of time. Currently running with 3 frame buffers (no snapshot)
I am getting a lot of the DMA errors, but it is recovering and continuing.

[erwango]

@KurtE About the dma changes. What is the dts dma configuraiton you're using ?

[KurtE]

@KurtE About the dma changes. What is the dts dma configuraiton you're using ?

Sorry, not sure what exactly you are asking, but will guess:

With the Arduino build which is based off of zephyr v4.2.0 on the Arduino Giga board the DMA on the dcmi is configured:

&dcmi {
	status = "okay";
	zephyr,deferred-init;
	/* 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>;
		};
	};
};

Since v4.2.0 released, the DCMI DMA code (video_stm32_dcmi) has changed in a couple of PRs.
Your commit: rivers: stm32: Keep DMA stream offset handling internal to driver
Which removed the offset:

	ret = dma_config(config->dma.dma_dev,
			config->dma.channel + STM32_DMA_STREAM_OFFSET, &dma_cfg);

And your commit: drivers: stm32: Make use of new GET_INSTANCE DMA macro
Which did not exist when 4.2 was released, so I to run on it on 4.2 I had to remove it (in my ArduinoCore Zephyr tests.

On Current stuff: I am using:

	dmas = <&dma1 1 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>;

And pulled in the PR:
#95803

Note: I tried on my testing of camera code back on ArduinoCore-zephyr tried to pull in the updated version (minus new macro) and neither the camera nor display worked properly. Not sure it interferred with the Display driver. Don't know how to test
the display natively on Zephyr yet.