Skip to content

RSDK-10579: Robot client update #425

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Apr 30, 2025
Merged

RSDK-10579: Robot client update #425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e71af6e
remove should_close_channel
lia-viam Apr 28, 2025
8653759
generalize client helper to work with robot client
lia-viam Apr 28, 2025
1d4b1b6
use client helper for robot client methods
lia-viam Apr 28, 2025
881ea2a
factor out enable_console_logging
lia-viam Apr 28, 2025
68d7a9f
guard calls to impl and reconnect logging on close
lia-viam Apr 28, 2025
f1cd2f3
use repeated ptr conversions
lia-viam Apr 28, 2025
52eab57
thread pool does not get shared_ptr
lia-viam Apr 28, 2025
0f8c575
remove commented line
lia-viam Apr 28, 2025
fc95c99
silence clang tidy
lia-viam Apr 29, 2025
ae3bac7
Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk
lia-viam Apr 29, 2025
e6e489a
Merge branch 'main' into robot-client-update
lia-viam Apr 29, 2025
c89c7f4
doc comments
lia-viam Apr 29, 2025
0e2bb78
document refresh
lia-viam Apr 30, 2025
cf3a49f
replace threads with single thread
lia-viam Apr 30, 2025
da78755
Merge branch 'main' into robot-client-update
lia-viam Apr 30, 2025
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
2 changes: 2 additions & 0 deletions src/viam/sdk/common/client_helper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ bool isStatusCancelled(int status) noexcept {
return status == ::grpc::StatusCode::CANCELLED;
}

void set_name(...) {} // NOLINT(cert-dcl50-cpp)
Copy link
Member

Choose a reason for hiding this comment

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

The cert warning is funny. If you make it a template with a parameter pack instead, I assume that doesn't work because it breaks the overload resolution / template instantiation balance that you are leveraging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

exactly! a variadic is always considered the worst/last choice for overload resolution so it's needed in this case


} // namespace client_helper_details

ClientContext::ClientContext() : wrapped_context_(std::make_unique<GrpcClientContext>()) {
Expand Down
15 changes: 14 additions & 1 deletion src/viam/sdk/common/client_helper.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <typename RequestType,
typename ClientType,
typename = decltype(&RequestType::mutable_name)>
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
Expand Down Expand Up @@ -96,7 +109,7 @@ class ClientHelper {

template <typename ResponseHandlerCallable, typename ErrorHandlerCallable>
auto invoke(ResponseHandlerCallable&& rhc, ErrorHandlerCallable&& ehc) {
*request_.mutable_name() = client_->name();
client_helper_details::set_name(&request_, client_);
ClientContext ctx;

if (debug_key_ != "") {
Expand Down
10 changes: 7 additions & 3 deletions src/viam/sdk/log/logging.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,15 @@ void LogManager::init_logging() {
console_sink_ = boost::make_shared<
boost::log::sinks::synchronous_sink<boost::log::sinks::text_ostream_backend>>(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() {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't really follow the logging changes too closely, so apologies if this is covering old ground, but what are the safety / concurrency guarantees around enabling and disabling console logging? If I have many RobotClient's coming into and out of existence in multiple threads, does it work right? It looks like the RobotClient::impl destructor puts back console logging. Does that mean there can only be one RobotClient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like the RobotClient::impl destructor puts back console logging.

That's right. It's a narrow window, and might all just happen with the } of main, but if someone destroys a RobotClient it's no longer sending logs to RDK, so we fall back to printing logs on the console. If we're running as a module, this will revert us back to the ugly \_ printing that was in effect before the logging PR

Does that mean there can only be one RobotClient?

Paraphrasing a discussion with @stuqdog , and I'm going to add a bunch of explanatory comments about this stuff. But basically the key is that non-default destruction in RobotClient::impl::~impl is wrapped in an if(log_sink) which...

  • only happens if RDK logging is enabled,
  • which only happens if RobotClient::connect_logging is called
  • which is a private method only accessible from ModuleService (the only place it gets called)
  • which is only getting called by us when RDK spins up an executable as a module

so under all these hypotheses, we can say that uniqueness and non-concurrency is guaranteed, and that the in-and-out-of-existence multiple threads thing can't happen in this case

console_sink_->set_filter(Filter{this});
VIAM_SDK_LOG(debug) << "Console logging enabled";
}

void LogManager::disable_console_logging() {
Expand Down
1 change: 1 addition & 0 deletions src/viam/sdk/log/logging.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ class LogManager {
LogManager& operator=(LogManager&&) = delete;

void init_logging();
void enable_console_logging();
void disable_console_logging();

LogSource sdk_logger_;
Expand Down
Loading