-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
d500 devices - hw ts from capture stats #13033
d500 devices - hw ts from capture stats #13033
Conversation
b8d8e97
to
49b5154
Compare
hw_ts = frame.get_frame_metadata(rs.frame_metadata_value.frame_timestamp) | ||
sensor_ts = frame.get_frame_metadata(rs.frame_metadata_value.sensor_timestamp) | ||
delta = sensor_ts - hw_ts | ||
test.check(delta < (1/fps * 10^6)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also check delta > 0
.
If hw_ts is after sensor_ts you will get a negative delta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
# License: Apache 2.0. See LICENSE file in root directory. | ||
# Copyright(c) 2024 Intel Corporation. All Rights Reserved. | ||
|
||
# test:donotrun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should enable the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test cannot be enabled right now, because we have no device that supports it in our CI, and this would fail it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, then maybe add a comment explaining why the test is disabled and when it should be enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
offsetof(md_rgb_mode, rgb_mode) + | ||
offsetof(md_rgb_normal_mode, intel_capture_stats); | ||
|
||
color_ep.register_metadata(RS2_FRAME_METADATA_FRAME_TIMESTAMP, make_attribute_parser(&md_capture_stats::hw_timestamp, md_capture_stat_attributes::hw_timestamp_attribute, md_prop_offset)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, before we merge, 1 question
why only color? IMO D500 FW will set this for all non IMU streams
Please make sure with Xuetao
Can you make sure with a drop from HKR that this is enabled?
Merging this will cause no timetag right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we also need to change sensor TS to be the new HW TS - Sensor timestamp from MD which is 1/2 of the exposure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Nir - this is for merging to dev branch. I will also do the change for depth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding the sensor ts - this is the only one that remains as uvc header's ts, so I'd rather let it as is - do you agree?
Moreover, I need this in my test - see in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On D400 and on D500 until now, uvc header timestamp was acctually the hw timestamp, fw set it to the time right after it grabbed the frame, and sensor ts reduced half the exposure so it was actually the sensor time stamp.
Now that FW will set the uvc header as the time the UVC message is constructed (could be much after the readout, reducing the exposure time has no meaning
This is why it needs to change to hw_timestamp - half the exposure (sensor timestamp in MD)
Please try to get more info from Evgeni on Sunday if needed
|
||
depth_sensor.open(depth_profile) | ||
depth_sensor.start(check_hw_ts_right_before_sensor_ts) | ||
time.sleep(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's check that we get into the CB at least once
8cbb2f2
into
IntelRealSense:development
Tracked by: RSDEV-2276