Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
bbf46f2
first pass navigation hpp
abe-winter Nov 13, 2024
a270b0d
missing include
abe-winter Nov 13, 2024
e338601
green
abe-winter Nov 13, 2024
e8e2aab
checkpoint
abe-winter Nov 13, 2024
4783dd6
proto constructors, unique_ptr
abe-winter Nov 14, 2024
0cb6b02
nav client compiles
abe-winter Nov 14, 2024
2deaf73
nav service compiles, register it
abe-winter Nov 14, 2024
6c8545d
clang format
abe-winter Nov 14, 2024
3b83c1b
extra nav mentions where needed in cmake
abe-winter Nov 14, 2024
f353325
sigh clang-tidy
abe-winter Nov 14, 2024
1f51fa8
vec <-> repeated pointer converters in utils
abe-winter Nov 14, 2024
3960542
use builtin types instead of protos
abe-winter Nov 14, 2024
b7c8cb5
clang format grr
abe-winter Nov 14, 2024
3f7c6c7
add inlines without
abe-winter Nov 14, 2024
d36b9ea
instead of constructor + operator, weird not-quite-sfinae approach to…
abe-winter Nov 14, 2024
2f470be
lint
abe-winter Nov 14, 2024
d334572
clear destination in ptr <-> vec conversions
abe-winter Nov 14, 2024
d640587
actually remove proto headers from nav wrapper
abe-winter Nov 14, 2024
b674f55
nolints
abe-winter Nov 14, 2024
49b210b
clang format
abe-winter Nov 14, 2024
d5e7d3a
get rid of annoying template approach
abe-winter Nov 15, 2024
aa5e1f0
fix comments
abe-winter Nov 15, 2024
359d53b
move proto_utils to private
abe-winter Nov 15, 2024
3def54d
nested namespaces only in cpp17
abe-winter Nov 15, 2024
226d860
remove name param, other sdks don't use it in nav wrappers
abe-winter Nov 16, 2024
83444b3
properties struct instead of returning raw maptype in case we add mor…
abe-winter Nov 16, 2024
305e2a8
mock and test stubs
abe-winter Nov 16, 2024
0128a00
roundtrip test passing
abe-winter Nov 16, 2024
679432c
operator==, fix type on NavServer::DoCommand
abe-winter Nov 16, 2024
6631bc0
test all methods
abe-winter Nov 16, 2024
d4cf2e7
docstrings
abe-winter Nov 16, 2024
c6d5da9
Merge branch 'main' into aw-nav-service
abe-winter Nov 16, 2024
7fa6630
missing const
abe-winter Nov 16, 2024
4e9f32c
x_from_proto -> from_proto
abe-winter Nov 18, 2024
6c26a8c
change mapOver signature
abe-winter Nov 18, 2024
1ae0b1f
return vector instead of unique_ptr
abe-winter Nov 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/viam/api/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,10 @@ if (VIAMCPPSDK_USE_DYNAMIC_PROTOS)
${PROTO_GEN_DIR}/service/motion/v1/motion.grpc.pb.h
${PROTO_GEN_DIR}/service/motion/v1/motion.pb.cc
${PROTO_GEN_DIR}/service/motion/v1/motion.pb.h
${PROTO_GEN_DIR}/service/navigation/v1/navigation.grpc.pb.cc
${PROTO_GEN_DIR}/service/navigation/v1/navigation.grpc.pb.h
${PROTO_GEN_DIR}/service/navigation/v1/navigation.pb.cc
${PROTO_GEN_DIR}/service/navigation/v1/navigation.pb.h
${PROTO_GEN_DIR}/tagger/v1/tagger.grpc.pb.cc
${PROTO_GEN_DIR}/tagger/v1/tagger.grpc.pb.h
${PROTO_GEN_DIR}/tagger/v1/tagger.pb.cc
Expand Down Expand Up @@ -328,6 +332,8 @@ target_sources(viamapi
${PROTO_GEN_DIR}/service/mlmodel/v1/mlmodel.pb.cc
${PROTO_GEN_DIR}/service/motion/v1/motion.grpc.pb.cc
${PROTO_GEN_DIR}/service/motion/v1/motion.pb.cc
${PROTO_GEN_DIR}/service/navigation/v1/navigation.grpc.pb.cc
${PROTO_GEN_DIR}/service/navigation/v1/navigation.pb.cc
${PROTO_GEN_DIR}/tagger/v1/tagger.grpc.pb.cc
${PROTO_GEN_DIR}/tagger/v1/tagger.pb.cc
PUBLIC FILE_SET viamapi_includes TYPE HEADERS
Expand Down Expand Up @@ -385,6 +391,8 @@ target_sources(viamapi
${PROTO_GEN_DIR}/../../viam/api/service/mlmodel/v1/mlmodel.pb.h
${PROTO_GEN_DIR}/../../viam/api/service/motion/v1/motion.grpc.pb.h
${PROTO_GEN_DIR}/../../viam/api/service/motion/v1/motion.pb.h
${PROTO_GEN_DIR}/../../viam/api/service/navigation/v1/navigation.grpc.pb.h
${PROTO_GEN_DIR}/../../viam/api/service/navigation/v1/navigation.pb.h
${PROTO_GEN_DIR}/../../viam/api/tagger/v1/tagger.pb.h
)

Expand Down
4 changes: 4 additions & 0 deletions src/viam/sdk/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,16 @@ target_sources(viamsdk
services/generic.cpp
services/mlmodel.cpp
services/motion.cpp
services/navigation.cpp
services/private/generic_client.cpp
services/private/generic_server.cpp
services/private/mlmodel.cpp
services/private/mlmodel_client.cpp
services/private/mlmodel_server.cpp
services/private/motion_client.cpp
services/private/motion_server.cpp
services/private/navigation_client.cpp
services/private/navigation_server.cpp
services/service.cpp
spatialmath/geometry.cpp
spatialmath/orientation.cpp
Expand Down Expand Up @@ -179,6 +182,7 @@ target_sources(viamsdk
../../viam/sdk/services/generic.hpp
../../viam/sdk/services/mlmodel.hpp
../../viam/sdk/services/motion.hpp
../../viam/sdk/services/navigation.hpp
../../viam/sdk/services/service.hpp
../../viam/sdk/spatialmath/geometry.hpp
../../viam/sdk/spatialmath/orientation.hpp
Expand Down
16 changes: 16 additions & 0 deletions src/viam/sdk/common/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -184,5 +184,21 @@ bool from_dm_from_extra(const ProtoStruct& extra) {
return false;
}

template <typename Src, typename Dst>
void vecToRepeatedPtr(const std::vector<Src>& vec, google::protobuf::RepeatedPtrField<Dst>& dest) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that we've announced this super loudly but we're currently trying to cut back on API and google:: types in headers, so I think we'll want to delete these helper functions even though they're being used a couple times.

As a suggested inline replacement, I think this could just be

dest.Assign(vec.begin(), vec.end());

You can also do that with the Add method, but note that if dest already had stuff then this function would be under-reserving potentially and adding to existing elements which is maybe not what I'd expect from an xToY function
As a suggested replacement for this one, can you just write inline

Copy link
Member Author

Choose a reason for hiding this comment

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

good call -- clearing the destination in d334572 to protect against adding to non-empty collection

I think Assign(begin, end) doesn't work because the types are different -- tried to get it working with constructor/operator implicit conversion approach, couldn't, guessing even harder now that the constructor/operators have been moved out of common headers

dest->Reserve(vec.size());
for (auto& x : vec) {
*dest.Add() = x;
}
}

template <typename Src, typename Dst>
void repeatedPtrToVec(const google::protobuf::RepeatedPtrField<Src>& src, std::vector<Dst>& vec) {
vec.reserve(src.size());
for (auto& x : src) {
vec.push_back(x);
Copy link
Collaborator

Choose a reason for hiding this comment

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

similarly I think this could be vec.assign(src.Begin(), src.End()), also with similar caveats as above about assigning vs appending

}
}

} // namespace sdk
} // namespace viam
10 changes: 10 additions & 0 deletions src/viam/sdk/common/utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,5 +99,15 @@ void set_logger_severity_from_args(int argc, char** argv);
/// @param extra The extra ProtoStruct.
bool from_dm_from_extra(const ProtoStruct& extra);

