-
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
JSON-based device-info & fix for no 'dds' settings #12540
Conversation
//bool locked = true; | ||
|
||
std::string const & name() const; | ||
void set_name( std::string && ); |
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 am not sure from user perspective that rvalue is always desired, for instance rs2_device_to_info
copies the name
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.
Yeah I debated but eventually I just decided that it's fine if we copied before calling the function. Otherwise I can change to const &. What would you prefer?
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 think I prefer const &
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 means always a copy vs. a move if possible. But OK, done.
|
||
// FW update ID is a must for DFU; all cameras should have one | ||
std::string const serial_number = dev.get_info( RS2_CAMERA_INFO_FIRMWARE_UPDATE_ID ); | ||
j["fw-update-id"] = serial_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.
Don't we want to check support for RS2_CAMERA_INFO_FIRMWARE_UPDATE_ID? Maybe there is a different ID
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.
All our cameras have a FW update ID. I'll add the supports anyway.
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 serial number is actually used to set up the topic root path. It's quite necessary. I think I'll leave as-is so it'll throw if it doesn't exist.
/** | ||
* \return the one-line description: "[<type>] <name> s/n <#>" | ||
*/ | ||
std::string get_description() const |
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.
Don't we need a C API corresponding to all CPP APIs?
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 think so, though we could certainly do it -- this only sits at the top of the hourglass
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.
According to Evgeni we need to be able to do in C whatever we do in CPP.
The APIs should be synchronized and the CPP just a wrapper to the C
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.
But it is: this is using the C APIs. I am not adding another API here. I am making the code reusable. Are you suggesting I add yet another API that does this inside librealsense?
With DFU, we need to communicate more fields, and maybe even custom fields.
The device-info is now just a JSON proxy.
There was a bug introduced recently with
"dds": false
in the settings. This includes a fix.Other minor stuff - go by commit.
Tracked on [LRS-989]