Avoid extra ioctl by allowing control query by description #62

donkeyteethUX · 2022-09-15T20:07:43Z

Changes based on discussion on #60, specifically comments from @MarijnS95.

Add get_control which takes a &Description and requires only 1 ioctl.
Deprecate control getter in favor of more explicit get_control_from_id (and document preference for get_control).
Bump minor version.

Cargo.toml

src/device.rs

raymanfx · 2022-09-16T12:17:07Z

I am not entirely convinced increasing the API surface is worth it just to avoid a single ioctl. This might very well be a case of premature optimization. Are you running into some actual issues that motivate this change?

MarijnS95 · 2022-09-16T12:59:03Z

I am not entirely convinced increasing the API surface is worth it just to avoid a single ioctl. This might very well be a case of premature optimization. Are you running into some actual issues that motivate this change?

I only suggested this to be able to pass an &Description instead of an untyped u32 which seemed a bit more clear to me. It's not premature optimization as much as it is removing unnecessary code that shouldn't have run/been there in the first place, IMO.

donkeyteethUX · 2022-09-16T19:16:57Z

@raymanfx agreed that it's not a huge difference. Still, my app calls control() for each available control inside an egui render loop. This might seem excessive, but it's simpler and safer to keep the driver as the single source of truth rather than caching anything. strace shows around 1000 ioctl / second before, and 600 after, and the code is slightly more explicit as @MarijnS95 points out.

src/device.rs

Cargo.toml

src/device.rs

remove old control getter

donkeyteethUX · 2023-01-22T04:24:29Z

Bump, after addressing old comment & rebasing this.

src/device.rs

MarijnS95 · 2023-02-03T13:55:37Z

@donkeyteethUX reverse bump: this is close to ready if you address the minor but remaining isuses.

donkeyteethUX · 2023-02-03T18:36:52Z

@donkeyteethUX reverse bump: this is close to ready if you address the minor but remaining isuses.

Thanks -- will-do when I get a chance. I'm swamped at work but haven't forgotten about it.

raymanfx · 2023-02-07T16:17:36Z

Looks good to me. Any objections?

MarijnS95

I'm not fond of conflating PRs with unrelated new-Rust-version clippy fixes as it takes away from the main semantic change, but will leave it to @raymanfx to decide to split that out to a separate PR (i.e. do we use rebase merges?).

EDIT: fwiw I've been told inlining format args will be demoted to pedantic.

src/device.rs

MarijnS95 · 2023-03-01T14:06:12Z

Ping again, we still have two comments open and shouldn't conflate this PR with unrelated clippy changes.

This reverts commit 8ba7d19.

raymanfx · 2023-11-05T18:45:08Z

src/device.rs