/// @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);

/// @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);

} // namespace sdk
} // namespace viam
3 changes: 3 additions & 0 deletions src/viam/sdk/registry/registry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@
#include <viam/sdk/services/private/mlmodel_server.hpp>
#include <viam/sdk/services/private/motion_client.hpp>
#include <viam/sdk/services/private/motion_server.hpp>
#include <viam/sdk/services/private/navigation_client.hpp>
#include <viam/sdk/services/private/navigation_server.hpp>
#include <viam/sdk/services/service.hpp>

namespace viam {
Expand Down Expand Up @@ -190,6 +192,7 @@ void register_resources() {
Registry::register_resource<impl::GenericServiceClient, impl::GenericServiceServer>();
Registry::register_resource<impl::MLModelServiceClient, impl::MLModelServiceServer>();
Registry::register_resource<impl::MotionClient, impl::MotionServer>();
Registry::register_resource<impl::NavigationClient, impl::NavigationServer>();
}

void Registry::initialize() {
Expand Down
19 changes: 19 additions & 0 deletions src/viam/sdk/services/navigation.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#include <viam/sdk/services/navigation.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
101 changes: 101 additions & 0 deletions src/viam/sdk/services/navigation.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/// @file services/navigation.hpp
///
/// @brief Defines a `Navigation` service.
#pragma once

#include <string>

#include <viam/api/common/v1/common.pb.h>
#include <viam/api/service/navigation/v1/navigation.pb.h>

#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 {
Waypoint(const viam::service::navigation::v1::Waypoint& proto)
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar to the other comment about keeping API stuff out of headers, @stuqdog is currently working on a PR to take these kinds of conversion functions out of public headers, so maybe you guys can talk so that we have a similar approach in this PR as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

but one option for example is, since I think these are only being used in navigation_client.cpp to just inline them there

Copy link
Member Author

@abe-winter abe-winter Nov 14, 2024

Choose a reason for hiding this comment

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

okay I removed all *.pb.h includes from the wrapper header (navigation.hpp)

how:

  • I moved the implicit conversions for Waypoint and Path to functions in the navigation client/server.cpp files
  • and gave the std::vector <-> RepeatedPointer conversions their own header

now the invocation looks like:

// when destination type has a `from_proto` method, use it:
repeatedPtrToVec(response.obstacles(), *ret);

// otherwise, pass in the conversion function:
repeatedPtrToVec(response.waypoints(), *ret, waypoint_from_proto);

: id(proto.id()), location(geo_point::from_proto(proto.location())) {}

operator viam::service::navigation::v1::Waypoint() {
viam::service::navigation::v1::Waypoint ret;
*ret.mutable_id() = id;
*ret.mutable_location() = location.to_proto();
return ret;
}

std::string id;
geo_point location;
};

struct Path {
Path(const viam::service::navigation::v1::Path& proto)
: destination_waypoint_id(proto.destination_waypoint_id()) {
repeatedPtrToVec(proto.geopoints(), geopoints);
}

operator viam::service::navigation::v1::Path() {
viam::service::navigation::v1::Path ret;
*ret.mutable_destination_waypoint_id() = destination_waypoint_id;
vecToRepeatedPtr(geopoints, *ret.mutable_geopoints());
return ret;
}

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;
Copy link
Member

Choose a reason for hiding this comment

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

Since this is the public facing header, this is the one that should have doc comments. Also, please add blank lines between the end of the declaration of one method and the beginning of the next.

    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;

That, moreso, once comments are added:

