Skip to content

Conversation

@randhid
Copy link
Contributor

@randhid randhid commented Feb 17, 2025

This PR adds the Discovery Service wrapper to the C++ SDK.

I mainly followed the pattern in #323.

I followed the arm.get_geometries method pattern to return an array of a ResourceConfig from discovery.discover_resources, which is intended to return a valid config with meaningful attributes for physically connected devices to viam-server.

@randhid randhid changed the title Discovery service [RSDK-9624] Add discovery service Feb 17, 2025
@randhid randhid force-pushed the discovery-service branch 4 times, most recently from fb3daa6 to 75c73e4 Compare February 20, 2025 00:11
Comment on lines +17 to +22
# Check if clang-format is installed
if ! command -v clang-format >/dev/null 2>&1; then
echo "Error: clang-format is not installed"
echo "Please install it using: brew install clang-format"
exit 1
fi
Copy link
Contributor Author

@randhid randhid Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove this - but it helped me figure out clang-format wasn't installed through brew on my mac. The original error returned was that xargs couldn't find a file.

@randhid randhid marked this pull request as ready for review February 20, 2025 01:25
@randhid randhid requested a review from a team as a code owner February 20, 2025 01:25
@randhid randhid requested review from njooma and stuqdog and removed request for a team February 20, 2025 01:25
@randhid randhid requested a review from lia-viam February 20, 2025 01:29
Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One or two minor things but otherwise this looks good to me!

}

ProtoStruct MockDiscovery::do_command(const sdk::ProtoStruct& command) {
return ProtoStruct{};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(minor) can we return the fake_map here instead? I tend to prefer returning something that isn't just the default constructed value, just out of an abundance of caution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@randhid randhid requested a review from stuqdog February 24, 2025 19:01
Copy link
Collaborator

@lia-viam lia-viam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, feel free to merge if you're done!

///
/// This acts as an abstract parent class to be inherited from by any drivers representing
/// specific discovery implementations. This class cannot be used on its own.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

I think this line needs to be deleted for the doxygen comment to bind

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need to build docs locally as well or will CI do that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI does that yeah!

randhid and others added 2 commits February 25, 2025 16:34
Co-authored-by: lia <167905060+lia-viam@users.noreply.github.com>
Co-authored-by: lia <167905060+lia-viam@users.noreply.github.com>
@randhid randhid merged commit 117de35 into main Feb 25, 2025
4 checks passed
@randhid randhid deleted the discovery-service branch February 25, 2025 22:14
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.

4 participants