-
Notifications
You must be signed in to change notification settings - Fork 5k
sw-dev profiles work; shorter rs-enum-devs output; use-basic-formats -> format-conversion #12109
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
Conversation
| rs2_stream fourcc_to_rs2_stream(uint32_t fourcc_format) const; | ||
|
|
||
| protected: | ||
| // Since _profiles is private, we need a way to get the final profiles |
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 is sensor_base, that is, intended to be inherited from.
Why not make the member protected?
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.
The member is pretty advanced (lazy) and really should be private, I didn't want to make it accessible.
| args.add_argument( '--time', metavar='<seconds>', type=time_arg, default=2, help='seconds to gather devices for (default 2)' ) | ||
| def domain_arg(x): | ||
| t = int(x) | ||
| if t <= 0: |
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.
Why not check > 232? Same for other script
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.
copy paste :)
Fixed.
| from argparse import ArgumentParser | ||
| args = ArgumentParser() | ||
| args.add_argument( '--debug', action='store_true', help='enable debug mode' ) | ||
| args.add_argument( '--quiet', action='store_true', help='No output; just the minimum FPS as a number' ) |
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.
Comment left from FPS script?
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.
Yep. Removed.
| tags.push_back( { RS2_STREAM_COLOR, | ||
| -1, // index | ||
| color_width, color_height, | ||
| ( format_conversion == format_conversion::full ) ? RS2_FORMAT_RGB8 : RS2_FORMAT_YUYV, |
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.
It would be more readable to have variables color_format / infrared_format and set them in the list above.
| if( ! context ) | ||
| return format_conversion::full; | ||
| std::string const format_conversion( "format-conversion", 17 ); | ||
| std::string const full( "full", 4 ); |
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.
Why use string only for one option?
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.
It's reused, I guess.
OhadMeir
left a comment
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
sensor_baseand split offraw_sensor_base_profilesnow only insensor_base; software_device uses_sw_profilesand no more_raw_rs_profilessensor_interface::get_raw_stream_profiles()RGB8; now all the permutations supported by LibRSraw,basic, orfull(use-basic-formats->format-conversion)rs-enumerate-devicesoutput now aggregates all FPS lines together, so more readableTracked on [LRS-848]