Fix set gain exception Linux #9845
Conversation
src/linux/backend-v4l2.cpp
Outdated
| @@ -1280,6 +1280,12 @@ namespace librealsense | |||
| { | |||
| struct v4l2_control control = {get_cid(opt), value}; | |||
| if (RS2_OPTION_ENABLE_AUTO_EXPOSURE==opt) { control.value = value ? V4L2_EXPOSURE_APERTURE_PRIORITY : V4L2_EXPOSURE_MANUAL; } | |||
|
|
|||
| std::lock_guard<std::mutex> lock(_set_ctrl_event_mutex); | |||
There was a problem hiding this comment.
I would rather not have this mutex - in worst case we're going to stall the main thread for 30msec, which is not acceptable
There was a problem hiding this comment.
The worst cast I saw on 50 set command was:
subscribe_to_ctrl_event took :0.011617 [ms]
pend_for_ctrl_status_event took :8.54738 [ms]
unsubscribe_from_ctrl_event took :0.006908 [ms]
src/linux/backend-v4l2.cpp
Outdated
| if (!pend_for_ctrl_status_event()) | ||
| return false; | ||
|
|
||
| unsubscribe_from_ctrl_event(control.id); |
There was a problem hiding this comment.
- What is the impact of subscribe/unsubscribe ? How much penalty it adds?
- Note that in case of
return falsetheunsubscribewill not be executed.
Please restructure subscribe/unsubscribe it into RAII scheme (class or unique_ptr with custom dtor) to ensure that if an exception is raised in-between, the unsubscribe call will be enforced
src/linux/backend-v4l2.cpp
Outdated
| memset(event_subscription.reserved,0, sizeof(event_subscription.reserved)); | ||
| if (xioctl(_fd, VIDIOC_SUBSCRIBE_EVENT, &event_subscription) < 0) | ||
| { | ||
| throw linux_backend_exception("xioctl(VIDIOC_SUBSCRIBE_EVENT) failed"); |
There was a problem hiding this comment.
Pls add the control id to the exception message here and below
src/linux/backend-v4l2.cpp
Outdated
| struct v4l2_event event; | ||
| memset(&event, 0 , sizeof(event)); | ||
|
|
||
| // Poll registered events and verify that set control event raised (wait max of 6 * 5 = 30 [ms]) |
There was a problem hiding this comment.
What is the impact of making it 10 X 2 msec ? (2msec is the min interval for control calls we have in UVC pipe) . Imo - 5 msec intervals are not optimized.
There was a problem hiding this comment.
The min time I tried was 15 [ms], and I got cases of no event raised yet (first iteration it failed was > 1000)
I will try with 10 X 2
There was a problem hiding this comment.
Timing constants updated .
| depth_ir_sensor = dev.first_depth_sensor() | ||
|
|
||
| for i in range(test_iterations): | ||
| log.d("{} Itearion {} {}".format("=" * 50, i, "=" * 50)) |
| depth_ir_sensor.set_option(rs.option.enable_auto_exposure, 0) | ||
| depth_ir_sensor.set_option(rs.option.exposure, 1) | ||
| depth_ir_sensor.set_option(rs.option.gain, 248) |
There was a problem hiding this comment.
Add check for sensor.supports(... for generality
Nir-Az
force-pushed
the
fix_set_gain_exception_linux
branch
from
daf2e9a
to
a394031
Compare
src/linux/backend-v4l2.cpp
Outdated
| // RAII to handle unsubscribe in case of exceptions | ||
| std::unique_ptr<uint32_t, std::function<void(uint32_t*)> > unsubscriber( | ||
| new uint32_t(control.id), | ||
| [this](uint32_t* id){ if (id) unsubscribe_from_ctrl_event(*id); }); |
There was a problem hiding this comment.
Also add delete id; to avoid memleak
| test.check(val == get_val) | ||
| log.d("Gain Set To: {}".format(get_val)) | ||
|
|
||
| time.sleep(0.1) |
There was a problem hiding this comment.
This sleep may mask the issue (if it still exist).
also pls repeat the loop several time so the total num of iteration will be 20+
There was a problem hiding this comment.
The sleep was there cause that's validation script, this exact test failed on their side.
The loop occurs 200 times, each time sets 5 values (total of 1000 sets), I didn't understand your request..
Nir-Az
force-pushed
the
fix_set_gain_exception_linux
branch
from
5dda755
to
b23614d
Compare
set_pucommand was executed before the function call returnsThe test pass on Linux with the fix and fail on Windows.
Tracked on [DSO-17185]