+    /// # Arguments
+    ///
+    /// * `id` - Control identifier
+    pub fn control_from_id(&self, id: u32) -> io::Result<Control> {


IMO, this can be removed. We don't need backwards compatibility here, no need to inflate the API surface when the new function does the same thing, but better.

Agreed if we're pushing to land this in a breaking release.

EDIT: Which is needed anyway because the signature of the existing fn control() changed rather than leaving it untouched and introducing a new fn control_from_description() instead. We probably discussed this a while ago but I forgot.

The new implementation of `control()` takes a `&Description` directly, and callers have enough means to get that `Description` such that we shouldn't cater to the "slower" use-case of letting `libv4l-rs` query it again.

raymanfx

Looks good to me. I still think the API is a touch too close to the raw C v4l2 API, but we can do iterative improvements. It's something I need to think about and sketch out separately.

MarijnS95 · 2023-11-09T13:01:50Z

@raymanfx do you mean the API added by this PR specifically, or the control API in general? If anything it seems this function is now more high-level by taking something typed instead of a generic i32.

raymanfx · 2023-11-09T13:02:49Z

I was talking about the API in general. This change is fine and I agree with its premises.

MarijnS95 requested changes Sep 15, 2022

View reviewed changes

donkeyteethUX force-pushed the master branch from 50859ac to 2acb899 Compare September 19, 2022 03:46

raymanfx requested changes Sep 23, 2022

View reviewed changes

src/device.rs Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

src/device.rs Outdated Show resolved Hide resolved

refactor to avoid extra ioctl

f3c04f3

remove old control getter

donkeyteethUX force-pushed the master branch from 2acb899 to f3c04f3 Compare January 22, 2023 04:19

donkeyteethUX requested a review from raymanfx January 22, 2023 04:21

MarijnS95 requested changes Jan 22, 2023

View reviewed changes

src/device.rs Outdated Show resolved Hide resolved

src/device.rs Outdated Show resolved Hide resolved

src/device.rs Outdated Show resolved Hide resolved

fix doc link and clippy lint

f575bb5

donkeyteethUX requested review from MarijnS95 and removed request for raymanfx February 7, 2023 07:54

donkeyteethUX force-pushed the master branch from befbde4 to 0d6bb47 Compare February 7, 2023 08:24

inline format args to please our pedantic overlords

8ba7d19

donkeyteethUX force-pushed the master branch from 0d6bb47 to 8ba7d19 Compare February 7, 2023 08:49

MarijnS95 reviewed Feb 7, 2023

View reviewed changes

src/device.rs Outdated Show resolved Hide resolved

MarijnS95 added 2 commits November 1, 2023 11:22

Add missing backticks around intradoc link

13cbcb1

Revert "inline format args to please our pedantic overlords"

85f1e89

This reverts commit 8ba7d19.

MarijnS95 approved these changes Nov 1, 2023

View reviewed changes

MarijnS95 requested a review from raymanfx November 1, 2023 10:24

raymanfx reviewed Nov 5, 2023

View reviewed changes

Drop unneeded control_from_id()

08db864

The new implementation of `control()` takes a `&Description` directly, and callers have enough means to get that `Description` such that we shouldn't cater to the "slower" use-case of letting `libv4l-rs` query it again.

MarijnS95 requested a review from raymanfx November 5, 2023 23:21

raymanfx approved these changes Nov 9, 2023

View reviewed changes

raymanfx merged commit dfbb778 into raymanfx:master Nov 10, 2023
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid extra ioctl by allowing control query by description #62

Avoid extra ioctl by allowing control query by description #62

donkeyteethUX commented Sep 15, 2022

raymanfx commented Sep 16, 2022

MarijnS95 commented Sep 16, 2022

donkeyteethUX commented Sep 16, 2022

donkeyteethUX commented Jan 22, 2023

MarijnS95 commented Feb 3, 2023

donkeyteethUX commented Feb 3, 2023

raymanfx commented Feb 7, 2023

MarijnS95 left a comment •

edited

Loading

MarijnS95 commented Mar 1, 2023

raymanfx Nov 5, 2023

MarijnS95 Nov 5, 2023 •

edited

Loading

raymanfx left a comment

MarijnS95 commented Nov 9, 2023

raymanfx commented Nov 9, 2023

Avoid extra ioctl by allowing control query by description #62

Avoid extra ioctl by allowing control query by description #62

Conversation

donkeyteethUX commented Sep 15, 2022

raymanfx commented Sep 16, 2022

MarijnS95 commented Sep 16, 2022

donkeyteethUX commented Sep 16, 2022

donkeyteethUX commented Jan 22, 2023

MarijnS95 commented Feb 3, 2023

donkeyteethUX commented Feb 3, 2023

raymanfx commented Feb 7, 2023

MarijnS95 left a comment • edited Loading

Choose a reason for hiding this comment

MarijnS95 commented Mar 1, 2023

raymanfx Nov 5, 2023

Choose a reason for hiding this comment

MarijnS95 Nov 5, 2023 • edited Loading

Choose a reason for hiding this comment

raymanfx left a comment

Choose a reason for hiding this comment

MarijnS95 commented Nov 9, 2023

raymanfx commented Nov 9, 2023

MarijnS95 left a comment •

edited

Loading

MarijnS95 Nov 5, 2023 •

edited

Loading