RSDK-10720 support TCP mode based on flag #435
Conversation
stuqdog
changed the title
src/viam/sdk/module/service.cpp
Outdated
| @@ -269,7 +269,11 @@ ModuleService::~ModuleService() { | |||
|
|
|||
| void ModuleService::serve() { | |||
| server_->register_service(impl_.get()); | |||
| const std::string address = "unix:" + module_->addr(); | |||
| std::string address; | |||
| if (module_->addr().find(':') == std::string::npos) { | |||
There was a problem hiding this comment.
I realize this is PoC and may not ever get merged, but this means of deciding that the intention is not to use AF_UNIX is potentially in conflict with fully solving RSDK-10642, because the presence of a : may be involved in helping a module determine whether it has been invoked in a way that is intended to be interpreted as a gRPC endpoint URI (e.g. dns:... or ipv4:... or unix:...), per https://grpc.github.io/grpc/cpp/md_doc_naming.html
There was a problem hiding this comment.
Yep! Totally agree, this was just to get something that works today available quickly. I will definitely clean this up if/when we want to merge it.
There was a problem hiding this comment.
@acmorrow if you're interested this is now ready for actual review, based on scope doc discussion.
…o RSDK-10720-support-tcp-module-connections
|
For my own later reference: to get the tflite_cpu module to build with this, I needed to give it a different version number, so that the compiler couldn't just use the cached version. So, the branch I actually used is in https://github.com/viamrobotics/viam-cpp-sdk/compare/main...penguinland:viam-cpp-sdk:ethan_tcp_branch?expand=1 This is not a "real" problem: when this is merged and deployed, it will get a different version anyway, and all will be well. |
…o RSDK-10720-support-tcp-module-connections
…o RSDK-10720-support-tcp-module-connections
stuqdog
requested review from
njooma,
lia-viam and
acmorrow
and removed request for
a team
|
Actual, non-POC C++ SDK changes. cc @penguinland @bhaney |
stuqdog
changed the title
src/viam/sdk/module/service.cpp
Outdated
| impl_ = std::make_unique<ServiceImpl>(*this); | ||
| } | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
minor style nit: generally, make one unnamed namespace up at the top of the file, after you've opened the file's main namespace:
#include <my_header.hpp>
#include <stuff>
namespace my {
namespace thing {
namespace {
// put get_protocol here
} // namespace
...
} // namespace thing
} // namespace my
It's a convention, there is no real harm to opening and closing the unnamed namespace as often as you need, but keeping all the little utilities up together in the unnamed namespace up at the top prevents a proliferation of tiny namespace blocks.
src/viam/sdk/module/service.cpp
Outdated
| std::string get_protocol(int argc, char** argv) { | ||
| for (int i = 0; i < argc; ++i) { | ||
| if (strcmp(argv[i], "--tcp-mode") == 0) { | ||
| return "dns:"; |
There was a problem hiding this comment.
Curious why this is dns:. Are we expecting hostnames here? Are we allowing cross host between modules and RDK?
There was a problem hiding this comment.
I don't think we're expecting hostnames here, but in testing I got dns to work and I recall having issues with ipv4 . Also, this line (The following schemes are supported by the gRPC C-core implementation, but may not be supported in other languages:) about ipv4 had me worried.
There was a problem hiding this comment.
That's fair, I'd forgotten about that caveat in the docs.
| namespace { | ||
| std::string get_protocol(int argc, char** argv) { | ||
| for (int i = 0; i < argc; ++i) { | ||
| if (strcmp(argv[i], "--tcp-mode") == 0) { |
There was a problem hiding this comment.
In future work, I'd like to see this argc/argv stuff entirely removed from ModuleService, and I'd like to see our command line parsing move to boost::program options rather than writing our own.
I've also been wondering whether we should have a libviamsdk_module_main library which defines a main which does the proper arg parsing, and delegates to a viam_module_main or viam_module_describe which exists only to populate a module registration vector.
There was a problem hiding this comment.
yeah strongly agree about refactoring the arg parsing logic. I didn't want to overcomplicate here this PR but it's definitely something I'd like to see.
Haven't thought much about the libviamsdk_module_main idea but I like the sound of it.
There was a problem hiding this comment.
Broad strokes here but roughly I think most modules shouldn't need to do anything more than:
- Define some number of classes implementing
Resource - Have a well-known entry point that returns a
std::vector<ModelRegistration>for eachModelthey offer, each of which constructs an instance of the appropriate class.
I got here after doing some work on the UR arm driver, where I was able to make main sort of trivial:
And then give the relevant class a static method that generates the relevant registrations:
Some thought would need to go into how to arrange symbols between libraries and how the well known points wired up, but I don't think it would be too hard, and it would eliminate a lot of boilerplate from modules. It would also make it easier to adjust the command line parsing in all modules that made use of the facility as part of an SDK upgrade.
If you and @lia-viam like this idea let me know and I'll write up a quick ticket.
There was a problem hiding this comment.
I mean, I definitely like the look of this. Unfortunately Lia is out until Friday, and I'm out starting on Friday, so we won't be able to speak about this until July 7th. But I'm more than happy to see a ticket made for this.
There was a problem hiding this comment.
No description provided.