diff --git a/src/viam/sdk/common/client_helper.cpp b/src/viam/sdk/common/client_helper.cpp index a6853a748..dc38fd3e0 100644 --- a/src/viam/sdk/common/client_helper.cpp +++ b/src/viam/sdk/common/client_helper.cpp @@ -23,6 +23,8 @@ bool isStatusCancelled(int status) noexcept { return status == ::grpc::StatusCode::CANCELLED; } +void set_name(...) {} // NOLINT(cert-dcl50-cpp) + } // namespace client_helper_details ClientContext::ClientContext() : wrapped_context_(std::make_unique()) { diff --git a/src/viam/sdk/common/client_helper.hpp b/src/viam/sdk/common/client_helper.hpp index 2fb918c55..d5e3a8240 100644 --- a/src/viam/sdk/common/client_helper.hpp +++ b/src/viam/sdk/common/client_helper.hpp @@ -15,6 +15,19 @@ namespace client_helper_details { // Helper function to test equality of status with grpc::StatusCode::CANCELLED. bool isStatusCancelled(int status) noexcept; +// Set the mutable name of a request to the client name. +// This function only participates in overload resolution if the request has a mutable_name method. +template +void set_name(RequestType* req, const ClientType* client) { + *req->mutable_name() = client->name(); +} + +// No-op version of set_name above. This overload is only selected if the request type does not have +// a mutable_name field. +void set_name(...); + } // namespace client_helper_details // the authority on a grpc::ClientContext is sometimes set to an invalid uri on mac, causing @@ -96,7 +109,7 @@ class ClientHelper { template auto invoke(ResponseHandlerCallable&& rhc, ErrorHandlerCallable&& ehc) { - *request_.mutable_name() = client_->name(); + client_helper_details::set_name(&request_, client_); ClientContext ctx; if (debug_key_ != "") { diff --git a/src/viam/sdk/log/logging.cpp b/src/viam/sdk/log/logging.cpp index 910636e80..b65764d21 100644 --- a/src/viam/sdk/log/logging.cpp +++ b/src/viam/sdk/log/logging.cpp @@ -133,11 +133,15 @@ void LogManager::init_logging() { console_sink_ = boost::make_shared< boost::log::sinks::synchronous_sink>(backend); - console_sink_->set_filter(Filter{this}); + boost::log::core::get()->add_sink(console_sink_); + console_sink_->set_formatter(fmt); + enable_console_logging(); +} - boost::log::core::get()->add_sink(console_sink_); - VIAM_SDK_LOG(debug) << "Initialized console logging"; +void LogManager::enable_console_logging() { + console_sink_->set_filter(Filter{this}); + VIAM_SDK_LOG(debug) << "Console logging enabled"; } void LogManager::disable_console_logging() { diff --git a/src/viam/sdk/log/logging.hpp b/src/viam/sdk/log/logging.hpp index aa86c40d2..657088eb4 100644 --- a/src/viam/sdk/log/logging.hpp +++ b/src/viam/sdk/log/logging.hpp @@ -113,6 +113,7 @@ class LogManager { LogManager& operator=(LogManager&&) = delete; void init_logging(); + void enable_console_logging(); void disable_console_logging(); LogSource sdk_logger_; diff --git a/src/viam/sdk/robot/client.cpp b/src/viam/sdk/robot/client.cpp index 6e172ac24..740c50fff 100644 --- a/src/viam/sdk/robot/client.cpp +++ b/src/viam/sdk/robot/client.cpp @@ -46,31 +46,36 @@ using viam::robot::v1::RobotService; // NOLINTNEXTLINE const std::string kStreamRemoved("Stream removed"); -RobotClient::frame_system_config from_proto(const FrameSystemConfig& proto) { +namespace proto_convert_details { + +RobotClient::frame_system_config from_proto_impl::operator()( + const FrameSystemConfig* proto) const { RobotClient::frame_system_config fsconfig; - fsconfig.frame = from_proto(proto.frame()); - if (proto.has_kinematics()) { - fsconfig.kinematics = from_proto(proto.kinematics()); + fsconfig.frame = from_proto(proto->frame()); + if (proto->has_kinematics()) { + fsconfig.kinematics = from_proto(proto->kinematics()); } return fsconfig; } -RobotClient::operation from_proto(const Operation& proto) { +RobotClient::operation from_proto_impl::operator()(const Operation* proto) const { RobotClient::operation op; - op.id = proto.id(); - op.method = proto.method(); - if (proto.has_session_id()) { - op.session_id = proto.session_id(); + op.id = proto->id(); + op.method = proto->method(); + if (proto->has_session_id()) { + op.session_id = proto->session_id(); } - if (proto.has_arguments()) { - op.arguments = from_proto(proto.arguments()); + if (proto->has_arguments()) { + op.arguments = from_proto(proto->arguments()); } - if (proto.has_started()) { - op.started = from_proto(proto.started()); + if (proto->has_started()) { + op.started = from_proto(proto->started()); } return op; } +} // namespace proto_convert_details + bool operator==(const RobotClient::frame_system_config& lhs, const RobotClient::frame_system_config& rhs) { return lhs.frame == rhs.frame && to_proto(lhs.kinematics).SerializeAsString() == @@ -83,16 +88,28 @@ bool operator==(const RobotClient::operation& lhs, const RobotClient::operation& } struct RobotClient::impl { - impl(std::unique_ptr stub) : stub_(std::move(stub)) {} + impl(std::unique_ptr stub) : stub(std::move(stub)) {} ~impl() { if (log_sink) { boost::log::core::get()->remove_sink(log_sink); + LogManager::get().enable_console_logging(); + } + } + + template + static auto client_helper(const std::unique_ptr& self, Method m) { + if (!self) { + throw std::runtime_error( + "Tried to call RobotClient method while not connected to robot"); } + return make_client_helper(self.get(), *self->stub, m); } - std::unique_ptr stub_; + std::unique_ptr stub; + // See doc comment for RobotClient::connect_logging. This pointer is non-null and installed as a + // sink only for apps being run by viam-server as a module. boost::shared_ptr log_sink; }; @@ -116,28 +133,27 @@ void RobotClient::connect_logging() { } RobotClient::~RobotClient() { - if (should_close_channel_) { - try { - this->close(); - } catch (const std::exception& e) { - VIAM_SDK_LOG(error) << "Received err while closing RobotClient: " << e.what(); - } catch (...) { - VIAM_SDK_LOG(error) << "Received unknown err while closing RobotClient"; - } + try { + this->close(); + } catch (const std::exception& e) { + VIAM_SDK_LOG(error) << "Received err while closing RobotClient: " << e.what(); + } catch (...) { + VIAM_SDK_LOG(error) << "Received unknown err while closing RobotClient"; } } void RobotClient::close() { should_refresh_.store(false); - for (auto& thread : threads_) { - thread.join(); + if (refresh_thread_.joinable()) { + refresh_thread_.join(); } - threads_.clear(); stop_all(); viam_channel_.close(); + + impl_.reset(); } bool is_error_response(const grpc::Status& response) { @@ -145,62 +161,32 @@ bool is_error_response(const grpc::Status& response) { } std::vector RobotClient::get_operations() { - const viam::robot::v1::GetOperationsRequest req; - viam::robot::v1::GetOperationsResponse resp; - ClientContext ctx; - - std::vector operations; - - grpc::Status const response = impl_->stub_->GetOperations(ctx, req, &resp); - if (is_error_response(response)) { - VIAM_SDK_LOG(error) << "Error getting operations: " << response.error_message(); - } - - for (int i = 0; i < resp.operations().size(); ++i) { - // NOLINTNEXTLINE - operations.push_back(from_proto(resp.operations().at(i))); - } - return operations; + return impl::client_helper(impl_, &RobotService::Stub::GetOperations) + .invoke( + [](auto& response) { return sdk::impl::from_repeated_field(response.operations()); }); } void RobotClient::cancel_operation(std::string id) { - viam::robot::v1::CancelOperationRequest req; - viam::robot::v1::CancelOperationResponse resp; - ClientContext ctx; - - req.set_id(id); - const grpc::Status response = impl_->stub_->CancelOperation(ctx, req, &resp); - if (is_error_response(response)) { - VIAM_SDK_LOG(error) << "Error canceling operation with id " << id; - } + return impl::client_helper(impl_, &RobotService::Stub::CancelOperation) + .with([&id](auto& req) { req.set_id(std::move(id)); }) + .invoke(); } void RobotClient::block_for_operation(std::string id) { - viam::robot::v1::BlockForOperationRequest req; - viam::robot::v1::BlockForOperationResponse resp; - ClientContext ctx; - - req.set_id(id); - - const grpc::Status response = impl_->stub_->BlockForOperation(ctx, req, &resp); - if (is_error_response(response)) { - VIAM_SDK_LOG(error) << "Error blocking for operation with id " << id; - } + return impl::client_helper(impl_, &RobotService::Stub::BlockForOperation) + .with([&id](auto& req) { req.set_id(std::move(id)); }) + .invoke(); } void RobotClient::refresh() { - const viam::robot::v1::ResourceNamesRequest req; - viam::robot::v1::ResourceNamesResponse resp; - ClientContext ctx; - - const grpc::Status response = impl_->stub_->ResourceNames(ctx, req, &resp); - if (is_error_response(response)) { - VIAM_SDK_LOG(error) << "Error getting resource names: " << response.error_message(); - } + auto resources = + impl::client_helper(impl_, &RobotService::Stub::ResourceNames).invoke([](auto& response) { + return response.resources(); + }); std::unordered_map> new_resources; std::vector current_resources; - for (const auto& name : resp.resources()) { + for (const auto& name : resources) { current_resources.push_back(from_proto(name)); if (name.subtype() == "remote") { continue; @@ -224,28 +210,19 @@ void RobotClient::refresh() { } } } - bool is_equal = current_resources.size() == resource_names_.size(); - if (is_equal) { - for (size_t i = 0; i < resource_names_.size(); ++i) { - if (!(resource_names_.at(i) == current_resources.at(i))) { - is_equal = false; - break; - } - } - } - if (is_equal) { + if (current_resources == resource_names_) { return; } const std::lock_guard lock(lock_); - resource_names_ = current_resources; + resource_names_ = std::move(current_resources); this->resource_manager_.replace_all(new_resources); } void RobotClient::refresh_every() { while (should_refresh_.load()) { try { - std::this_thread::sleep_for(std::chrono::seconds(refresh_interval_)); + std::this_thread::sleep_for(refresh_interval_); refresh(); } catch (std::exception&) { @@ -256,7 +233,6 @@ void RobotClient::refresh_every() { RobotClient::RobotClient(ViamChannel channel) : viam_channel_(std::move(channel)), - should_close_channel_(false), impl_(std::make_unique(RobotService::NewStub(viam_channel_.channel()))) {} std::vector RobotClient::resource_names() const { @@ -268,6 +244,11 @@ void RobotClient::log(const std::string& name, const std::string& level, const std::string& message, time_pt time) { + if (!impl_) { + throw std::runtime_error("Tried to send logs to robot when it was not connected"); + } + + // Do client request/response setup manually so we can override the usual exception handling robot::v1::LogRequest req; common::v1::LogEntry log; @@ -279,7 +260,7 @@ void RobotClient::log(const std::string& name, robot::v1::LogResponse resp; ClientContext ctx; - const auto response = impl_->stub_->Log(ctx, req, &resp); + const auto response = impl_->stub->Log(ctx, req, &resp); if (is_error_response(response)) { // Manually override to force this to get logged to console so we don't set off an infinite // loop @@ -292,10 +273,10 @@ void RobotClient::log(const std::string& name, std::shared_ptr RobotClient::with_channel(ViamChannel channel, const Options& options) { auto robot = std::make_shared(std::move(channel)); - robot->refresh_interval_ = options.refresh_interval(); - robot->should_refresh_ = (robot->refresh_interval_ > 0); + robot->refresh_interval_ = std::chrono::seconds{options.refresh_interval()}; + robot->should_refresh_ = (robot->refresh_interval_ > std::chrono::seconds{0}); if (robot->should_refresh_) { - robot->threads_.emplace_back(&RobotClient::refresh_every, robot); + robot->refresh_thread_ = std::thread{&RobotClient::refresh_every, robot.get()}; } robot->refresh(); @@ -307,7 +288,6 @@ std::shared_ptr RobotClient::at_address(const std::string& address, const char* uri = address.c_str(); auto robot = RobotClient::with_channel(ViamChannel::dial_initial(uri, options.dial_options()), options); - robot->should_close_channel_ = true; return robot; }; @@ -318,53 +298,33 @@ std::shared_ptr RobotClient::at_local_socket(const std::string& add auto robot = RobotClient::with_channel( ViamChannel(sdk::impl::create_viam_channel(addr, grpc::InsecureChannelCredentials())), options); - robot->should_close_channel_ = true; return robot; }; std::vector RobotClient::get_frame_system_config( const std::vector& additional_transforms) { - viam::robot::v1::FrameSystemConfigRequest req; - viam::robot::v1::FrameSystemConfigResponse resp; - ClientContext ctx; - - *(req.mutable_supplemental_transforms()) = sdk::impl::to_repeated_field(additional_transforms); - - const grpc::Status response = impl_->stub_->FrameSystemConfig(ctx, req, &resp); - if (is_error_response(response)) { - VIAM_SDK_LOG(error) << "Error getting frame system config: " << response.error_message(); - } - - const RepeatedPtrField configs = resp.frame_system_configs(); - - std::vector fs_configs = std::vector(); - - for (const FrameSystemConfig& fs : configs) { - fs_configs.push_back(from_proto(fs)); - } - - return fs_configs; + return impl::client_helper(impl_, &RobotService::Stub::FrameSystemConfig) + .with([&additional_transforms](auto& req) { + *(req.mutable_supplemental_transforms()) = + sdk::impl::to_repeated_field(additional_transforms); + }) + .invoke( + [](auto& resp) { return sdk::impl::from_repeated_field(resp.frame_system_configs()); }); } pose_in_frame RobotClient::transform_pose( const pose_in_frame& query, std::string destination, const std::vector& additional_transforms) { - viam::robot::v1::TransformPoseRequest req; - viam::robot::v1::TransformPoseResponse resp; - ClientContext ctx; - - *req.mutable_source() = to_proto(query); - *req.mutable_destination() = std::move(destination); - *req.mutable_supplemental_transforms() = sdk::impl::to_repeated_field(additional_transforms); - - const grpc::Status response = impl_->stub_->TransformPose(ctx, req, &resp); - if (is_error_response(response)) { - VIAM_SDK_LOG(error) << "Error getting PoseInFrame: " << response.error_message(); - } - - return from_proto(resp.pose()); + return impl::client_helper(impl_, &RobotService::Stub::TransformPose) + .with([&](auto& req) { + *req.mutable_source() = to_proto(query); + *req.mutable_destination() = std::move(destination); + *req.mutable_supplemental_transforms() = + sdk::impl::to_repeated_field(additional_transforms); + }) + .invoke([](const auto& resp) { return from_proto(resp.pose()); }); } std::shared_ptr RobotClient::resource_by_name(const Name& name) { @@ -380,25 +340,20 @@ void RobotClient::stop_all() { } void RobotClient::stop_all(const std::unordered_map& extra) { - viam::robot::v1::StopAllRequest req; - viam::robot::v1::StopAllResponse resp; - ClientContext ctx; - - RepeatedPtrField* ep = req.mutable_extra(); - for (const auto& xtra : extra) { - const Name& name = xtra.first; - const ProtoStruct& params = xtra.second; - const google::protobuf::Struct s = to_proto(params); - viam::robot::v1::StopExtraParameters stop; - *stop.mutable_name() = to_proto(name); - *stop.mutable_params() = s; - *ep->Add() = stop; - } - const grpc::Status response = impl_->stub_->StopAll(ctx, req, &resp); - if (is_error_response(response)) { - VIAM_SDK_LOG(error) << "Error stopping all: " << response.error_message() - << response.error_details(); - } + return impl::client_helper(impl_, &RobotService::Stub::StopAll) + .with([&](auto& req) { + RepeatedPtrField* ep = req.mutable_extra(); + for (const auto& xtra : extra) { + const Name& name = xtra.first; + const ProtoStruct& params = xtra.second; + const google::protobuf::Struct s = to_proto(params); + viam::robot::v1::StopExtraParameters stop; + *stop.mutable_name() = to_proto(name); + *stop.mutable_params() = s; + *ep->Add() = stop; + } + }) + .invoke(); } std::ostream& operator<<(std::ostream& os, const RobotClient::status& v) { @@ -419,24 +374,18 @@ std::ostream& operator<<(std::ostream& os, const RobotClient::status& v) { } RobotClient::status RobotClient::get_machine_status() const { - const robot::v1::GetMachineStatusRequest req; - robot::v1::GetMachineStatusResponse resp; - ClientContext ctx; - - const grpc::Status response = impl_->stub_->GetMachineStatus(ctx, req, &resp); - if (is_error_response(response)) { - VIAM_SDK_LOG(error) << "Error getting machine status: " << response.error_message() - << response.error_details(); - } - switch (resp.state()) { - case robot::v1::GetMachineStatusResponse_State_STATE_INITIALIZING: - return RobotClient::status::k_initializing; - case robot::v1::GetMachineStatusResponse_State_STATE_RUNNING: - return RobotClient::status::k_running; - case robot::v1::GetMachineStatusResponse_State_STATE_UNSPECIFIED: - default: - return RobotClient::status::k_unspecified; - } + return impl::client_helper(impl_, &RobotService::Stub::GetMachineStatus) + .invoke([](const auto& resp) { + switch (resp.state()) { + case robot::v1::GetMachineStatusResponse_State_STATE_INITIALIZING: + return RobotClient::status::k_initializing; + case robot::v1::GetMachineStatusResponse_State_STATE_RUNNING: + return RobotClient::status::k_running; + case robot::v1::GetMachineStatusResponse_State_STATE_UNSPECIFIED: + default: + return RobotClient::status::k_unspecified; + } + }); } } // namespace sdk diff --git a/src/viam/sdk/robot/client.hpp b/src/viam/sdk/robot/client.hpp index 1c2e325d6..3c19a9edc 100644 --- a/src/viam/sdk/robot/client.hpp +++ b/src/viam/sdk/robot/client.hpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -18,6 +19,16 @@ #include namespace viam { + +namespace robot { +namespace v1 { + +class FrameSystemConfig; +class Operation; + +} // namespace v1 +} // namespace robot + namespace sdk { namespace impl { @@ -32,10 +43,6 @@ struct LogBackend; /// - `RobotClient::at_address(...)` /// - `RobotClient::with_channel(...)` /// @ingroup Robot -/// -/// You must `close()` a robot when finished with it in order to release its resources. -/// Robots creates via `at_address` will automatically close, but robots created via -/// `with_channel` require a user call to `close()`. class RobotClient { public: /// @enum status @@ -68,7 +75,17 @@ class RobotClient { ~RobotClient(); + /// @brief Call out to the robot to see if there are any new resources that need to be + /// registered. Compares the currently registered resources to the ones from the robot, seeing + /// if any updates have been made. If so, they are registered and updated in @ref + /// ResourceManager. This method can be called manually, or it will be called periodically and + /// automatically if a positive refresh_interval is passed in the Options of the named + /// constructors. void refresh(); + + /// @brief Disconnect this robot client from any robot to which it is connected. + /// After calling this method it is no longer valid to call any methods which communicate with + /// the robot. void close(); /// @brief Create a robot client connected to the robot at the provided address. @@ -88,8 +105,6 @@ class RobotClient { /// @brief Creates a robot client connected to the provided channel. /// @param channel The channel to connect with. /// @param options Options for connecting and refreshing. - /// Connects directly to a pre-existing channel. A robot created this way must be - /// `close()`d manually. static std::shared_ptr with_channel(ViamChannel channel, const Options& options); std::vector resource_names() const; @@ -161,17 +176,20 @@ class RobotClient { const std::string& message, time_pt time); + // Makes this RobotClient manage logging by sending logs over grpc to viam-server. + // This is private and only ever called by ModuleService; in other words it is only called when + // viam-server is running a Viam C++ SDK application as a module. + // Disables console logging so as to avoid log message duplication; console logging is + // re-enabled on destruction. void connect_logging(); void refresh_every(); - std::vector threads_; - + std::thread refresh_thread_; std::atomic should_refresh_; - unsigned int refresh_interval_; + std::chrono::seconds refresh_interval_; ViamChannel viam_channel_; - bool should_close_channel_; struct impl; std::unique_ptr impl_; @@ -182,5 +200,18 @@ class RobotClient { ResourceManager resource_manager_; }; +namespace proto_convert_details { + +template <> +struct from_proto_impl { + RobotClient::operation operator()(const robot::v1::Operation*) const; +}; + +template <> +struct from_proto_impl { + RobotClient::frame_system_config operator()(const robot::v1::FrameSystemConfig*) const; +}; + +} // namespace proto_convert_details } // namespace sdk } // namespace viam diff --git a/src/viam/sdk/tests/test_robot.cpp b/src/viam/sdk/tests/test_robot.cpp index 81970673c..18137d258 100644 --- a/src/viam/sdk/tests/test_robot.cpp +++ b/src/viam/sdk/tests/test_robot.cpp @@ -55,9 +55,6 @@ void robot_client_to_mocks_pipeline(F&& test_case) { // Run the passed-in test case on the created stack and give access to the // created RobotClient and MockRobotService. std::forward(test_case)(client, service); - - // Shutdown Server afterward. - server->shutdown(); } BOOST_AUTO_TEST_CASE(test_registering_resources) {