Skip to content

JSON-based device-info & fix for no 'dds' settings - #12540

Merged
maloel merged 12 commits into
realsenseai:developmentfrom
maloel:device-info
Jan 1, 2024
Merged

JSON-based device-info & fix for no 'dds' settings#12540
maloel merged 12 commits into
realsenseai:developmentfrom
maloel:device-info

Conversation

@maloel

@maloel maloel commented Dec 26, 2023

Copy link
Copy Markdown
Contributor

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]

@maloel
maloel requested a review from OhadMeir December 26, 2023 11:23
Comment thread third-party/rsutils/include/rsutils/json.h Outdated
//bool locked = true;

std::string const & name() const;
void set_name( std::string && );

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Contributor

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 &

Copy link
Copy Markdown
Contributor Author

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;

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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?

@maloel
maloel merged commit 4d12d4f into realsenseai:development Jan 1, 2024
@maloel
maloel deleted the device-info branch January 1, 2024 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants