RSDK-4573: remove proto types from robot client apis #207

purplenicole730 · 2024-01-28T23:18:41Z

Removed the proto types that were in the robot client API's:

defined new discovery_query, discovery, frame_system_config, status, and operation
changed robot client API's to use already defined WorldState::transform and Name
defined proto conversion methods in a private namespace in robot/client.cpp
created functions to return custom defined types while keeping old proto functions for MockRobotService
added new tests to compare mock proto and custom methods to ensure same fields
changed test_robot.cpp to test custom types
added a new Impl object to handle robot::v1::RobotService

…nt-apis

src/viam/sdk/robot/client.hpp

acmorrow

Lots of comments and ideas, but I think this is headed in a good direction. @stuqdog I have some questions here and there for you.

In some places I've left notes on existing code that looks like it needs help or can be improved. Those things don't necessarily need to be acted on to land this review but if they aren't they should probably be ticketed and TODOs added to the review.

src/viam/sdk/common/utils.cpp

acmorrow · 2024-02-02T21:17:29Z

src/viam/sdk/common/utils.cpp

+            .append(resource_subtype)
+            .append("/")
+            .append(resource->name());
+        resource_names.push_back(Name::from_string(name_str));


So, Name::from_string actually just does splits on \ and :, so more or less this string is being built up just to be decomposed. Could you just do

resource_names.push_back({{kRDK, std::move(resource_type), std::move(resource_subtype)}, resource->name()})

or similar? I don't think that quite works because of something about name vs remote_name in the Name::Name constructor, which I'm not quite sure what the difference is. But maybe the constructor needs a little help.

acmorrow · 2024-02-02T21:18:38Z

src/viam/sdk/common/utils.hpp

    }
-    static bool check_equal(const ResourceName r1, const ResourceName r2) {
-        return r1.SerializeAsString().compare(r2.SerializeAsString());
+    static bool check_equal(const Name r1, const Name r2) {


These parameters should also be by const reference, while you are here.

Actually is ResourceNameEqual even still needed? Name defines operator==.

acmorrow · 2024-02-02T21:19:15Z

src/viam/sdk/common/utils.hpp


 class ResourceNameHasher {
   public:
-    size_t operator()(ResourceName const& key) const {
-        return std::hash<std::string>()(key.SerializeAsString());
+    size_t operator()(Name const& key) const {


This class should probably be renamed to NameHasher

@stuqdog - If Names as keys in unordered_maps will almost always use this hasher, we could make life a little simpler by doing this:

... } // namespace viam namespace std { template<> struct hash<::viam::sdk::Name> { size_t operator()(const ::viam::sdk::Name& name) const noexcept { return name.hash(); } }; } // namespace std

And then add size_t Name::hash() const noexcept to the Name class. Then ResourceNameHasher just goes away, and you can use unordered_map<Name, T> without specifying the hash.

Yep I completely agree. In fact it looks like we already define hash for Name in resource_api.hpp so I think we can just delete the ResourceNameEqual and ResourceNameHasher classes entirely and just be done with it, they were only ever necessary (if at all) because ResourceName was defined in proto.

acmorrow · 2024-02-02T21:51:25Z

src/viam/sdk/robot/client.hpp

-using viam::robot::v1::Status;
+using time_point = std::chrono::time_point<long long, std::chrono::nanoseconds>;
+
+struct discovery_query {


I feel like these types belong within Robot (which apparently doesn't exist?). @stuqdog should there be a Robot base class with an abstract API? Or is there no purpose / need for such a thing.

Either way, I think these types should not be at the ::viam::sdk:: level and should be inside RobotClient in the absence of Robot.

Hmm... I personally am not a fan of having a Robot base class. In my mind there's only a single canonical implementation of how one should interact with a robot as a client in C++, and so it doesn't really make sense to expose an abstract API the same way it does for the various resources.

That said, there's something very slightly smelly about having these types exist on RobotClient (as opposed to Robot) that I'm having trouble putting my finger on. So, I'm certainly open to being convinced that an abstract Robot base class makes sense.

Either way, I agree that these structs should exist within RobotClient (or Robot if we create it) rather than as free-floating types.

Yes I share your unease, and I find the lack of consistency between Robot and the other resource types a little troublesome as well. Also unclear what the use case is for RobotService_. I can't quite tell what it is for, which makes it hard for me to honestly evaluate whether a Robot base is needed. It feels like if RobotService_ is meaningful, shouldn't be what it is serving be an implementation of Robot, not a RobotClient?

acmorrow · 2024-02-02T22:23:42Z

src/viam/sdk/robot/client.hpp

-    /// @param extra Any extra params to pass to resources' `stop` methods, keyed by `ResourceName`.
-    void stop_all(std::unordered_map<ResourceName,
+    /// @param extra Any extra params to pass to resources' `stop` methods, keyed by `Name`.
+    void stop_all(std::unordered_map<Name,


Huh. So the second parameter here is like an unwrapped AttributeMap that's not held by shared_ptr. Not asking you to change it, but I think the AttributeMap infra is something we should look at cleaning up.

Oh good catch! I actually think it would be great to change this to an AttributeMap now while we're already changing the RobotClient API considerably.

acmorrow · 2024-02-02T22:33:04Z

src/viam/examples/camera/example_camera.cpp


        cout << "Resources of the robot:" << endl;
-        for (vs::ResourceName resource : *resource_names) {
-            cout << " - " << resource.name() << " (" << resource.subtype() << ")" << endl;
+        for (vs::Name resource : *resource_names) {


for (const auto& resource : *resource_names) while you are here.

acmorrow · 2024-02-02T22:38:19Z

src/viam/sdk/resource/resource.cpp

@@ -21,8 +21,8 @@ std::string Resource::name() const {

 void Resource::reconfigure(Dependencies deps, ResourceConfig cfg){};

-ResourceName Resource::get_resource_name(std::string name) {
-    ResourceName r;
+common::v1::ResourceName Resource::get_resource_name(std::string name) {


You can bring this in with using since you are in .cpp scope, if you like.

acmorrow · 2024-02-02T22:42:31Z

src/viam/sdk/robot/client.cpp

@@ -57,6 +47,110 @@ using viam::robot::v1::Status;
 // NOLINTNEXTLINE
 const std::string kStreamRemoved("Stream removed");

+viam::robot::v1::DiscoveryQuery discovery_query::to_proto() const {


@stuqdog - I'm considering whether we want some sort of traits class for proto to type interconversion. Think From/Into sort of thing. It'd let us use a uniform syntax for moving between our vocabulary types and the backing protos. I don't have syntax in mind yet, but any thoughts?

I like this. I don't think it's urgent but programmatically enforcing a consistent syntax and having a way of clearly expressing in code which types we expect to have proto conversions seems like a win. I'm not positive I have the look of it precisely right in my head, something like the following?

class ProtoConvertible { public: template <typename T, typename ProtoT> struct traits; template <typename T, typename ProtoT> static T from_proto(const ProtoT& proto) { return traits<T, ProtoT>::from_proto(proto); } template <typename T, typename ProtoT> static ProtoT to_proto(const T& t) const { return traits<T, ProtoT>::to_proto(t); } ... }

and then define in robot_client.hpp

template <> struct ProtoConvertible::traits<frame_system_config, FrameSystemConfig> { static FrameSystemConfig to_proto(const frame_system_config& fsc); static frame_system_config to_proto(const FrameSystemConfig& proto); } class RobotClient { ... struct frame_system_config { ... FrameSystemConfig to_proto() const { return ProtoConvertible::to_proto<frame_system_config, FrameSystemConfig>(this); } // actual implementation should go in .cpp static frame_system_config from_proto(const FrameSystemConfig& proto) { return ProtoConvertible::from_proto<frame_system_config, FrameSystemConfig>(proto); } // actual implementation should go in .cpp } ... }

And then implement all the actual logic in (for this example) robot_client.cpp?

Assuming I've got the gist of it right here, it seems like a bit of boilerplate but probably worthwhile for clarity and consistency.

Yes, I think that's along the lines of what I had in mind. I filed a ticket so we don't forget - it definitely isn't work to be done in this review.

acmorrow · 2024-02-02T22:44:37Z

src/viam/sdk/robot/client.cpp

    const std::lock_guard<std::mutex> lock(lock_);
-    std::vector<ResourceName>* resources = &resource_names_;
+    std::vector<Name>* resources = &resource_names_;


Oh, this is worse than I thought. The lock is completely defeated here because the returned pointer is to internal state. This needs to return by value.

Not entirely certain I follow why this needs to return by value, does changing return type to const& not solve this? If so, then returning a copy seems totally fine.

No, returning by const& isn't sufficient, because you could still be doing an (unlocked) read of the state via the reference at the same time a locked mutation was in progress, since the thread holding the reference doesn't hold the lock.

Ahh sure, that makes sense. Thanks for clarifying! In that case then yeah, let's change this to return a copy.

stuqdog

Nice, excited to see this happen! Got some general suggestions/comments/questions but overall this definitely is looking good.

src/viam/sdk/common/utils.cpp

src/viam/sdk/common/utils.hpp

src/viam/sdk/tests/mocks/mock_robot.cpp

src/viam/sdk/tests/mocks/mock_robot.hpp

src/viam/sdk/robot/service.hpp

src/viam/sdk/robot/client.hpp

stuqdog · 2024-02-02T23:43:24Z

src/viam/sdk/robot/client.hpp

-        *r.mutable_type() = api.resource_type();
-        *r.mutable_subtype() = api.resource_subtype();
-        *r.mutable_name() = std::move(name);
+        Name r = Name::from_string(api.to_string() + "/" + name);


This could be better written as Name r(api, "", name);

stuqdog · 2024-02-02T23:49:25Z

Lots of comments and ideas, but I think this is headed in a good direction. @stuqdog I have some questions here and there for you.

In some places I've left notes on existing code that looks like it needs help or can be improved. Those things don't necessarily need to be acted on to land this review but if they aren't they should probably be ticketed and TODOs added to the review.

👍 I'll take a look through your comments and reply on Monday!

acmorrow

Just following up on a few comment threads, I'll take another look at this when updates have been pushed.

acmorrow · 2024-02-06T15:22:12Z

src/viam/sdk/robot/client.cpp

@@ -57,6 +47,110 @@ using viam::robot::v1::Status;
 // NOLINTNEXTLINE
 const std::string kStreamRemoved("Stream removed");

+viam::robot::v1::DiscoveryQuery discovery_query::to_proto() const {


Yes, I think that's along the lines of what I had in mind. I filed a ticket so we don't forget - it definitely isn't work to be done in this review.

src/viam/sdk/robot/client.hpp

acmorrow · 2024-02-06T15:32:02Z

src/viam/sdk/robot/client.hpp

-using viam::robot::v1::Status;
+using time_point = std::chrono::time_point<long long, std::chrono::nanoseconds>;
+
+struct discovery_query {


Yes I share your unease, and I find the lack of consistency between Robot and the other resource types a little troublesome as well. Also unclear what the use case is for RobotService_. I can't quite tell what it is for, which makes it hard for me to honestly evaluate whether a Robot base is needed. It feels like if RobotService_ is meaningful, shouldn't be what it is serving be an implementation of Robot, not a RobotClient?

acmorrow · 2024-02-06T15:38:25Z

src/viam/sdk/robot/client.hpp

@@ -68,10 +119,10 @@ class RobotClient {
    static std::shared_ptr<RobotClient> with_channel(std::shared_ptr<ViamChannel> channel,
                                                     Options options);
    RobotClient(std::shared_ptr<ViamChannel> channel);
-    std::vector<ResourceName>* resource_names();
+    std::vector<Name>* resource_names();


See the other thread about this on the implementation but it definitely needs to be a copy.

stuqdog

Nice, definitely looking better! A couple general comments (including one about proto leakage in robot/client.hpp that may or may not be out of scope for this PR), but I'm really liking the way this is shaping up. Thanks!

stuqdog · 2024-02-08T20:40:17Z

src/viam/examples/camera/example_camera.cpp

@@ -44,11 +44,12 @@ int main() {
            throw;
        }

-        std::vector<vs::ResourceName>* resource_names = robot->resource_names();
+        std::vector<vs::Name>* resource_names = robot->resource_names();


Looks like this method is still returning a reference. We'll want to make a copy of the robot's resource_names and return that by value (i.e., not as a pointer).

Does this mean simply returning a std::vector<Name>?

stuqdog · 2024-02-08T21:25:35Z

src/viam/examples/dial/example_dial.cpp

+    for (const Name& resource : *resource_names) {
+        std::cout << "\tname: " << resource.name() << " (type:" << resource.api().resource_type()
+                  << " subtype:" << resource.api().resource_subtype() << ")" << std::endl;


Given that we implement << for Name, maybe we can simplify this and just say std::cout << "\t" << name << "\n";

stuqdog · 2024-02-08T21:26:21Z

src/viam/examples/dial_api_key/example_dial_api_key.cpp

+    for (const Name& resource : *resource_names) {
+        std::cout << "\tname: " << resource.name() << " (type:" << resource.api().resource_type()
+                  << " subtype:" << resource.api().resource_subtype() << ")" << std::endl;


Ditto above comment on Name implementing <<

src/viam/sdk/common/utils.cpp

stuqdog · 2024-02-08T21:28:45Z

src/viam/sdk/common/utils.cpp

-        *r.mutable_subtype() = resource_subtype;
-        resource_names.push_back(r);
+        resource_names.push_back(
+            Name({kRDK, resource_type, resource_subtype}, "", resource->name()));


No need to change this, but fyi I don't think you even need to declare the type Name here. you could just push back {{kRDK, resource_type, resource_subtype}, "", resource->name()}

stuqdog · 2024-02-08T22:08:29Z

src/viam/sdk/robot/client.hpp

@@ -68,10 +119,10 @@ class RobotClient {
    static std::shared_ptr<RobotClient> with_channel(std::shared_ptr<ViamChannel> channel,
                                                     Options options);
    RobotClient(std::shared_ptr<ViamChannel> channel);
-    std::vector<ResourceName>* resource_names();
+    std::vector<Name>* resource_names();


@purplenicole730 per above comment, we should definitely update this to not return a pointer.

stuqdog · 2024-02-08T22:16:07Z

src/viam/sdk/robot/client.hpp

+        viam::robot::v1::DiscoveryQuery to_proto() const;
+        static discovery_query from_proto(const viam::robot::v1::DiscoveryQuery& proto);


It occurs to me that the goal of this PR is to remove proto types from the robot_client APIs, but these structs are reintroducing a proto dependency. This isn't great! I wanna think about the best way to go about resolving this... perhaps it is best to introduce a Robot base class after all. We can define the structs in Robot, and then define their to_proto/from_proto methodology in RobotClient.

Another possibility: I believe the only users of the to_proto/from_proto methods here are the robot tests. If that is correct (which will need to be confirmed). That means possibly we could remove those tests, or have them include robot/client.*cpp* rather than robot/client.hpp in order to access methods not exposed in the .hpp. Honestly both these options seem pretty bad and probably worth just discarding (especially when the Robot base class option exists), but I'm curious if others have thoughts or other proposals.

Yes, I mentioned this in an earlier round of review too. Ultimately, this review isn't currently achieving the stated goal of removing the proto types from the Robot API: it has just pushed them into the to_proto and from_proto methods on the nested types.

One thing you could do is just make all of them into free functions in an unnamed namespace inside robot.cpp:

// robot.cpp ... namespace { viam::robot::v1::DiscoveryQuery to_proto(const RobotClient::discovery_query& dq) { ... }; discovery_query from_proto(const viam::robot::v1::DiscoveryQuery& proto); // Repeat for `discovery`, `frame_system_config`, etc. } // namespace

This will work very well for implementing Robot and wouldn't force the introduction of a base class for Robot.

However, it comes with one notable downside, which is that the conversion functions are no longer available to the tests.

That's frustrating, but maybe not the end of the world. After all, the tests are perfectly able to traffic in the proto types. So if you can arrange a suitable situation via mocks, then you can test indirectly: call the API with the vocabulary type, collect the proto object in the mock, and compare. Then do the reverse.

After all, the whole point of having a class like Robot is to encapsulate the proto stuff away and provide a better UX to consumers.

So the move seems to be to make a base class Robot in a new robot.cpp file. As for testing, test_robot doesn't use the protos, but test_types does.

@acmorrow When you say "collect the proto object in the mock, and compare", do you mean having test_types include the robot.cpp file that has the base Robot class with all the to/from proto functions? I changed the mocks so that they return the custom defined types, but I can easily define them in test_type.

No, let's stay away from #includeing .cpp files: it is a quick way to end up with multiply defined symbols.

Instead, my suggestion is to make all of the proto conversion functions private to the robot.cpp file. Tests will no longer be able to explicitly test them.

Instead, use the "mock pipeline" approach to connect the RobotClient to a mock. Then, inside the mock, you can validate that the vocabulary types work correctly end-to-end without ever using the proto types directly: that'll happen only in the RobotClient code. So the testing will still happen, but it will be indirect.

@stuqdog - Does that make sense to you?

Yep, completely agree.

Actually to clarify: I think the proto conversion functions can't be just private in robot.cpp if Robot is an abstract base class. Instead I think they should be private in robot/client.cpp where they are actually being used. So I think we should define the types in robot.hpp, and then add proto conversion methods privately in the client.

But, there isn't currently a robot.hpp or a Robot base class right now, is there? Do you think adding one should be part of this review? I think we can get away without it?

Yep agreed, a project for a future ticket.

stuqdog · 2024-02-08T22:19:05Z

src/viam/sdk/robot/client.hpp

@@ -155,9 +197,10 @@ class RobotClient {
    std::shared_ptr<ViamChannel> viam_channel_;
    std::shared_ptr<Channel> channel_;
    bool should_close_channel_;
-    std::unique_ptr<RobotService::Stub> stub_;
+    struct Impl;


@acmorrow a question for you! you proposed naming this struct Impl but that naively seems to go against our standard naming convention for structs (that they be snake_case). I realize this is a bit of a unique case in the SDK (Impl is private while structs in general are defined by their public-ness, and this is our only current instance of the pimpl idiom). Is it more idiomatic given these distinctions to break the normal naming convention and call it Impl, or keep with the struct naming convention and call it impl?

Oh, I'm just typing from muscle memory there. Fine to call it impl I think.

I truly wish we could bulk convert the whole SDK to snake_case.

stuqdog · 2024-02-08T22:22:16Z

src/viam/sdk/robot/client.cpp

-using viam::common::v1::PoseInFrame;
-using viam::common::v1::ResourceName;
-using viam::common::v1::Transform;
-using viam::robot::v1::Discovery;
-using viam::robot::v1::DiscoveryQuery;
-using viam::robot::v1::FrameSystemConfig;
-using viam::robot::v1::Operation;
 using viam::robot::v1::RobotService;
-using viam::robot::v1::Status;


Not a big deal either way, but I think maintaining these using lines in the .cpp file (as opposed to the .hpp) is actually fine and may be worth doing if typing out the fully qualified type becomes tedious.

stuqdog · 2024-02-08T22:34:27Z

src/viam/sdk/robot/client.hpp

-        *r.mutable_type() = api.resource_type();
-        *r.mutable_subtype() = api.resource_subtype();
-        *r.mutable_name() = std::move(name);
+        Name r = Name(api, "", name);


(minor) I think we still want to update this to Name r(api, "", name);. I'm a little fuzzy on some of these details but I believe what we have now uses the copy constructor (i.e., we're making a Name, and then copying it and assigning r to equal that copy), where as Name r(api, "", name) only constructs one Name. I could be wrong given that we're dealing with an rvalue Name though! Happy for @acmorrow to come in and correct me on this one :)

You could just write it Name r{api, "", name}, but I don't think you even need r anymore. I'd expect the following to work fine:

auto resource = this->resource_by_name({API::get<T>(), "", std::move(name)});

acmorrow

I'm in agreement with all of @stuqdog's comments and thoughts. I've shared a few of my own around the issue of the visibility of the proto conversion functions.

src/viam/sdk/common/utils.cpp

acmorrow · 2024-02-08T21:07:01Z

src/viam/sdk/robot/client.cpp

+}
+
+struct RobotClient::Impl {
+    Impl(std::unique_ptr<RobotService::Stub> stub) : stub_(std::move(stub)) {}


Did you try this as Impl(RobotService::Stub&& stub) : stub_(std::move(stub)) {} per my other comment?

Yes, after making it so that stub_ is no longer a pointer, it then gives me an error like so:

note: in instantiation of function template specialization 'std::make_unique<viam::sdk::RobotClient::impl, std::unique_ptr<viam::robot::v1::RobotService::Stub>>' requested here impl_(std::make_unique<impl>(RobotService::NewStub(channel_))) {} ^

called when we try to call:

RobotClient::RobotClient(std::shared_ptr<ViamChannel> channel) : viam_channel_(channel), channel_(channel->channel()), should_close_channel_(false), impl_(std::make_unique<impl>(RobotService::NewStub(channel_))) {}

acmorrow · 2024-02-08T21:10:33Z

src/viam/sdk/robot/client.cpp

-using viam::robot::v1::Discovery;
-using viam::robot::v1::DiscoveryQuery;
-using viam::robot::v1::FrameSystemConfig;
-using viam::robot::v1::Operation;
 using viam::robot::v1::RobotService;


Do you still need v1::RobotService here?

Yes, we call it quite often, both to call the RobotService::NewStub function, or just as a type RobotService::Stub.

Apologies, I meant to leave that comment on the client.hpp header, not the client.cpp file. I will re-add the comment in the right place.

src/viam/sdk/common/utils.cpp

acmorrow · 2024-02-09T16:33:32Z

src/viam/sdk/robot/client.hpp

+        viam::robot::v1::DiscoveryQuery to_proto() const;
+        static discovery_query from_proto(const viam::robot::v1::DiscoveryQuery& proto);


Yes, I mentioned this in an earlier round of review too. Ultimately, this review isn't currently achieving the stated goal of removing the proto types from the Robot API: it has just pushed them into the to_proto and from_proto methods on the nested types.

One thing you could do is just make all of them into free functions in an unnamed namespace inside robot.cpp:

// robot.cpp ... namespace { viam::robot::v1::DiscoveryQuery to_proto(const RobotClient::discovery_query& dq) { ... }; discovery_query from_proto(const viam::robot::v1::DiscoveryQuery& proto); // Repeat for `discovery`, `frame_system_config`, etc. } // namespace

This will work very well for implementing Robot and wouldn't force the introduction of a base class for Robot.

However, it comes with one notable downside, which is that the conversion functions are no longer available to the tests.

That's frustrating, but maybe not the end of the world. After all, the tests are perfectly able to traffic in the proto types. So if you can arrange a suitable situation via mocks, then you can test indirectly: call the API with the vocabulary type, collect the proto object in the mock, and compare. Then do the reverse.

After all, the whole point of having a class like Robot is to encapsulate the proto stuff away and provide a better UX to consumers.

acmorrow · 2024-02-09T16:36:18Z

src/viam/sdk/robot/client.hpp

@@ -155,9 +197,10 @@ class RobotClient {
    std::shared_ptr<ViamChannel> viam_channel_;
    std::shared_ptr<Channel> channel_;
    bool should_close_channel_;
-    std::unique_ptr<RobotService::Stub> stub_;
+    struct Impl;


Oh, I'm just typing from muscle memory there. Fine to call it impl I think.

I truly wish we could bulk convert the whole SDK to snake_case.

acmorrow

This is coming along well.

Please take care of the items I've called out here, and then I think it is time to tackle moving the to_proto and from_proto methods out of the header and refactoring the testing to use a mock pipeline per discussion.

src/viam/examples/dial/example_dial.cpp

acmorrow · 2024-02-12T16:14:54Z

src/viam/sdk/common/utils.cpp

@@ -23,6 +23,7 @@ namespace sdk {

 using time_point = std::chrono::time_point<long long, std::chrono::nanoseconds>;

+// TODO: RSDK-6627 : move this function to `Resource` class.


acmorrow · 2024-02-12T16:20:26Z

src/viam/sdk/robot/client.cpp

-using viam::robot::v1::Discovery;
-using viam::robot::v1::DiscoveryQuery;
-using viam::robot::v1::FrameSystemConfig;
-using viam::robot::v1::Operation;
 using viam::robot::v1::RobotService;


Apologies, I meant to leave that comment on the client.hpp header, not the client.cpp file. I will re-add the comment in the right place.

src/viam/sdk/robot/client.hpp

acmorrow · 2024-02-12T16:28:16Z

src/viam/sdk/robot/client.cpp

    const std::lock_guard<std::mutex> lock(lock_);
-    std::vector<Name>* resources = std::move(&resource_names_);
+    std::vector<Name> resources = std::move(resource_names_);


I think this is actually going to destroy the contents of resource_names_ since they will be moved into resources and then returned. I think you can get rid of this line entirely and now write return resource_names_ which will implicitly copy. It actually wouldn't have been possible to make this error if RobotClient::resource_names was declared const, which I think it probably should be. Can you please make it const? Note that you will probably need to declare lock_ as mutable to do so. That's fine, it is very common for private mutex members to be mutable: https://stackoverflow.com/questions/14131572/always-declare-stdmutex-as-mutable-in-c11

std::vector<Name> RobotClient::resource_names() const { mutable std::lock_guard<std::mutex> lock(lock_); return resource_names_; }

This code gave the error below:

error: 'mutable' can only be applied to member variables mutable std::lock_guard<std::mutex> lock(lock_); ^

The mutable should go on the declaration of RobotClient::lock_.

acmorrow · 2024-02-12T16:36:28Z

src/viam/sdk/robot/client.hpp

-        Name r = Name(api, "", name);
-
-        auto resource = this->resource_by_name(std::move(r));
+        auto resource = this->resource_by_name({API::get<T>(), "", name});


I don't think you need the this-> here. And the function is now simple enough it could be a one liner:

std::shared_ptr<T> resource_by_name(std::string name) { return std::dynamic_pointer_cast<T>(resource_by_name({API::get<T>(), "", std::move(name)})); }

Note that name should be moved from.

acmorrow · 2024-02-12T16:45:07Z

src/viam/sdk/robot/client.hpp

-        std::vector<Transform> additional_transforms = std::vector<Transform>());
+    std::vector<frame_system_config> get_frame_system_config(
+        std::vector<WorldState::transform> additional_transforms =
+            std::vector<WorldState::transform>());


You can make this simpler: std::vector<WorldState::transform> additional_transforms = {}

acmorrow · 2024-02-12T16:45:30Z

src/viam/sdk/robot/client.hpp

+    pose_in_frame transform_pose(pose_in_frame query,
+                                 std::string destination,
+                                 std::vector<WorldState::transform> additional_transforms =
+                                     std::vector<WorldState::transform>());


Ditto as = {} for the defaulted parameter.

acmorrow · 2024-02-12T16:46:39Z

src/viam/sdk/robot/client.cpp

+DiscoveryQuery RobotClient::discovery_query::to_proto() const {
+    DiscoveryQuery proto;
+    *proto.mutable_subtype() = subtype;
+    *proto.mutable_model() = std::move(model);


This std::move probably doesn't do anything: the method is const so model is const, so can't be moved from.

acmorrow · 2024-02-12T16:48:55Z

src/viam/sdk/robot/client.cpp

+}
+
+bool operator==(const RobotClient::discovery& lhs, const RobotClient::discovery& rhs) {
+    return lhs.query == rhs.query && map_to_struct(lhs.results).SerializeAsString() ==


Why do results get turned back into proto Struct's in order to compare them with SerializeAsString? I'd expect (*lhs.results == *rhs.results) to work, since then you are comparing unordered maps.

Ohhh.... I think I see. It won't work because it will end up comparing the embedded shared_ptr<ProtoType> rather than the pointed-to ProtoType. That's unfortunate.

I think we should prioritize fixing things up around AttributeMap / ProtoType. I think if we do that we will end up back here because I'd expect to see map_to_struct and struct_to_map change.

OK to go ahead with what you have here unless @stuqdog disagrees.

No disagreement here. Agreed also that AttributeMap could use another look. Do you recall offhand if we made a ticket for that already?

No, I don't think we have. Could you make one?

Done. https://viam.atlassian.net/browse/RSDK-6636

src/viam/sdk/tests/test_robot.cpp

…nt-apis

acmorrow · 2024-02-14T16:00:10Z

src/viam/sdk/common/proto_type.hpp

@@ -52,6 +52,7 @@ class ProtoType {

    explicit ProtoType(bool b) : proto_type_(std::move(b)) {}
    explicit ProtoType(std::string s) : proto_type_(std::move(s)) {}
+    explicit ProtoType(const char* c) : proto_type_(std::string(c)) {}


@stuqdog - Our godbolt session exploring this is here: https://godbolt.org/z/KP7EfTaox

stuqdog

Nice! This looks great. There are a couple minor comments/requested changes I left but I don't think they're significant enough to require another round of review after being fixed. Thanks so much for putting this together!

One note, however: tests will now fail in this branch because we've made clang-tidy a bit stricter recently. So you'll need to make a couple changes around const correctness to address those. To avoid having to go through the CI cycle over and over, I suggest invoking cmake with the DVIAMCPPSDK_CLANG_TIDY=ON flag and building so you can see all places that may require fixes.

stuqdog · 2024-02-14T17:13:27Z

src/viam/examples/modules/complex/client.cpp

+        std::cout << "\tname: " << resource.name() << " (type:" << resource.api().resource_type()
+                  << " subtype:" << resource.api().resource_subtype() << ")" << std::endl;


(minor) we can change this to also be std::cout << "\t" << resource << "\n";

stuqdog · 2024-02-14T17:14:01Z

src/viam/examples/motor/example_motor.cpp

+        for (const vs::Name& resource : resource_names) {
+            cout << " - " << resource.name() << " (" << resource.api().resource_subtype() << ")"
+                 << endl;


(minor) here too we can simply << resource directly.

stuqdog · 2024-02-14T17:21:19Z

src/viam/sdk/tests/test_types.cpp

+#include <cctype>
 #include <unordered_map>

 #include <boost/test/included/unit_test.hpp>

 #include <viam/sdk/common/proto_type.hpp>
 #include <viam/sdk/config/resource.hpp>
+#include <viam/sdk/robot/client.hpp>
+#include <viam/sdk/tests/mocks/mock_robot.hpp>


We can probably get rid of these extra includes here.

stuqdog · 2024-02-14T17:23:05Z

src/viam/sdk/tests/test_robot.cpp

-template <typename T>
-// Sorts our vecs and converts to strings for consistent comparisons
-std::vector<std::string> vec_to_string_util(std::vector<T>& vec) {
+std::vector<std::string> name_vec_to_string(std::vector<Name>& vec) {


I still believe we can get rid of this entirely.

stuqdog · 2024-02-14T17:24:26Z

src/viam/sdk/tests/test_robot.cpp

+            std::vector<Name> resource_names = client->resource_names();
+            auto names = name_vec_to_string(resource_names);
            auto mocks = mock_resource_names_response();
-
-            auto mock_resp = vec_to_string_util(mocks);
+            auto mock_resp = name_vec_to_string(mocks);


following up on the above comment, I deleted name_vec_to_string and changed this test to compare resource_names and mocks directly and had no issues with compilation or tests.

stuqdog · 2024-02-14T17:26:29Z

src/viam/sdk/tests/test_robot.cpp

-            // unfortunately the sorting is a bit odd so we end up with a mismatch of index,
-            // but this ensures that the statuses we received do exist in the mocks, as
-            // expected.


stuqdog · 2024-02-14T17:27:55Z

src/viam/sdk/tests/test_robot.cpp


-            BOOST_TEST(some_status_strs == some_mock_strs, boost::test_tools::per_element());
+BOOST_AUTO_TEST_CASE(test_frame_system_config) {


I agree that this test is (sadly) necessary if we want the to_proto methods to be completely hidden but still want verification that the proto and native types have identical values, but can we add a quick comment above just explaining this point?

stuqdog · 2024-02-14T17:32:48Z

src/viam/sdk/tests/mocks/mock_robot.cpp

+    std::vector<RobotClient::status> resp;
    resp.push_back(camera_status);
+    resp.push_back(motor_status);
    resp.push_back(generic_status);
+
+    return resp;


(minor) this can all be reduced to return {std::move(camera_status), std::move(motor_status), std::move(generic_status)};

We should definitely be moving these values, it's a good habit to get into.

stuqdog · 2024-02-14T17:34:27Z

src/viam/sdk/tests/mocks/mock_robot.cpp

-    viam::robot::v1::DiscoveryQuery query;
-    *query.mutable_subtype() = "camera";
-    *query.mutable_model() = "webcam";
+std::vector<RobotClient::discovery> mock_discovery_response() {


I was a bit hasty with my edits to this method, do you mind std::moveing values when they're last used (query at line 88, discovery at line 90, etc.).

stuqdog · 2024-02-14T17:34:55Z

src/viam/sdk/tests/mocks/mock_robot.cpp


-    google::protobuf::Map<std::string, google::protobuf::Value>* map = results.mutable_fields();
-    map->insert(pair);
+        v.push_back(discovery);


Another "me being too hasty" thing, this discovery can be moved :)

acmorrow

LGTM with one optional suggestion / thing to try. I only took a quick look at the mock / test stuff because I think Ethan has a good handle on what is going on there.

This was quite a review but I think the final result looks great, with a much cleaner RobotClient API!

acmorrow · 2024-02-14T22:37:21Z

src/viam/sdk/robot/client.cpp

+}
+
+struct RobotClient::impl {
+    impl(std::unique_ptr<RobotService::Stub> stub) : stub_(std::move(stub)) {}


I still think you can write this as impl(RobotService::Stub&& stub) : stub_(std::move(stub) and get rid of the unique_ptr, but, at the same time, what you have here isn't wrong, it just has an extra level of indirection.

fwiw I get errors when I try to make this change since the RobotService::NewStub method returns a unique_ptr and not a raw Stub type. I can get rid of the unique_ptr by dereferencing it in the constructor but I'm not sure that's better.

I tried this and it gave me errors even though I changed everything. It's a little hard to find the two conversations we had on this since I was asked not to resolve the comments, but I had tried twice before.
I'm guessing it's because of what Ethan just said.

purplenicole730 and others added 11 commits January 23, 2024 12:53

use sdk defined pose in frame

80ae416

define and use sdk defined transform

3990a49

add and use sdk defined discovery and discoveryquery

75b0797

use worldstate::transform and define framesystemconfig

31ff0d7

add kinematics to fsconfig

dc45f0e

use snake case

913e6dd

replace proto ResourceName with Name

2444a3a

Merge branch 'main' into RSDK-4573-remove-proto-types-from-robot-clie…

4e94f16

…nt-apis

fix lint

015c037

use defined status

a975a2f

fix frame test

ec8669f

purplenicole730 commented Feb 2, 2024

View reviewed changes

src/viam/sdk/robot/client.hpp Outdated Show resolved Hide resolved

purplenicole730 added 3 commits February 2, 2024 13:47

define and use operation

9401cf1

use boost::optional

078894f

use string append instead of concatenation

06d1f27

purplenicole730 marked this pull request as ready for review February 2, 2024 20:26

purplenicole730 requested a review from a team as a code owner February 2, 2024 20:26

purplenicole730 requested review from stuqdog, benjirewis and acmorrow and removed request for a team February 2, 2024 20:26

purplenicole730 commented Feb 2, 2024

View reviewed changes

src/viam/sdk/robot/client.hpp Outdated Show resolved Hide resolved

acmorrow requested changes Feb 2, 2024

View reviewed changes

stuqdog requested changes Feb 2, 2024

View reviewed changes

add pr feedback

27f10bd

acmorrow reviewed Feb 6, 2024

View reviewed changes

purplenicole730 added 3 commits February 6, 2024 11:30

add pr feedback

7a217b8

add functions to compare custom type equality

9b42a7a

move custom types into RobotClient

027942f

purplenicole730 added 4 commits February 7, 2024 16:12

compare custom defined operation and discovery in test

bf46ad1

test using custom defined status and frame_system_config

bae7479

test discovery to and from proto methods

044e34d

test robot type to and from proto methods

47b77b0

purplenicole730 requested review from acmorrow and stuqdog February 8, 2024 18:58

stuqdog requested changes Feb 8, 2024

View reviewed changes

acmorrow requested changes Feb 9, 2024

View reviewed changes

purplenicole730 added 6 commits February 9, 2024 18:12

clean up code

204a4a3

fix mock status index confusion

4b04d10

use using lines

10beed3

return copy instead of pointer in resource_names()

77b038e

test transform without proto

76463de

define default_pose in mock robot header file

8cfb56d

acmorrow requested changes Feb 12, 2024

View reviewed changes

purplenicole730 added 3 commits February 12, 2024 13:09

small cleanup in robot client files

7fe501d

make resource_names function const

efa8547

move proto conversion methods to private ns in robot.cpp

6bded4c

purplenicole730 commented Feb 13, 2024

View reviewed changes

src/viam/sdk/tests/test_robot.cpp Outdated Show resolved Hide resolved

stuqdog and others added 2 commits February 14, 2024 10:35

test fixes, add const char* constructor for ProtoType

7ec505e

Merge branch 'main' into RSDK-4573-remove-proto-types-from-robot-clie…

6661375

…nt-apis

purplenicole730 requested review from acmorrow and stuqdog February 14, 2024 15:56

acmorrow reviewed Feb 14, 2024

View reviewed changes

stuqdog approved these changes Feb 14, 2024

View reviewed changes

purplenicole730 added 2 commits February 14, 2024 14:26

clean up unnecessary parts of code

c2a23f2

fix clang tidy error

970e3c4

acmorrow approved these changes Feb 14, 2024

View reviewed changes

purplenicole730 merged commit 1dac2e9 into viamrobotics:main Feb 15, 2024
3 checks passed

purplenicole730 deleted the RSDK-4573-remove-proto-types-from-robot-client-apis branch February 15, 2024 15:36

		viam::robot::v1::DiscoveryQuery to_proto() const;
		static discovery_query from_proto(const viam::robot::v1::DiscoveryQuery& proto);

		@@ -23,6 +23,7 @@ namespace sdk {

		using time_point = std::chrono::time_point<long long, std::chrono::nanoseconds>;

		// TODO: RSDK-6627 : move this function to `Resource` class.

		std::cout << "\tname: " << resource.name() << " (type:" << resource.api().resource_type()
		<< " subtype:" << resource.api().resource_subtype() << ")" << std::endl;


		BOOST_TEST(some_status_strs == some_mock_strs, boost::test_tools::per_element());
		BOOST_AUTO_TEST_CASE(test_frame_system_config) {

RSDK-4573: remove proto types from robot client apis #207

RSDK-4573: remove proto types from robot client apis #207

Conversation

purplenicole730 commented Jan 28, 2024 • edited Loading

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog commented Feb 2, 2024

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purplenicole730 Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purplenicole730 Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purplenicole730 commented Jan 28, 2024 •

edited

Loading

purplenicole730 Feb 10, 2024 •

edited

Loading

stuqdog Feb 12, 2024 •

edited

Loading

purplenicole730 Feb 12, 2024 •

edited

Loading