Skip to content
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

RSDK6734 - Remove viam/api imports from public headers #325

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

stuqdog
Copy link
Member

@stuqdog stuqdog commented Nov 15, 2024

Moves all(1) proto conversion out of public files and into either the relevant client or server (when specific to a particular resource), or into the new private/proto_conversions.xpp (when used in multiple places). The one exception is private/encoder.xpp, which contains methods only used for encoders, but used by both client and server code.

A lot of this can be skimmed over/ignored, as it's just mechanical moves of methods with straightforward changes (e.g., taking an object of type T as an arg instead of being a method in class T).

Notable changes are as follows (I have tried to add comments indicating where these occur as well):

  • add private/client_helper.hpp. all it does is #include <google/protobuf/struct.pb.h> so that the public client helper doesn't have to.
  • remove id_ field from LinkConfig type. This was never used and was impossible to access. Technically breaking since the shape of a LinkConfig has changed.
  • A new constructors (see ResourceConfig) and getters (see WorldState). Shouldn't be anything controversial, they getters return const& and the constructors just take the data members.
  • Changed get_resource_name in a resource to 1) no longer take a name argument (we were always just using the resource name so we might as well not ask for it), and 2) return a Name instead of a protobuf ResourceName. This is technically breaking though I doubt anyone was relying on this method to return a ResourceName before.

Also a couple flybys here and there (mostly removing unused imports, removed some proto conversion methods that were unused, removing an unused stub created in the dial method).

(1) technically viam/api imports still exist for robot_server.hpp and module_service.hpp. Given that these are both explicitly defining an implementation of a proto type, and that we want these to be public, I question whether removing the viam/api imports from these is possible or desirable.

@stuqdog stuqdog marked this pull request as ready for review November 15, 2024 19:19
@stuqdog stuqdog requested a review from a team as a code owner November 15, 2024 19:19
@stuqdog stuqdog requested review from purplenicole730 and lia-viam and removed request for a team November 15, 2024 19:19
Copy link
Member Author

Choose a reason for hiding this comment

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

Note the existence of this is only to hide the google/protobuf import from the public client_helper.hpp

Comment on lines +22 to +28
ResourceConfig(std::string type,
std::string name,
std::string namespace_,
ProtoStruct attributes,
std::string api,
Model model,
LinkConfig frame);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note the new constructor here. It's just taking the component parts.

Comment on lines +25 to +26
const std::vector<geometries_in_frame>& obstacles() const;
const std::vector<transform>& transforms() const;
Copy link
Member Author

Choose a reason for hiding this comment

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

note the new getters here

Comment on lines +25 to +26
/// @brief Returns the `Name` for a particular resource.
virtual Name get_resource_name() const;
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the get_resource_method name here has changed to give us our own Name type rather than the proto ResourceName. We can use the conversion methods from that Name when necessary.

translation get_translation() const;
OrientationConfig get_orientation_config() const;
GeometryConfig get_geometry_config() const;
std::string get_parent() const;

private:
std::string id_;
Copy link
Member Author

Choose a reason for hiding this comment

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

Note the removal of this id_ field, which was unused and inaccessible.

@lia-viam
Copy link
Contributor

So I think there's a lot here we'll want to use going forward but also some decisions which I think we might want to give some more thought before jumping into. My short term recommendation would be that probably the changes to viam/sdk/components and class Resource could be factored into their own PR more or less as-is, but some of the other stuff we should hold on to for now--I think we'll be glad of having everything in a single header/source file (and out of public headers) as an intermediate step to build on for future work, but I'm not sure it makes sense for the end user.

Some further thoughts to elaborate:

  • I'm not sure that one giant proto conversions header makes the most sense from a user experience point of view since it's kind of giant and might be harder to find what you're looking for, and also involves pulling in a lot of potentially not relevant stuff, eg if I want to get some conversions for X component, I'm also pulling in everything to do with module service, spatial math, etc
  • I think we want to go with the pointer-insulated form of conversions used in proto_value.hpp rather than more functional return-by-value or methods, eg
// forward declare
namespace viam::api::v1 { class SomeComponent; }

void to_proto(const SomeComponent& source, viam::api::v1::SomeComponent* dest)

We probably wan to distinguish our approaches between

  1. single-use conversion functions that probably won't be needed outside of a components/private. For example arm_client.cpp needs to convert a Arm::KinematicsData to and from proto, but in the universal robots arm module this is not used
  2. conversion functions that may be used multiple places or be of interest to end users, possibly stuff in sdk/common, spatialmath components, etc

I think the first case is more insulated and well understood but there's more open ended stuff for the second case. In the second case I think we might end up with something that looks like the forward declaration and pointer-based conversion function above appearing in some_component.hpp, an implementation in some_component.cpp, and then proto_conversions.hpp might just have some template metaprogramming based helpers that will detect a void to_proto(const X& src, Y* dst) and let you call it as to_proto(X)

@stuqdog
Copy link
Member Author

stuqdog commented Nov 18, 2024

Cool, thanks for the comprehensive reply! I think it might be good to talk non-async on this since there's probably going to be a fair amount of back and forth, but one quick high level clarification that I think will help drive that conversation:

I'm not sure that one giant proto conversions header makes the most sense from a user experience point of view since it's kind of giant and might be harder to find what you're looking for, and also involves pulling in a lot of potentially not relevant stuff, eg if I want to get some conversions for X component, I'm also pulling in everything to do with module service, spatial math, etc

So as I understand it, a major goal here is that from a user experience point of view, proto no longer exists. The proto conversions header is intentionally private private specifically because a user should never have to do proto conversions with any of our internal types. So I'm not sure we should be worrying about how a user finds this, or including any of this in some_component.xpp at all.

@lia-viam
Copy link
Contributor

@stuqdog we can definitely talk non-async! Also going to cc @acmorrow if he has time to weigh in, but regarding the "proto no longer exists" point, I think the goal is rather that proto should not comprise any part of our API nor ABI, so that

  • it no longer has to be a public, transitive dependency of the SDK
  • we're not at the whims of all the horrible stuff google does

That said, in some cases it's possible that users will still want to access proto types directly as an escape hatch for some advanced use case not covered by the API surface we provide, and if so they can make proto a public dependency of their module, #include the relevant types, and toil away

@stuqdog stuqdog changed the title RSDK6734 - RSDK-6634 - remove viam/api imports from public headers RSDK6734 - Remove viam/api imports from public headers Nov 19, 2024
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