    /// Some comment
    virtual LocationResponse get_location(const std::string name, const ProtoStruct& extra) = 0;

    /// Another comment
    virtual std::unique_ptr<std::vector<Waypoint>> get_waypoints(const std::string name,
                                                                 const ProtoStruct& extra) = 0;

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;

protected:
explicit Navigation(std::string name);
};

template <>
struct API::traits<Navigation> {
static API api();
};

} // namespace sdk
} // namespace viam
141 changes: 141 additions & 0 deletions src/viam/sdk/services/private/navigation_client.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
#include <viam/sdk/services/private/navigation_client.hpp>

#include <math.h>

#include <grpcpp/support/status.h>

#include <viam/api/service/navigation/v1/navigation.grpc.pb.h>
#include <viam/api/service/navigation/v1/navigation.pb.h>

#include <viam/sdk/common/client_helper.hpp>
#include <viam/sdk/common/proto_value.hpp>
#include <viam/sdk/common/utils.hpp>
#include <viam/sdk/services/navigation.hpp>

namespace viam {
namespace sdk {
namespace impl {

using namespace viam::service::navigation::v1;

NavigationClient::NavigationClient(std::string name, std::shared_ptr<grpc::Channel> channel)
: Navigation(std::move(name)),
stub_(service::navigation::v1::NavigationService::NewStub(channel)),
channel_(std::move(channel)){};

Navigation::Mode NavigationClient::get_mode(const std::string name, const ProtoStruct& extra) {
return make_client_helper(this, *stub_, &StubType::GetMode)
.with([&](auto& request) {
*request.mutable_name() = name;
*request.mutable_extra() = map_to_struct(extra);
})
.invoke([](auto& response) { return Navigation::Mode(response.mode()); });
}

void NavigationClient::set_mode(const std::string name,
const Navigation::Mode mode,
const ProtoStruct& extra) {
return make_client_helper(this, *stub_, &StubType::SetMode)
.with([&](auto& request) {
*request.mutable_name() = name;
request.set_mode(viam::service::navigation::v1::Mode(mode));
*request.mutable_extra() = map_to_struct(extra);
})
.invoke([](auto& response) {});
}

Navigation::LocationResponse NavigationClient::get_location(const std::string name,
const ProtoStruct& extra) {
return make_client_helper(this, *stub_, &StubType::GetLocation)
.with([&](auto& request) {
*request.mutable_name() = name;
*request.mutable_extra() = map_to_struct(extra);
})
.invoke([](auto& response) {
return Navigation::LocationResponse{
geo_point::from_proto(response.location()),
response.compass_heading(),
};
});
}

std::unique_ptr<std::vector<Navigation::Waypoint>> NavigationClient::get_waypoints(
const std::string name, const ProtoStruct& extra) {
return make_client_helper(this, *stub_, &StubType::GetWaypoints)
.with([&](auto& request) {
*request.mutable_name() = name;
*request.mutable_extra() = map_to_struct(extra);
})
.invoke([](auto& response) {
auto ret = std::make_unique<std::vector<Navigation::Waypoint>>();
repeatedPtrToVec(response.waypoints(), *ret);
return ret;
});
}

void NavigationClient::add_waypoint(const std::string name,
const geo_point& location,
const ProtoStruct& extra) {
return make_client_helper(this, *stub_, &StubType::AddWaypoint)
.with([&](auto& request) {
*request.mutable_name() = name;
*request.mutable_location() = location.to_proto();
*request.mutable_extra() = map_to_struct(extra);
})
.invoke([](auto& response) {});
}

void NavigationClient::remove_waypoint(const std::string name,
const std::string id,
const ProtoStruct& extra) {
return make_client_helper(this, *stub_, &StubType::RemoveWaypoint)
.with([&](auto& request) {
*request.mutable_name() = name;
*request.mutable_id() = id;
*request.mutable_extra() = map_to_struct(extra);
})
.invoke([](auto& response) {});
}

std::unique_ptr<std::vector<geo_geometry>> NavigationClient::get_obstacles(
const std::string name, const ProtoStruct& extra) {
return make_client_helper(this, *stub_, &StubType::GetObstacles)
.with([&](auto& request) {
*request.mutable_name() = name;
*request.mutable_extra() = map_to_struct(extra);
})
.invoke([](auto& response) {
auto ret = std::make_unique<std::vector<geo_geometry>>();
repeatedPtrToVec(response.obstacles(), *ret);
return ret;
});
}

std::unique_ptr<std::vector<NavigationClient::Path>> NavigationClient::get_paths(
const std::string name, const ProtoStruct& extra) {
return make_client_helper(this, *stub_, &StubType::GetPaths)
.with([&](auto& request) {
*request.mutable_name() = name;
*request.mutable_extra() = map_to_struct(extra);
})
.invoke([](auto& response) {
return std::make_unique<std::vector<Path>>(response.paths().begin(),
response.paths().end());
});
}

NavigationClient::MapType NavigationClient::get_properties(const std::string name) {
return make_client_helper(this, *stub_, &StubType::GetProperties)
.with([&](auto& request) { *request.mutable_name() = name; })
.invoke([](auto& response) { return MapType(response.map_type()); });
}

ProtoStruct NavigationClient::do_command(const ProtoStruct& command) {
return make_client_helper(this, *stub_, &StubType::DoCommand)
.with([&](auto& request) { *request.mutable_command() = map_to_struct(command); })
.invoke([](auto& response) { return struct_to_map(response.result()); });
}

} // namespace impl
} // namespace sdk
} // namespace viam
Loading
Loading