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.
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
minimal example of how to compose nodes at compile or run time #145
Conversation
rclcpp::executors::SingleThreadedExecutor exec; | ||
std::vector<std::shared_ptr<rclcpp::Node>> nodes; // save shared pointers ! | ||
std::vector<std::string> lib_names; | ||
char * ament_prefix_path = getenv("AMENT_PREFIX_PATH"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't portable to Windows I think. Also, in general this code to iterate over the AMENT_PREFIX_PATH
and stuff should be in a function somewhere and not something we expect the user to copy-paste and modify.
@dirk-thomas is this handle in part or completely by the ament_index_cpp
package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what this code is supposed to do. It seems to take AMENT_PREFIX_PATH
(which contains a list of paths) and concat it with "/lib/...". That is not only a platform specific issue but doesn't even work conceptionally on Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol come on man, it works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok Deanna has enlightened me as to why this is not the best idea I've ever had. We'll fix this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codebot Btw. you should really compile the code with --isolated
locally which you seem to be not doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the very specific case on Linux where everything is built from source in the same incantation of "ament build", the result is a setup.bash file which sets AMENT_PREFIX_PATH
to a single path, and you can just jam on /lib/
at the end and start grabbing libraries. It works! It's super simple! But alas, I agree, it's fragile, not cross-platform, and it's not the best idea to recommend as a usage pattern 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea is to iterate (even if it's not doing it here) over the AMENT_PREFIX_PATH
and search for the library in question. I think ament_index_cpp
should at least be able to get the AMENT_PREFIX_PATH
or perhaps metadata about the libraries such that iterating over the APP is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The complete logic is currently only implement in the existing demo and not exposed through a header: https://github.com/ros2/demos/blob/master/composition/src/api_composition.cpp
{ | ||
publisher_ = create_publisher<std_msgs::msg::String>("topic"); | ||
timer_ = create_wall_timer( | ||
500_ms, std::bind(&PublisherNode::on_timer, this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is under 100 characters without wrapping it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my non-sentient editor operates in an 80-character terminal 😃
"topic", | ||
[](std_msgs::msg::String::UniquePtr msg) { | ||
printf("Subscriber: [%s]\n", msg->data.c_str()); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd run this through uncrustify
, because I would have formatted it as:
SubscriberNode::SubscriberNode()
: Node("subscriber_node")
{
subscription_ = create_subscription<std_msgs::msg::String>(
"topic",
[](std_msgs::msg::String::UniquePtr msg) {
printf("Subscriber: [%s]\n", msg->data.c_str());
});
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised as well; when I first wrote it, I used the same indentation as you suggested. But ament_uncrustify
seems to prefer the syntax included in this branch at the moment; it flags the indentation as being incorrect when I add that extra indent for the printf()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate uncrustify sometimes because of how it reacts to lambda's.
#define VISIBILITY_LOCAL | ||
#endif | ||
|
||
#define VISIBILITY_PUBLIC_TYPE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These macros should really use the package's name in the macro name, i.e. MINIMAL_COMPOSITION_PUBLIC
instead of VISIBILITY_PUBLIC
. They need to be unique to the package (actually even to the shared library) in which they are used to avoid collisions when using headers from other packages and libraries. This is the same reason that we cannot provide a generic, reusable "visibility control" header for people to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. the bummer is that the macros get long, but I understand the point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we provide a package called "visibility" or something concise that provides this header, and namespaces it appropriately? Or if we put a header like this into rclcpp so that the macros are concise like RCLCPP_PUBLIC
or something? Then packages wouldn't have to basically copy this file and change the prefixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I tried to explain these macros need to be unique to the library they are used in and therefore you cannot have a single generic header for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could have a header that only provides the definition for "import" (a.k.a. on Windows dllimport) and "export" (a.k.a. on Windows dllexport), but not the switching define "public".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why they need to be unique to the library. To the naive user (me), it looks just like a few ifdefs and defines. Any compilation unit can include them and they will expand accordingly, or so it would seem (?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cucial function of these macros is determined by the definition of VISIBILITY_DLL
. It is only set when you build your package. And not set when other packages use your headers. That is the basic reason why the macros need to be package specific.
You can search on stackoverflow for very lengthy descriptions of this with lots of details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback. I have learned much about this. To summarize for anybody driving by in the future: the reason why every library has to have its own prefix for the visibility flags is for the (potentially common) case where you want to write a shared library that imports symbols from another shared library. If they are using the same visibility symbols, it will be impossible for the compiler/linker to export a symbol from one library while importing that same symbol to the other library. This typically only happens on Windows, since unix-like OS'es commonly export all symbols as public.
Given all this complexity, I'm going to remove compose_at_run_time.cpp
from this example package, since it's not simple and will most likely be implemented by a truly generic solution in the future.
Thanks everyone! Cheers 🎉
rclcpp::executors::SingleThreadedExecutor exec; | ||
std::vector<std::shared_ptr<rclcpp::Node>> nodes; // save shared pointers ! | ||
std::vector<std::string> lib_names; | ||
char * ament_prefix_path = getenv("AMENT_PREFIX_PATH"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the ament_prefix_path might contain multiple paths even if you have built everything at once, but you sourced a local_setup.bash
(or use an overlay workspace, etc).
You can use a method from ament_index_cpp
to work with resource files without knowing the install paths. (there's more info about the ament resource index
here but it's a bit hard to find). Maybe check ros2/demos#84 for examples on registering/accessing the appropriate markers for this example
ok, version 0.0.2 of this package has now been submitted. Thanks everybody for the patience 😃 |
Since we're going to hold off on #149, I'd suggest merging this. |
🎉 |
No description provided.