WIP: RSDK-10435: Dial direct #430

lia-viam · 2025-05-02T18:16:12Z

Enables dialing directly to gRPC, potentially circumventing rust-utils entirely.

This includes some QoL/drive-by changes, such as structifying DialOptions and removing the shared_ptr from RobotClient

lia-viam · 2025-05-02T18:16:59Z

src/viam/sdk/module/service.cpp

    server_->shutdown();

    if (parent_) {
        try {
-            parent_->close();


this was causing a double call to stop_all, the second one happening after disconnecting from the robot

Does conan now give us a vehicle for building the entire dependency tree under ASAN and UBSAN?

I think yes in theory but it's probably pretty tricky, see here for discussion (old conan docs) https://docs.conan.io/1/howtos/sanitizers.html

stuqdog

Nice! A couple small things but generally looks good to me.

stuqdog · 2025-05-05T12:08:03Z

src/viam/sdk/rpc/dial.cpp

@@ -1,3 +1,4 @@
+#include <grpc/grpc.h>


(nit) should move this to a separate include block

stuqdog · 2025-05-05T12:09:44Z

src/viam/sdk/rpc/dial.cpp

+#include <viam/api/proto/rpc/v1/auth.grpc.pb.h>
+#include <viam/api/proto/rpc/v1/auth.pb.h>
 #include <viam/api/robot/v1/robot.grpc.pb.h>
 #include <viam/api/robot/v1/robot.pb.h>
+#include <viam/sdk/common/client_helper.hpp>
 #include <viam/sdk/common/exception.hpp>
+#include <viam/sdk/common/private/instance.hpp>
 #include <viam/sdk/rpc/private/viam_grpc_channel.hpp>
 #include <viam/sdk/rpc/private/viam_rust_utils.h>


(nit) Hm we did this wrong here (my bad!) but in most places in the SDK we've got the api includes in a separate block before the sdk includes. Could we do the same here?

stuqdog · 2025-05-05T12:17:12Z

src/viam/sdk/rpc/private/viam_grpc_channel.cpp

+
+    return grpc::CreateChannel(address, tls_creds);
+}
+
 std::shared_ptr<grpc::Channel> create_viam_channel(


probably not worth fixing here, but it's unfortunate and a little confusing that create_viam_channel creates a grpc::Channel, not a ViamChannel. :(

fixed it :)

stuqdog · 2025-05-05T12:19:47Z

src/viam/sdk/rpc/dial.cpp

+    using namespace proto::rpc::v1;
+
+    auto auth_stub = AuthService::NewStub(channel_for_auth);
+    ClientContext ctx;
+    AuthenticateRequest req;
+
+    *req.mutable_entity() = opts.auth_entity.get();
+    *req.mutable_credentials()->mutable_payload() = opts.credentials->payload();
+    *req.mutable_credentials()->mutable_type() = opts.credentials->type();
+
+    AuthenticateResponse resp;
+
+    auto status = auth_stub->Authenticate(ctx, req, &resp);


Can we factor this out into a helper function? I think it's a bit cleaner, and also will be relevant if/when we want to have app client support in C++.

stuqdog · 2025-05-05T12:25:43Z

src/viam/sdk/rpc/dial.hpp

+    /// @brief Bypass WebRTC and connect directly to the robot.
+    /// This dials directly through grpc bypassing rust utils.
+    bool disable_webrtc = false;


(opt) Maybe not a big deal (I don't think we do this in other SDKs), but it might be good to add a note here about the specialized uri form that is required (<part>.<location>.local.viam.cloud:8080). I could definitely imagine that being a gotcha.

Oh yeah I wanted to ask you about that, should we try to validate the URI under this code path and error out of it's not of that form?

Hmm good question. I think probably no. My thinking is:

that format might change in the future and that could lead to some issues that might be annoying to track down, and

that format is currently required assuming the user isn't doing anything weird or bespoke. If, e.g., they were to use a different app instance than app.viam.com then maybe the format would be different? I have no idea but I don't want to just break things for someone if such a case were to arise

On the other hand, it is a little annoying if we have users saying "why doesn't this work??" and we say "oh it says in the comments" and they say "that wasn't helpful, if you'd just given me an error message I would've known much faster". So while I'm inclined not to error out, I'm open to being convinced.

Maybe we add a warning or info log message? Probably excessive 😅

…-direct

acmorrow

I took a look out of interest. Please don't let these comments get in the way of moving this forward, they are just observations and notes on things that have bugged me about the code that is under modification here, so this seemed like a time to raise them. I'm happy for any or none of these issues to be deferred into future work.

acmorrow · 2025-05-09T15:55:34Z

src/viam/sdk/rpc/dial.hpp

-    // TODO (RSDK-917): We currently don't provide a flag for disabling webRTC, instead relying on a
-    // `local` uri. We should update dial logic to consider such a flag.
-
+struct DialOptions {


Just curious, what was the motivation for making this a struct over a class? Keeping it as a class would make it easier in the future to hide the implementation details behind a pimpl in order to make it not part of the ABI.

I also liked the fluent style setters, so you could write:

ViamChannel::dial(where, DialOptions().set_entity(...).set_credentials()...));

Admittedly, the structure of some of the existing setters is a little off. For instance, it isn't clear to me why set_credentials takes an optional. When would you call set_credentials with a disengaged one?

Ah so personally I was applying the C++ core guideline against trivial getters and setters in a class which hint that something probably should have just been a struct. With newer C++ (or older C++ with gnu extensions) I'm much more in favor of using designated initializers, eg

ViamChannel::dial(where, { .entity = ..., .credentials = ..., });

this PR has exposed a bunch of weird stuff around these classes and optional, I was going to try to clean up the example code because there's a lot of unnecessary instances of constructing an options and then a optional<options> which is set to the previous options

Designated initializers are definitely great (though C++20 IIRC), but it does create a very tightly bound interface: not only can you not change the sizes / orders of the members of the struct, but you can't even interpose on the act of setting the values to add behavior or checks. I have some doubts that they are the right thing to use for a public interface in an SDK.

On the other hand, we aren't there yet with worrying about ABI, so I'm not going to push too hard either way on it.

acmorrow · 2025-05-09T15:57:42Z

src/viam/sdk/rpc/dial.hpp


    /// @brief Timeout of connection attempts when initially dialing a robot
    /// Defaults to 20sec to match the default timeout duration
-    std::chrono::duration<float> initial_connection_attempt_timeout_{20};
+    std::chrono::duration<float> initial_connection_attempt_timeout{20};


⬇️ - I've always wondered: what is the point of having Options separate from DialOptions? And if they are specifically part of RobotClient, maybe they belong as part of that class (i.e. RobotClient::Options) or at least in that header? Having a SDK namespace level type named Options when it is only associated with RobotClienthas always bothered me.

I'm also curious if having the default should be to have a refresh interval (what would it be?), and provide an API to opt out:

class [RobotClient::]Options { Options(); Options(DialOptions dial_options); Options& disable_refresh(); Options& set_refresh_interval(std::chrono...); }

I suspect a lot of this may be cruft that was not thought over a ton and hasn't been updated. I agree no refresh seems like an odd default

doing an overhaul of this API (commit forthcoming) and made the troubling discovery that a lot of our example code says that passing a refresh interval of zero seconds does auto refresh. I'm not sure what auto refresh means or if this was ever the case!

update: documentation has been updated to reflect that zero seconds means 'only on config update'

acmorrow · 2025-05-09T16:02:54Z

src/viam/sdk/rpc/dial.hpp

@@ -11,7 +11,8 @@
 namespace viam {
 namespace sdk {

-class DialOptions;
+struct DialOptions;


Are these really ViamChannelOptions? Or ViamChannel::Options? In which case, this header could be viam_channel.h?

I think they probably are indeed ViamChannel::Options

acmorrow · 2025-05-09T16:16:12Z

src/viam/sdk/rpc/dial.cpp

+    }
+
+    const std::chrono::duration<float> float_timeout = opts.timeout;
+    void* ptr = init_rust_runtime();


If this was captured by unique_ptr with free_rust_runtime as the deleter type, some things could be simplified.

that's a good idea i'll try to do it that way

If you decide to do it, you could probably just create a nice shim class in sdk/rpc/private/viam_rust_utils.h that called init_rust_runtime in the ctor, and presented dial as a member.

acmorrow · 2025-05-09T16:18:35Z

src/viam/sdk/common/client_helper.cpp

+
+    const std::string& token =
+        Instance::current(Instance::Creation::open_existing).impl_->direct_dial_token;
+    if (!token.empty()) {


I'm a little surprised to see this as a global, but maybe I'm misunderstanding. But if you had one channel that was doing direct dial and another that wasn't, wouldn't this by trying to add this metadata to the contexts for both channels?

I think yes, although I'm not sure how common/feasible such a situation would be in practice, or if we should error out if we detect that kind of thing? cc @stuqdog

Hmm this is a good question. I'm not sure how common such a situation is in practice currently, but I do think it's certainly feasible and I could imagine a case where, e.g., someone is connecting to two machines at once and would prefer to use direct gRPC but is required to use webRTC for some of its added functionality on one of the machines only.

For that matter, even if both channels were dialing direct, if they were dialing to different machines then the token would only be valid for one of them! So yeah, probably tokens should either not be global or (if that's not feasible) should be keyed to a particular channel.

hmm ok so this is probably something we should handle better. i'll try to look into the architecture and see how to make this work probably on a per channel basis

@acmorrow after much wailing and gnashing of teeth we figured out a way to do this without a global, thereby ballooning the surface area of this PR considerably.

acmorrow · 2025-05-09T16:22:01Z

src/viam/sdk/module/service.cpp

    server_->shutdown();

    if (parent_) {
        try {
-            parent_->close();


Does conan now give us a vehicle for building the entire dependency tree under ASAN and UBSAN?

…-direct

lia-viam and others added 21 commits April 11, 2025 09:46

make dial_options a struct

a56574f

wip: direct dial

d86eac7

get auth token successfully

4d5f8aa

make calls successfully

6eb0fdd

we can still verify server certs

d247cc8

cleanup

4b7e4c2

cleanup2

c41325b

docs

c549c07

Merge branch 'main' into dial-direct

a21c424

use instance for global token

41e6960

remove dtor logging

d0c3f21

Merge branch 'fix/dtor-log' into dial-direct

1f41cad

newline

a53494a

join if joinable

bbc0adf

Merge branch 'fix/dtor-log' into dial-direct

a3add02

Merge branch 'main' into dial-direct

1ae3732

spacing and variable simplification

c3ff877

reset parent to prevent double close

7d8175b

do not disable webrtc by default

26c2225

revert dtor logging

0cd8723

unshare robot client

d3cdb58

lia-viam requested a review from a team as a code owner May 2, 2025 18:16

lia-viam requested review from njooma and stuqdog and removed request for a team May 2, 2025 18:16

lia-viam commented May 2, 2025

View reviewed changes

stuqdog reviewed May 5, 2025

View reviewed changes

lia-viam added 3 commits May 5, 2025 10:59

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into dial…

c0e2197

…-direct

include fixes and helper rename

beab7d2

document direct dial uri

89104c9

lia-viam added 3 commits May 5, 2025 12:30

factor auth request into helper function

8d68fd2

fix doxygen comment

29cfd35

ifdef namespace alias

fde961c

lia-viam changed the title ~~RSDK-10435: Dial direct~~ WIP: RSDK-10435: Dial direct May 8, 2025

acmorrow reviewed May 9, 2025

View reviewed changes

lia-viam added 5 commits May 12, 2025 15:59

use ifdef for direct dial

44d8601

replace with custom unique_ptr deleters

c7d8b4c

fix if stmt

afa6c70

overhaul options and refresh interval

8ba5efa

add include

95dc5f8

lia-viam force-pushed the dial-direct branch from 0ced228 to 95dc5f8 Compare May 14, 2025 15:02

lia-viam added 12 commits May 14, 2025 11:05

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into dial…

f86011f

…-direct

silence unused var warning on ifdef

4a82a6d

client context gets token from channel ctor

8e99746

sdk overhaul for channel/token

a8b858d

test code update

2e2b2ec

update complex module rpc clients

67aec53

const viam channel

5a3d47c

linter fixes

350f0ff

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into dial…

fa1f598

…-direct

typo fix

cb7ec67

remove no longer needed friend decl

ed6bfed

linter

2c7a942

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: RSDK-10435: Dial direct #430

WIP: RSDK-10435: Dial direct #430

lia-viam commented May 2, 2025

lia-viam May 2, 2025

acmorrow May 9, 2025

lia-viam May 9, 2025

stuqdog left a comment

stuqdog May 5, 2025

stuqdog May 5, 2025

stuqdog May 5, 2025

lia-viam May 5, 2025

stuqdog May 5, 2025

stuqdog May 5, 2025

lia-viam May 5, 2025

stuqdog May 5, 2025 •

edited

Loading

acmorrow left a comment

acmorrow May 9, 2025

lia-viam May 12, 2025

acmorrow May 13, 2025

acmorrow May 9, 2025

lia-viam May 12, 2025

lia-viam May 13, 2025

lia-viam May 13, 2025

acmorrow May 9, 2025

lia-viam May 12, 2025

acmorrow May 9, 2025

lia-viam May 12, 2025

acmorrow May 13, 2025

acmorrow May 9, 2025

lia-viam May 12, 2025

stuqdog May 12, 2025

lia-viam May 12, 2025

lia-viam May 15, 2025

acmorrow May 9, 2025

WIP: RSDK-10435: Dial direct #430

Are you sure you want to change the base?

WIP: RSDK-10435: Dial direct #430

Conversation

lia-viam commented May 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog May 5, 2025 • edited Loading

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog May 5, 2025 •

edited

Loading