Usability tweaks: allow query of menu controls & value comparisons

[donkeyteethUX]

This would allow me to remove some hacks in my toy webcam app: https://github.com/donkeyteethUX/kcam/pull/9

[donkeyteethUX]

ping @raymanfx

[raymanfx]

I‘m still on vacation, but will take a look at this once I‘m back next week.

[MarijnS95]

This change looks good to me, and I opened #61 to fix clippy lints separately so that this PR can focus on just the semantic changes.

A couple questions/followups:

1. Should this path also handle Type::IntegerMenu?
2. Is the assumption of loading Integer and Integer64 (and Boolean) from the same value64 field still valid on big-endian architectures (and following that, should Menu/IntegerMenu be treated as a 32-bit or 64-bit value)?
3. Could fn control() take a &control::Description to save on an ioctl when reading values directly from query_controls()? All the function needs is the typ and id. Original functionality can be retained in fn control_from_id() or something.

[MarijnS95]

In addition, I changed examples/device.rs locally to test this on a random camera that emits Menu values:

     for ctrl in controls {
-        println!(
-            "{:indent$} : [{}, {}]",
-            ctrl.name,
-            ctrl.minimum,
-            ctrl.maximum,
-            indent = max_name_len
-        );
+        if ctrl.typ == Type::CtrlClass {
+            println!();
+            println!("{}", ctrl.name);
+            println!();
+        } else {
+            let value = dev.control(&ctrl)?;
+            println!(
+                "\t{:indent$} : [{}, {}] = {:?}",
+                ctrl.name,
+                ctrl.minimum,
+                ctrl.maximum,
+                value.value,
+                indent = max_name_len
+            );
+        }
     }

This shows the actual values, and groups the controls by CtrlClass (which would otherwise return -EPERM as a group doesn't have a value).

[raymanfx]

Please include the clippy fixes in this PR. A pull request should be self-contained (atomic).

[MarijnS95]

@raymanfx Of course, I cannot fix the clippy issues for the PartialEq that has been added in this PR. I can, however, fix the clippy lints for the sole PartialEq that was already in the repository and is now failing the CI since bumping (implicitly) from Rust 1.62 to 1.63.

In other words, this still requires manual intervention by @donkeyteethUX after #61 is merged.

[donkeyteethUX]

I rebased on top of #61 and would now expect CI to pass.

[MarijnS95]

@donkeyteethUX Any comment on the points I mentioned above?

[donkeyteethUX]

@donkeyteethUX Any comment on the points I mentioned above?

(1) It seems like IntegerMenu could be handled the same way, but I haven't tried it.

Fundamentally it seems like there is an implied mapping between the variants of control::Value <=> control::Type. It's not obvious to me what this mapping is for all cases, but it seems like it should be explicit somewhere.

(2) I don't know about this, but seems out-of-scope here?

(3) It makes sense to me to split the existing control() function into 2 parts. Maybe another PR could add fn control_from_descriptor (and keep the current function to avoid a breaking change).

[raymanfx]

2. Is the assumption of loading Integer and Integer64 (and Boolean) from the same value64 field still valid on big-endian architectures (and following that, should Menu/IntegerMenu be treated as a 32-bit or 64-bit value)?

This crate has not been tested on big-endian so far AFAIK, so I don't think we have to worry about it for now.

3. Could fn control() take a &control::Description to save on an ioctl when reading values directly from query_controls()? All the function needs is the typ and id. Original functionality can be retained in fn control_from_id() or something.

I've been thinking of changing the API for this in the future. Currently, the name control() implies a getter method, which is only half-true. It really invokes an IOCTL, so we might actually want to name it read_control() and write_control() or something along those lines. This is stuff for another PR though.

Fundamentally it seems like there is an implied mapping between the variants of control::Value <=> control::Type. It's not obvious to me what this mapping is for all cases, but it seems like it should be explicit somewhere.

I agree that the current API has issues in that regard. It is clear why the C API would require a dedicated type field for each control, since the actual control value is untyped in the V4L2 API. We can probably get away with just dropping the Type enum and the corresponding field entirely, since we only exposed typed control values.

[MarijnS95]

This crate has not been tested on big-endian so far AFAIK, so I don't think we have to worry about it for now.

Any more areas where this might be an issue? I wouldn't want it to "compile just fine" and then run into such obscure issues especially if this is the only area where it matters. Then again I won't be able to test it either :grimacing:

I've been thinking of changing the API for this in the future. Currently, the name control() implies a getter method, which is only half-true. It really invokes an IOCTL, so we might actually want to name it read_control() and write_control() or something along those lines. This is stuff for another PR though.

Sounds good. Rust recommends against the get_ prefix anyway so a read_ is preferable, and change set_ to write_. Obviously for another PR, as is any other unrelated request.

I agree that the current API has issues in that regard. It is clear why the C API would require a dedicated type field for each control, since the actual control value is untyped in the V4L2 API. We can probably get away with just dropping the Type enum and the corresponding field entirely, since we only exposed typed control values.

The Description represents the Type without holding the current Value. However, we could wrap minimum/maximum/default in Value?

[raymanfx]

This crate has not been tested on big-endian so far AFAIK, so I don't think we have to worry about it for now.

Any more areas where this might be an issue? I wouldn't want it to "compile just fine" and then run into such obscure issues especially if this is the only area where it matters. Then again I won't be able to test it either 😬

Nope, but I really haven't properly thought about big-endian support so far. If none of us has a platform that needs it, why bother? People who require this can do this work in the future or sponsor someone else to do so I think.

I agree that the current API has issues in that regard. It is clear why the C API would require a dedicated type field for each control, since the actual control value is untyped in the V4L2 API. We can probably get away with just dropping the Type enum and the corresponding field entirely, since we only exposed typed control values.

The Description represents the Type without holding the current Value. However, we could wrap minimum/maximum/default in Value?

Another way could be to provide a "default" value for the control in the Description. In the V4L2 API, this would invoke two ioctls each time a description is queried: the actual VIDIOC_QUERY one and the VIDIOC_G_EXT one. Since this is an operation that is not usually called in tight loops but once on program initialization, this might be a price worth paying for the improved UX.

[MarijnS95]

Nope, but I really haven't properly thought about big-endian support so far. If none of us has a platform that needs it, why bother? People who require this can do this work in the future or sponsor someone else to do so I think.

The point is, however, that I'd like to actively like to avoid doing the wrong thing if we can; but since it doesn't seem anyone found docs on whether to use value64 this isn't something we can prevent right now.

Another way could be to provide a "default" value for the control in the Description. In the V4L2 API, this would invoke two ioctls each time a description is queried: the actual VIDIOC_QUERY one and the VIDIOC_G_EXT one. Since this is an operation that is not usually called in tight loops but once on program initialization, this might be a price worth paying for the improved UX.

VIDIOC_QUERY_EXT_CTRL used in query_controls() already seems to provide that information.