drivers: video: Add the video shell #88566

josuah · 2025-04-14T01:11:27Z

Dependencies:

Implement video control framework #82158

Replaces:

drivers: video: shell: initial implementation #82393

Now that we have a nice shell control querying API, it is possible to tab-complete in-depth every control value.

video - Video driver commands
Subcommands:
  start    : Start a video device and its sources
             Usage: video start <device>
  stop     : Stop a video device and its sources
             Usage: video stop <device>
  capture  : Capture a given number of frames from a device
             Usage: video capture <device> <num-frames>
  format   : Query or set the video format of a device
             Usage: video format <device> <ep> [<fourcc> <width>x<height>]
  frmival  : Query or set the video frame rate/interval of a device
             Usage: video frmival <device> <ep> [<n>fps|<n>ms|<n>us]
  ctrl     : Query or set video controls of a device
             Usage: video ctrl <device> [<ctrl> <value>]

To test on native_sim with VIDEO_SW_GENERATOR:

west build -t run -b native_sim/native/64 samples/drivers/video/capture -- -DCONFIG_VIDEO_SHELL=y

On another terminal, using your favorite serial console client, and likely COM# on Windows instead of /dev/pts/3:

picocom -b 115200 /dev/pts/3

josuah · 2025-04-14T01:13:07Z

Because of the limitation of the shell API, a hack was used using LISTIFY() to generate completion functions that have an extra argument (i.e. the device number), so that the tab completion results are aware of the beginning of the line.

This is not expected to take more RAM, only more ROM/Flash.

josuah · 2025-05-11T12:31:01Z

Force-push:

Deploy a fix for CI's ERROR - error: XDG_RUNTIME_DIR not set in the environment. error

ngphibang · 2025-05-09T22:54:18Z

drivers/video/video_shell.c

+#include <stdlib.h>
+#include <stdio.h>
+#include <ctype.h>
+
+#include <zephyr/device.h>
+#include <zephyr/drivers/video.h>
+#include <zephyr/drivers/video-controls.h>
+#include <zephyr/sys/byteorder.h>
+#include <zephyr/shell/shell.h>


nit: better in alphabetic order

Thank you, I keep forgetting...

ngphibang · 2025-05-11T16:17:39Z

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

+	if (IS_ENABLED(CONFIG_TEST)) {
+		ctrl.id = VIDEO_CID_TEST_PATTERN;
+		video_set_ctrl(video_dev, &ctrl);
+	}


This is unrelated to the video shell change. While it is better to use if instead of #ifdef here, this should be in a separate commit so that people will not be confused when reading commit history.

Good point, moved and also converted the other in the process.

ngphibang · 2025-05-11T16:34:11Z

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

+	if (IS_ENABLED(CONFIG_VIDEO_SHELL)) {
+		LOG_INF("Letting the user control the device with the video shell");
+		return 0;
+	}


I think we should return as soon as possible, right at the beginning of the main function.

By doing this here, the sample already reserved some video buffers for nothing:

struct video_buffer *buffers[CONFIG_VIDEO_BUFFER_POOL_NUM_MAX], *vbuf;

Users can be very fast (or if something is stuck in the sample) doing something already in the shell before this point, so we waste a significant amount of memory and not enough memory for the shell.

In fact, the idea is we can use video shell not only on the video device in the sample but also for other video devices (e.g. sw generator). And all the logs information in the sample up to this point (format, framerate, controls) are not needed as user can get them with the shell commands ?

Good point, the video shell already offers a way to print all of the early main content, and the main loop as well.

I moved the if (IS_ENABLED(CONFIG_VIDEO_SHELL)) all the way up. Easy to see that nothing is required for the shell to work that way.

Currently, all the sample main loop does is to just log the buffer fields and stays at that:

LOG_DBG("Got frame %u! size: %u; timestamp %u ms", frame++, vbuf->bytesused, vbuf->timestamp);

And the video shell actually does not do it. Should I add buffer timestamps to the shell capture command?
The basic logging of the shell sample can still be useful for troubleshooting when "debug" logs are enabled.

https://github.com/zephyrproject-rtos/zephyr/compare/e992d709d11230d26ea32e5844d289441eda2eef..3e707557fb8eb7486310b024f5c663862d9ce3d5

Yes, I think

ngphibang

VIDEO_EP_ is dropped but I see that the PR still uses it and passes CI (?). That means there is no built-only test cover the video shell. I don't know if we could add a built-only test for it ?

PS : It seems we just need to re-trigger the CI

ngphibang · 2025-05-16T21:20:43Z

drivers/video/video_shell.c

+	struct video_format fmt = {0};
+	struct video_buffer *buffers[CONFIG_VIDEO_BUFFER_POOL_NUM_MAX] = {NULL};
+	struct video_buffer *vbuf = NULL;
+	char *arg2 = argv[2];


As we make use of a local variable here for argv[2], is it better to use a more descriptive name to know what it is ?

ngphibang · 2025-05-16T21:21:45Z

drivers/video/video_shell.c

