-
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 temperatures as xu #12741
D500 temperatures as xu #12741
Conversation
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.
Looks great (just a small typo in the comment)
src/ds/ds-private.h
Outdated
@@ -60,6 +60,10 @@ namespace librealsense | |||
const uint8_t DS5_THERMAL_COMPENSATION = 0xF; | |||
const uint8_t DS5_EMITTER_FREQUENCY = 0x10; | |||
const uint8_t DS5_DEPTH_AUTO_EXPOSURE_MODE = 0x11; | |||
|
|||
const uint8_t DS5_HKR_PVT_TEMPERATURE = 0x15; |
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.
Shouldn't these be in d500-private??
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.
Sounds like it, @remibettan ?
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.
we agreed this is ok to for that to remain here, so that we do not override the value here for some other common command
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.
I don't like it. The code dependencies should reflect actual products. D400 shouldn't know about D500 etc.
Common commands need to take into account all products -- "ensuring it" by putting all product codes here is not 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.
As discussed, internally
2d75217
to
632d173
Compare
|
||
test.check(depth_sensor.supports(rs.option.soc_pvt_temperature)) | ||
test.check(depth_sensor.supports(rs.option.ohm_temperature)) | ||
test.check(depth_sensor.supports(rs.option.projector_temperature)) |
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 and the get will fail on D555e.
Maybe we should change to:
proj_temp = None
if depth_sensor.supports(rs.option.projector_temperature):
proj_temp = depth_sensor.get_option(rs.option.projector_temperature)
?
@remibettan can you please rebase and verify why the test failed? |
Sorry my bad. |
- temperatures test relaxed: tolerance increased from 0.1 to 1.0
13a0445
to
1561616
Compare
src/ds/d500/d500-options.h
Outdated
platform::extension_unit xu, | ||
uint8_t id, | ||
std::string description, | ||
bool allow_set_while_streaming = true); |
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.
Not a big deal, but this is not needed, can be set to false directly in the c'tor
src/ds/ds-private.h
Outdated
@@ -60,6 +60,7 @@ namespace librealsense | |||
const uint8_t DS5_THERMAL_COMPENSATION = 0xF; | |||
const uint8_t DS5_EMITTER_FREQUENCY = 0x10; | |||
const uint8_t DS5_DEPTH_AUTO_EXPOSURE_MODE = 0x11; | |||
|
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.
Redundant, only if you plan another push :)
Tracked by: RSDEV-732
REMINDER:
As we agreed, this PR should be merged after the corresponding feature is merged in the FW code