RSDK-3589 add wrapper for navigation service #323
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| /// @file common/proto_utils.hpp | ||
| /// | ||
| /// @brief Utils that require generated proto includes. These should be #included | ||
| /// in cpp implementation files, but not in wrapper headers consumed by third party code. | ||
| #pragma once | ||
|
|
||
| #include <viam/api/common/v1/common.pb.h> | ||
|
|
||
| namespace viam { | ||
| namespace sdk { | ||
|
|
||
|
Member
There was a problem hiding this comment. add |
||
| /// @brief Copies elements from a protobuf repeated pointer array into a std::vector. Src type | ||
| /// must be implicitly convertible to Dst (probably via operator on Src). | ||
| template <typename Src, typename Dst> | ||
| void vecToRepeatedPtr(const std::vector<Src>& vec, google::protobuf::RepeatedPtrField<Dst>& dest) { | ||
|
Member
There was a problem hiding this comment. I have a slight preference for out parameters being taken by pointer, as it makes it clearer at the call site that the field is likely being mutated: vs Others dislike this because they worry that it allows dest to be nullptr. I don't find that compelling because requiring it to be a reference doesn't prevent the opposite mistake:
Collaborator
There was a problem hiding this comment. So generally speaking I'm very much against output parameters being passed as pointers, it's one of the things I dislike the most about google's C++ coding guidelines. I think with your counterexample above the point is that, in C++ and as high level as this code is, we generally aren't passing around pointer types in the first place, and the point of references is that they must point to a valid object or else it's UB. So as for
That's on the caller for dereferencing a null pointer in the first place, and is equally true of if Callsite legibility I'm somewhat sympathetic to but would be better done with something like .....All that said, I think this could be a pointer parameter just for ABI insulation reasons |
||
| dest.Clear(); | ||
| dest.Reserve(vec.size()); | ||
| for (auto& x : vec) { | ||
| *dest.Add() = x.to_proto(); | ||
| } | ||
| } | ||
|
|
||
| /// @brief Non-member to_proto() version. (necessary for moving generated types out of wrapper | ||
| /// headers). Template param F is a function that converts from Src to Dst. | ||
| template <typename Src, typename Dst> | ||
|
Member
There was a problem hiding this comment. More idiomatic would just to be take Because then you can pass any sort of callable (std::function, lambda, function pointer as you have now). You can then enforce the shape of the callable with a static assert or similar.
Member
Author
There was a problem hiding this comment. explain this one -- this is better because it would select this specialization and return a static assert error, rather than giving a long 'no matching template' error?
Member
There was a problem hiding this comment. I think, as written, yours will only accept function pointers. I could be wrong, I'd need to test it out on godbolt. |
||
| void vecToRepeatedPtr(const std::vector<Src>& vec, | ||
| google::protobuf::RepeatedPtrField<Dst>& dest, | ||
| Dst to_proto(const Src&)) { | ||
| dest.Clear(); | ||
| dest.Reserve(vec.size()); | ||
| for (auto& x : vec) { | ||
|
Member
There was a problem hiding this comment. This would work with other containers than just std::vector right? std::array, std::list, etc.? |
||
| *dest.Add() = to_proto(x); | ||
| } | ||
| } | ||
|
|
||
| /// @brief Copies elements from a std::vector into a protobuf repeated pointer array. Src type | ||
| /// must be implicitly convertible to Dst (probably via constructor on Dst). | ||
| template <typename Src, typename Dst> | ||
| void repeatedPtrToVec(const google::protobuf::RepeatedPtrField<Src>& src, std::vector<Dst>& vec) { | ||
| vec.clear(); | ||
| vec.reserve(src.size()); | ||
| for (auto& x : src) { | ||
| vec.push_back(Dst::from_proto(x)); | ||
| } | ||
| } | ||
|
|
||
| /// @brief Non-member to_proto() version. (necessary for moving generated types out of wrapper | ||
| /// headers). Template param F is a function that converts from Src to Dst. | ||
| template <typename Src, typename Dst> | ||
| void repeatedPtrToVec(const google::protobuf::RepeatedPtrField<Src>& src, | ||
| std::vector<Dst>& vec, | ||
| Dst from_proto(const Src&)) { | ||
|
Member
There was a problem hiding this comment. You could eliminate the distinction between the converting and non-converting by just always taking a transformation function and defaulting it to std::identity (might need to get it from boost for now) That actually has me wondering whether all of these are really glosses on std::transform and would be better implemented that way? |
||
| vec.clear(); | ||
| vec.reserve(src.size()); | ||
| for (auto& x : src) { | ||
| vec.push_back(from_proto(x)); | ||
| } | ||
| } | ||
|
|
||
| } // namespace sdk | ||
| } // namespace viam | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| #include <viam/sdk/services/navigation.hpp> | ||
|
|
||
| #include <viam/api/service/navigation/v1/navigation.pb.h> | ||
| #include <viam/sdk/common/proto_utils.hpp> | ||
| #include <viam/sdk/common/utils.hpp> | ||
|
|
||
| namespace viam { | ||
| namespace sdk { | ||
|
|
||
| Navigation::Navigation(std::string name) : Service(std::move(name)){}; | ||
|
|
||
| API Navigation::api() const { | ||
| return API::get<Navigation>(); | ||
| } | ||
|
|
||
| API API::traits<Navigation>::api() { | ||
| return {kRDK, kService, "navigation"}; | ||
| } | ||
|
|
||
| } // namespace sdk | ||
| } // namespace viam |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| /// @file services/navigation.hpp | ||
| /// | ||
| /// @brief Defines a `Navigation` service. | ||
| #pragma once | ||
|
|
||
| #include <string> | ||
|
|
||
| #include <viam/sdk/common/pose.hpp> | ||
| #include <viam/sdk/common/utils.hpp> | ||
| #include <viam/sdk/services/service.hpp> | ||
| #include <viam/sdk/spatialmath/geometry.hpp> | ||
|
|
||
| namespace viam { | ||
| namespace sdk { | ||
|
|
||
| class Navigation : public Service { | ||
| public: | ||
| enum class Mode : uint8_t { | ||
| k_unspecified, | ||
| k_manual, | ||
| k_waypoint, | ||
| k_explore, | ||
| }; | ||
|
|
||
| enum class MapType : uint8_t { | ||
| k_unspecified, | ||
| k_none, | ||
| k_gps, | ||
| }; | ||
|
|
||
| struct LocationResponse { | ||
| geo_point location; | ||
| double compass_heading; | ||
| }; | ||
|
|
||
| struct Waypoint { | ||
| std::string id; | ||
| geo_point location; | ||
| }; | ||
|
|
||
| struct Path { | ||
| std::string destination_waypoint_id; | ||
| std::vector<geo_point> geopoints; | ||
| }; | ||
|
|
||
| API api() const override; | ||
|
|
||
| virtual Mode get_mode(const std::string name, const ProtoStruct& extra) = 0; | ||
abe-winter marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| virtual void set_mode(const std::string name, const Mode mode, const ProtoStruct& extra) = 0; | ||
| virtual LocationResponse get_location(const std::string name, const ProtoStruct& extra) = 0; | ||
| virtual std::unique_ptr<std::vector<Waypoint>> get_waypoints(const std::string name, | ||
| const ProtoStruct& extra) = 0; | ||
| virtual void add_waypoint(const std::string name, | ||
| const geo_point& location, | ||
| const ProtoStruct& extra) = 0; | ||
| virtual void remove_waypoint(const std::string name, | ||
| const std::string id, | ||
| const ProtoStruct& extra) = 0; | ||
| virtual std::unique_ptr<std::vector<geo_geometry>> get_obstacles(const std::string name, | ||
| const ProtoStruct& extra) = 0; | ||
| virtual std::unique_ptr<std::vector<Path>> get_paths(const std::string name, | ||
| const ProtoStruct& extra) = 0; | ||
| virtual MapType get_properties(const std::string) = 0; | ||
| virtual ProtoStruct do_command(const ProtoStruct& command) = 0; | ||
|
|
||
| // overloads without `extra` param. | ||
| inline Mode get_mode(const std::string name) { | ||
| return get_mode(name, {}); | ||
| } | ||
| inline void set_mode(const std::string name, const Mode mode) { | ||
| set_mode(name, mode, {}); | ||
| } | ||
| inline LocationResponse get_location(const std::string name) { | ||
| return get_location(name, {}); | ||
| } | ||
| inline std::unique_ptr<std::vector<Waypoint>> get_waypoints(const std::string name) { | ||
| return get_waypoints(name, {}); | ||
| } | ||
| inline void add_waypoint(const std::string name, const geo_point& location) { | ||
| add_waypoint(name, location, {}); | ||
| } | ||
| inline void remove_waypoint(const std::string name, const std::string id) { | ||
| remove_waypoint(name, id, {}); | ||
| } | ||
| inline std::unique_ptr<std::vector<geo_geometry>> get_obstacles(const std::string name) { | ||
| return get_obstacles(name, {}); | ||
| } | ||
| inline std::unique_ptr<std::vector<Path>> get_paths(const std::string name) { | ||
| return get_paths(name, {}); | ||
| } | ||
|
|
||
| protected: | ||
| explicit Navigation(std::string name); | ||
| }; | ||
|
|
||
| template <> | ||
| struct API::traits<Navigation> { | ||
| static API api(); | ||
| }; | ||
|
|
||
| } // namespace sdk | ||
| } // namespace viam | ||
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.
A semi-near-term goal for the C++ SDK is to move all the proto stuff out of public headers. Could you push this down into
viam/sdk/common/private/proto_utils.hpp? That's how we indicate that files shouldn't be consumed except by files that form part of the SDK or published as part of the install step.