Repository navigation
fix(v4l2): fix str, rect and bitmask logging - #367
Conversation
Signed-off-by: Patrick Gehrsitz <github@mryel.de>
|
@8023 could you please test this PR and share the controls output of the log? |
There was a problem hiding this comment.
🟡 Changes recommended
Critical Python compatibility and ctypes pointer issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes V4L2 logging for string, rectangle, bitmask, and menu controls.
Changes:
- Adds rectangle and extended-control payload support.
- Formats compound and hexadecimal control values.
- Generalizes UVC control metadata logging.
File summaries
| File | Summary |
|---|---|
crowsnest/v4l2/utils.py |
Adds rectangle mapping and hex formatting. |
crowsnest/v4l2/raw.py |
Adds rectangle and extended-control definitions. |
crowsnest/v4l2/ctl.py |
Parses metadata and retrieves compound control values. |
crowsnest/v4l2/constants.py |
Defines the rectangle control type. |
crowsnest/camera/types/uvc.py |
Updates control logging and value typing. |
Review details
Suppressed comments (3)
crowsnest/v4l2/ctl.py:269
- Formatting every non-payload value as a string changes the established numeric return type.
_brokenfocus()compares this result withint(focus_absolute_conf)(components/streamer/ustreamer.py:142), so a configuredfocus_absoluteis now treated as different on every startup and the corrective branch always runs. Preserve integers for scalar controls and use a mixed return annotation for payload controls.
return f"{ctrl.value}"
crowsnest/v4l2/ctl.py:246
ioctl_safe()catchesOSErrorand returns-1; it never raises into thistry. Since this return value is ignored, a failedVIDIOC_G_EXT_CTRLSleaves the zero-initialized payload buffer and the code logs an empty or zero rectangle/string as if it were the current value. Check the ioctl result and returnNoneon-1before decoding.
utils.ioctl_safe(fd, raw.VIDIOC_G_EXT_CTRLS, ctrls)
crowsnest/v4l2/ctl.py:50
- A string control's
default_valueis not a printable/default setting; the V4L2 control logger printsmin,max, andstepfor strings and omitsdefault. With this addition every string is logged asdefault=0whilestepis missing, which makes the new string logging inaccurate. AddSTRINGto the step condition instead and keep it out of this default whitelist.
constants.V4L2_CTRL_TYPE_STRING,
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Patrick Gehrsitz <github@mryel.de>
Signed-off-by: Patrick Gehrsitz <github@mryel.de>
Signed-off-by: Patrick Gehrsitz <github@mryel.de>
|
Thanks for following up on this properly, instead of leaving the guard from #366 as the fix. Heads up: I used AI to help me write this comment. Tested on the same machine as #366 . The full log in attached. The rect and bitmask controls now match A small difference: Regression: brokenfocus now triggers on every start The value is already 200. I had several earlier starts with the same config and no warning. The cause is that Suggestion: keep returning Two more things from reading the code. My camera has no controls of these types, so I couldn't test either:
|
|
Thanks for the small review and the output. Seems like I have to fix the intendation (currently just a magic number 😂)
I saw this in your output too. But if you look at the
Thanks for pointing out the regression. I already thought that I introduced a bug, but I couldn't find it 😅
Good catch ^^
This is true but this is nothing new introduced by this PR. This would need a separate fix. |
Signed-off-by: Patrick Gehrsitz <github@mryel.de>
Signed-off-by: Patrick Gehrsitz <github@mryel.de>
|
The string and brokenfocus should be fixed now. Please test them again. |
Signed-off-by: Patrick Gehrsitz <github@mryel.de>
Signed-off-by: Patrick Gehrsitz <github@mryel.de>
Signed-off-by: Patrick Gehrsitz <github@mryel.de>
|
Retested with c44d142. The config and
One small thing in Thanks for the quick fixes! |
Signed-off-by: Patrick Gehrsitz <github@mryel.de>
Please keep off-topic stuff out of the issues. You can share timelapses and similar things in our Discord in the
I'm not sure if there are any UVC devices without any controls at all, but it's a good point to add some fallback, even if it just occurs due to some weird communication error.
Thanks for the quick review and testing! |
This fixes the logging of the
str,rectandbitmasktypes in the v4l2 module.The wrong logging can be seen here: #366 (comment)
The logging is expected to look like this now:
A side effect of this PR is a fix for the logging of
minandmaxfor themenutype, similar to #366, as they are needed forstrandbitmasktoo.