Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further improval of controls #42

Merged
merged 47 commits into from
May 22, 2023

Conversation

otaku42
Copy link
Contributor

@otaku42 otaku42 commented Apr 23, 2023

Hi @tiagocoutinho . I'm still fairly new to Git and collaboration here on Github, so I thought I'd ask you for your input.

I've worked a bit to further improve controls (see this Draft PR), which happened in the improve_controls branch in otaku42/v4l2py, and I would like to contribute that work upstream.

While I did my stuff, you have committed a nice bunch of changes. This results in improve_controls being 35 commits ahead, 65 commits behind of your master branch. Merging is not easily possible due to merge conflicts that need to be resolved. So far, so good - but what is the best way to do this?

The "sync fork" tool on Github would discard my 35 commits "to make [the improve_controls] branch match the upstream repository". Although it just might be the explanation what happens during a merge anyway, I don't feel comfortable with trying what happens next, as I don't want to mess the repository/branch.

I'd rather resort to merging things locally and then pushing the results to Github, and I see the following options for that:

  1. otaku42/4l2py:master is tracking tiagocoutinho/v4l2py:master. I could merge the result of your 65 commits from otaku42/4l2py:master into otaku42/v4l2py:improve_controls, then send the PR. If you accept the PR, the changes from my improve_controls branch will go into tiagocoutinho/v4l2py:master, from where they will then be synced into otaku42/v4l2py:master.
  2. Similar to 1., but merge to otaku42/v4l2py:improve_controls from tiagocoutinho/v4l2py:master rather than from otaku42/v4l2py:master; code-wise the result would be the same, but I'm not sure if it makes a difference on the Git/Github level - or if it is actually even possible.
  3. Merge otaku42/v4l2py:improve_controls into otak42/v4l2py:master, and create the PR from there. In contrast to 1, otaku42/v4l2py:master would be ahead of tiagocoutinho/v4l2py:master until you accept and merge the PR.

Assuming that you're willing to accept the changes, which of these options would you consider to be the best way to contribute them? Or are there other, better options that I've missed? I'd appreciate any suggestion.

These two methods were not well-thought. While a control that has the
read-only flag set is certainly not writeable, a control with the
write-only flag set could still be unwriteable because of being inactive.

The original intention for these two methods was to check for the flags,
so adjust their behaviour and naming accordingly.
Before 000696e, a control was assumed (and reported by this method) to be
writeable when the read-only was not set. However, such a control would
still be unwriteable when being flagged as inactive, which was not taken
into account before.

In other words, is_writeable now checks that a control can *actually* be
written to.
This will be used later to facilitate instantiation of a Controls object with diverse Control sub-classes.
This commit covers the part up to LegacyControl, as discussed in #15.
The upgrade path for existing code is:

Use LegacyControl instead of Control for instantiations, and use either
BaseControl or BaseSingleControl for isinstance() checks (see Controls).
is_<flag> is working for many of the flags, but is weird for stuff like
V4L2_CTRL_FLAG_HAS_PAYLOAD or V4L2_CTRL_FLAG_UPDATE. Switching to
is_flagged_<flag> to check if a flag is set, e.g. is_flagged_has_payload,
is still not perfect, but better.
…l doesn't have the attributes minimum and maximum.
…tiation of different control class objects without a messy if/elif construct.
This class inherits from UserDict and thus can be used like a dict to access
the items defined for the menu. For the sake of this dict-like behaviour the
MenuItem class is not used in MenuControl; the names of the menu items will
be transformed straight to either string or integer, depending on the menu
type.

MenuItem is kept for now, but renamed to LegacyMenuItem to signal it should
not be used except for legacy code.
This will most probably be the last patch in the current series of the
improve_controls branch for now. This series closes #15 and closes #12.
@otaku42
Copy link
Contributor Author

otaku42 commented Apr 23, 2023

It turns out that my option 2 matches what the Pro Git book describes being "the GitHub workflow". I think I'll try that path and see where it ends :-)

v4l2py/device.py Outdated Show resolved Hide resolved
v4l2py/device.py Outdated Show resolved Hide resolved
…egacyControl for control types not found in ctrl_type_map.
the rest to the new derived class BaseMonoControl. The latter is more
or less a reprise of BaseSingleControl, which had been removed in
5640d4e. It's similar but not identical, and used to prepare for the
upcoming ButtonControl.
no real device that provides a button control and trying vivid did
not work (see #17).
@otaku42 otaku42 marked this pull request as ready for review May 2, 2023 22:45
@otaku42
Copy link
Contributor Author

otaku42 commented May 2, 2023

Ok, with a9d5f3a I think I now have covered everything I originally wanted to have in this PR.

To summarize, the idea is to provide a more pythonic API for working with a device's controls. See otaku42#12 for the initial motivation, and otaku42#15 for some kind of rationale. A basic comparison of the old vs. the new API is given in README.md, and with v4l2py-ctl.py there is an example of how to use the new API.

Please let me know if you have any concerns, suggestions or questions.

@tiagocoutinho
Copy link
Owner

Thanks for these awesome improvements @otaku42! 🤩

I tried the examples/web with your branch but they fail with:

jinja2.exceptions.UndefinedError: 'v4l2py.device.IntegerControl object' has no attribute 'info'

would it be possible to include a fix? Thanks in advance

@otaku42
Copy link
Contributor Author

otaku42 commented May 16, 2023

jinja2.exceptions.UndefinedError: 'v4l2py.device.IntegerControl object' has no attribute 'info'

Oops. Thanks for pointing that out, I'll fix it.

I didn't test the examples by now, to be honest, but was about to do that soon. I didn't expect they make use of the controls API.

The info-attribute of controls has been renamed to _info, to signal that while it can be accessed it's on ones own risk. Sub-attributes of info are made available as top-level attributes if and where it makes sense. For example, minimum or maximum may or may not be available depending on the control type - see otaku42#12, which is what triggered this whole controls improvement thing :-)

@otaku42
Copy link
Contributor Author

otaku42 commented May 19, 2023

I've taken the lazy approach to fix the issue you have reported; see a98df12 .

While working on it, I came across some issues:

and had some ideas for slight enhancements for examples/web/async.py :

I didn't want to cram these things into the PR, but I'd try to fix them at a later time, after this PR has been merged.

@tiagocoutinho
Copy link
Owner

Thanks for the fix and the extra work to keep example working with python 3.7.
I really appreciate you being so thorough. I agree the issues you point out and the proposal for improvements should be addressed in other issues/PRs

It is an honor to accept your PR

@tiagocoutinho tiagocoutinho self-requested a review May 22, 2023 06:49
Copy link
Owner

@tiagocoutinho tiagocoutinho left a comment

Choose a reason for hiding this comment

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

LGTM! Merging

@tiagocoutinho tiagocoutinho merged commit 429f340 into tiagocoutinho:master May 22, 2023
5 checks passed
@otaku42
Copy link
Contributor Author

otaku42 commented May 22, 2023

It is an honor to accept your PR

It's my pleasure, and I'm glad that I can contribute a tiny bit to this great project. Thanks for the great work - and for accepting my contributions :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants