hdr test converted to python and stabilized - #12670
Conversation
| ctx = rs.context() | ||
| device = test.find_first_device_or_exit() | ||
| if ctx is not None: | ||
| devices_list = ctx.query_devices() |
There was a problem hiding this comment.
The original test used to perform a HW reset between each test, using restart_first_device_and_return_depth_sensor
This was removed, is this not required for this test suite?
@remibettan ?
There was a problem hiding this comment.
It would be better to perform HW reset between the tests, if they are changing some parameters, as the exposure for ID sequence 1/2.
Not needed for other tests.
| min_counter_set = False | ||
|
|
||
| if not min_counter_set: | ||
| min_counter = int(depth_counter) |
There was a problem hiding this comment.
is this casting needed in python?
There was a problem hiding this comment.
the original file had a static cast there, so i thought to do it like that.
but I'll just remove it, it's not really needed
There was a problem hiding this comment.
Still exist, let's please handle and answer all comments and than re-ask for a review from @remibettan
| test.check(depth_sensor.get_option(rs.option.hdr_enabled) == 1) | ||
|
|
||
| sequence_id = -1 | ||
| iterations_for_preparation = 30 |
There was a problem hiding this comment.
Can you explain the increase from 6 to 30?
|
|
||
| # emitter on/off works with PWM (pulse with modulation) in the hardware | ||
| # this takes some time to configure it | ||
| iterations_for_preparation = 30 |
750d293 to
94ce367
Compare
c3a48e2 to
74e36a8
Compare
|
Please try to fix this conflict: |
| device = test.find_first_device_or_exit() | ||
| depth_sensor = device.first_depth_sensor() | ||
|
|
||
| if depth_sensor and depth_sensor.supports(rs.option.hdr_enabled): |
There was a problem hiding this comment.
The test will not fail if this if statement is not filled, but the test itself will not be checked - please add else statement with failure of the test, with relevant message.
This comment is relevant for all below tests.
| depth_sensor.set_option(rs.option.sequence_id, 1) | ||
| test.check(depth_sensor.get_option(rs.option.sequence_id) == 1) | ||
| exp = depth_sensor.get_option(rs.option.exposure) | ||
| test.check(depth_sensor.get_option(rs.option.exposure) == exposure_range.default - 1000) # w/a |
There was a problem hiding this comment.
Please add some comment to explain this w/a
| depth_sensor.set_option(rs.option.hdr_enabled, 1) | ||
| test.check(depth_sensor.get_option(rs.option.hdr_enabled) == 1) | ||
|
|
||
| depth_sensor.set_option(rs.option.sequence_id, 1) |
There was a problem hiding this comment.
Please add some comment to explain that seq id 1 should be with default values
| test.check(depth_sensor.get_option(rs.option.exposure) == exposure_range.default - 1000) # w/a | ||
| test.check(depth_sensor.get_option(rs.option.gain) == gain_range.default) | ||
|
|
||
| depth_sensor.set_option(rs.option.sequence_id, 2) |
There was a problem hiding this comment.
Please add some comment to explain that seq id 2 should be with min values
| test.check(depth_sensor.get_option(rs.option.hdr_enabled) == 0) | ||
|
|
||
| device.hardware_reset() | ||
| time.sleep(5) |
There was a problem hiding this comment.
- isn't it long? 5 seconds.
- I see these lines are between each test, please make a function for it
| """ | ||
| helper method | ||
| checks that the frames resulting from the hdr_merge processing block are generated | ||
| from frames streamed within this method (and not using old frames) - see RSDSO-17341. |
There was a problem hiding this comment.
I would change "within this method" to "within this streaming session" - only if you agree
| 5 3 (no frame to merge with 5 as 6 didn't arrive yet) | ||
| 6 5 (merged frames 5+6) | ||
| ... (more frames, until a frame counter reset) | ||
| 1 1 (hdr # should like the value we reset to) |
There was a problem hiding this comment.
Did you mean "hdr # should be around the value we reset to"?
There was a problem hiding this comment.
not around, should be that value exactly... I'll fix that typo
| pipe.stop() | ||
| depth_sensor.set_option(rs.option.hdr_enabled, 0) # disable hdr before next tests | ||
|
|
||
|
|
There was a problem hiding this comment.
Yes it's on purpose, by your comment I figured it's not needed here?
| # get depth frame data | ||
| depth_frame = data.get_depth_frame() | ||
| if not test.check(depth_frame.supports_frame_metadata(rs.frame_metadata_value.sequence_id)): | ||
| return |
There was a problem hiding this comment.
Could you add here some log reading the fact that this metadata could not be read?
Do you think it should fail the test?
There was a problem hiding this comment.
on a second look... I dont think we need sequence id here, right? just the frame counter, what if we just remove it and remove also test.check(merged_frame.supports_frame_metadata(rs.frame_metadata_value.sequence_id)) ?
| frame_counter = frame.get_frame_metadata(rs.frame_metadata_value.frame_counter) | ||
| frame_seq_id = frame.get_frame_metadata(rs.frame_metadata_value.sequence_id) | ||
| if frame_counter != prev_frame_counter + 1: # can only compare sequential frames | ||
| sequence_id = frame_seq_id |
There was a problem hiding this comment.
Please change this param name from sequence_id to expected_sequence_id.
| depth_sensor.set_option(rs.option.emitter_on_off, 1) | ||
| test.check(depth_sensor.get_option(rs.option.emitter_on_off) == 0) | ||
|
|
||
| # Control's presence verified in the beginning of the block |
There was a problem hiding this comment.
I do not understand this comment - could you rephrase it, please?
remibettan
left a comment
There was a problem hiding this comment.
Few comments - but really great work!
|
Still conflict that needs to be resolved |
Tracked on: [LRS-1000]