+				   enum video_endpoint_id ep, size_t argc, char **argv)
+{
+	struct video_frmival frmival = {0};
+	char *arg0 = argv[0];


ngphibang · 2025-05-16T21:22:10Z

drivers/video/video_shell.c

+				  enum video_endpoint_id ep, size_t argc, char **argv)
+{
+	struct video_format fmt = {0};
+	char *arg1 = argv[1];


ngphibang · 2025-05-16T21:22:35Z

drivers/video/video_shell.c

+	struct video_ctrl_query cq = {0};
+	struct video_control ctrl;
+	char menu_name[CONFIG_VIDEO_SHELL_CTRL_NAME_SIZE];
+	char *arg1 = argv[1];


ngphibang · 2025-05-16T21:26:53Z

drivers/video/video_shell.c

+		    argv[0], ctrl.id, argv[1], ctrl.val64);
+
+	if (cq.type != VIDEO_CTRL_TYPE_INTEGER64) {
+		ctrl.val = ctrl.val64;


This is not needed as val and val64 are in the same union so they point to the same memory address ?

I have no problem with removing it, but is that also a no-op for big-endian architectures?

[EDIT: newer approach avoids the problem altogether]

Ok, I see now the concern for BE / LE. In fact, it's related to the implementation earlier, if val and val64 are used "normally", this is not a concern (?). It does not matter to me, no need to change if you prefer to do it this way.

I went for something with 1 more line of code but more straightforward.
In hope that makes sense.
Thank you for the feedback.

ngphibang · 2025-05-16T21:33:09Z

drivers/video/video_shell.c

+	case VIDEO_CTRL_TYPE_BOOLEAN:
+		if (strcmp(argv[1], "on") == 0) {
+			ctrl.val64 = true;
+		} else if (strcmp(argv[1], "off") == 0) {
+			ctrl.val64 = false;
+		} else {
+			shell_error(sh, "Video control value must be 'on' or 'off', not '%s'",
+				    argv[1]);
+			return -EINVAL;
+		}


Is it simpler to just use 1 and 0 interger instead of "on" and "off" string as v4l2-ctl ? Query-ctrl command can show the min, max, def, cur values so users will not confuse.

Having "on" and "off" permitted the user to discover the type is bool with just 'tab'.
I can offer completion candidates for only 0 and 1 then?

I see, so it's ok as currently then.

I also added "0" and "1" as possible input values even though not tab completed. I can also add them as tab completion candidates, very easy.

ngphibang · 2025-05-16T21:35:14Z

drivers/video/video_shell.c

+		for (i = 0; cq.menu[i] != NULL; i++) {
+			video_shell_convert_ctrl_name(cq.menu[i], menu_name, sizeof(menu_name));
+			if (strcmp(menu_name, argv[1]) == 0) {
+				ctrl.val64 = i;


type menu value is integer, so it should be ctrl.val (though they can be used interchangeably) ?

I did set everything to ctrl.val64 so that strtoull() could be used for every type:

ctrl.val64 = strtoull(arg1, &arg1, 10);

But that is confusing, so let's get back to something more readable.

It's not confusing, I see the intention, just a bit unusual where I see ctrl.val = ctrl.val64 as commented above. Keep the implementation as is if you see it's more convenient.

ngphibang · 2025-05-16T21:40:13Z

drivers/video/video_shell.c

+/* Current video endpoint, used for tab completion */
+static enum video_endpoint_id video_shell_ep = VIDEO_EP_NONE;


VIDEO_EP_ is dropped so this need to be rebased. Idem for others.

josuah · 2025-05-18T12:12:26Z

drivers/video/video_shell.c

+
+	num_buffers = strtoull(arg2, &arg2, 10);
+	if (*arg2 != '\0') {
+		shell_error(sh, "Invalid integer '%s' for this type", argv[1]);


This should have been argv[2].

josuah · 2025-05-18T15:44:12Z

drivers/video/video_shell.c

+		    argv[0], ctrl.id, argv[1], ctrl.val64);
+
+	if (cq.type != VIDEO_CTRL_TYPE_INTEGER64) {
+		ctrl.val = ctrl.val64;


I went for something with 1 more line of code but more straightforward.
In hope that makes sense.
Thank you for the feedback.

josuah · 2025-05-18T15:46:34Z

drivers/video/video_shell.c

+	case VIDEO_CTRL_TYPE_BOOLEAN:
+		if (strcmp(argv[1], "on") == 0) {
+			ctrl.val64 = true;
+		} else if (strcmp(argv[1], "off") == 0) {
+			ctrl.val64 = false;
+		} else {
+			shell_error(sh, "Video control value must be 'on' or 'off', not '%s'",
+				    argv[1]);
+			return -EINVAL;
+		}


I also added "0" and "1" as possible input values even though not tab completed. I can also add them as tab completion candidates, very easy.

josuah · 2025-05-18T15:47:32Z

Force-push:

Address silent SCA warnings
Set vbuf->type before dequeuing
strtoull() -> strtoll()
Move for a more straightforward way to assing ctrl.val/ctrl.val64
Drop VIDEO_EP_###
Allow "0" or "1" to be used instead of "on" or "off"

Tested again each command against the sw_generator using the patch soup:

git remote add josuah https://github.com/josuah/zephyr
git fetch josuah
git cherry-pick josuah/pr-video-sw-generator~13..josuah/pr-video-sw-generator
west build -b native_sim/native/64 samples/drivers/video/capture -- -DCONFIG_VIDEO_SHELL=y
west flash

Then on another terminal, replacing /dev/pts/0 by what is printed by the sample:

picocom /dev/pts/0

sonarqubecloud · 2025-05-18T16:23:14Z

Quality Gate passed

Issues
4 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

Introduce the video shell and implement some video shell commands. Make use of the various querying API to implement tab-completion, and validiate the data, as well as convert string names into integers. Commands provided: frmival, format, ctrl, start, stop, capture Signed-off-by: Josuah Demangeon <me@josuah.net>

Modify the capture sample to only run the video shell when the user enables it with -DCONFIG_VIDEO_SHELL=y, as an alternative way to trigger captures. Signed-off-by: Josuah Demangeon <me@josuah.net>

Convert #ifdef CONFIG_TEST into if (IS_ENABLED(CONFIG_TEST)) to improve readability. Signed-off-by: Josuah Demangeon <me@josuah.net>

josuah added area: Drivers area: Shell Shell subsystem area: Video Video subsystem labels Apr 14, 2025

josuah requested a review from ngphibang April 14, 2025 01:11

josuah marked this pull request as draft April 14, 2025 01:11

josuah requested a review from loicpoulain April 14, 2025 01:11

github-actions bot added area: Build System platform: NXP Drivers NXP Semiconductors, drivers platform: ESP32 Espressif ESP32 platform: STM32 ST Micro STM32 area: Samples Samples labels Apr 14, 2025

github-actions bot requested review from 57300, dbaluta, decsny, dleach02, jeremybettis, kartben, manuargue, mmahadevan108, nashif, nordicjm, Raymond0225, sylvioalves and tejlmand April 14, 2025 01:12

github-actions bot assigned dleach02 and mmahadevan108 Apr 14, 2025

josuah force-pushed the pr-video-shell branch 3 times, most recently from 11760ae to d34be47 Compare April 14, 2025 01:28

josuah closed this May 10, 2025

josuah reopened this May 10, 2025

josuah force-pushed the pr-video-shell branch 2 times, most recently from b064217 to de4e547 Compare May 11, 2025 12:30

ngphibang reviewed May 11, 2025

View reviewed changes

josuah force-pushed the pr-video-shell branch 2 times, most recently from e992d70 to 1f5c5b0 Compare May 11, 2025 19:51

josuah marked this pull request as draft May 11, 2025 21:07

josuah force-pushed the pr-video-shell branch from 1f5c5b0 to c09d7b1 Compare May 11, 2025 21:07

josuah marked this pull request as ready for review May 11, 2025 22:13

josuah force-pushed the pr-video-shell branch from c09d7b1 to 6245c83 Compare May 11, 2025 22:15

ngphibang reviewed May 16, 2025

View reviewed changes

josuah force-pushed the pr-video-shell branch 5 times, most recently from 8582c26 to 0d41a44 Compare May 18, 2025 15:38

josuah commented May 18, 2025

View reviewed changes

josuah added 3 commits May 18, 2025 17:38

sample: drivers: video: capture: add support for the video shell

33c4c86

Modify the capture sample to only run the video shell when the user enables it with -DCONFIG_VIDEO_SHELL=y, as an alternative way to trigger captures. Signed-off-by: Josuah Demangeon <me@josuah.net>

samples: drivers: video: capture: convert #ifdef into if()

0d41a44

Convert #ifdef CONFIG_TEST into if (IS_ENABLED(CONFIG_TEST)) to improve readability. Signed-off-by: Josuah Demangeon <me@josuah.net>

ngphibang approved these changes May 18, 2025

View reviewed changes

josuah requested a review from kartben May 19, 2025 12:29

dleach02 approved these changes May 20, 2025

View reviewed changes

kartben approved these changes May 20, 2025

View reviewed changes

kartben merged commit 8248854 into zephyrproject-rtos:main May 20, 2025
26 checks passed

josuah mentioned this pull request May 22, 2025

drivers: video: CONFIG_VIDEO_SW_GENERATOR -> zephyr,video-sw-generator #88780

Merged

		/* Current video endpoint, used for tab completion */
		static enum video_endpoint_id video_shell_ep = VIDEO_EP_NONE;

drivers: video: Add the video shell #88566

drivers: video: Add the video shell #88566

Uh oh!

Conversation

josuah commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josuah commented Apr 14, 2025

Uh oh!

josuah commented May 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngphibang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josuah May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josuah commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented May 18, 2025

Quality Gate passed

Uh oh!

Uh oh!

josuah commented Apr 14, 2025 •

edited

Loading

ngphibang left a comment •

edited

Loading

josuah May 18, 2025 •

edited

Loading

josuah commented May 18, 2025 •

edited

Loading