Skip to content
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

Merged
merged 2 commits into from Dec 16, 2016

Conversation

Projects
None yet
5 participants
@codebot
Copy link
Member

commented Dec 1, 2016

No description provided.

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");

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

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?

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Dec 9, 2016

Member

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.

This comment has been minimized.

Copy link
@codebot

codebot Dec 9, 2016

Author Member

lol come on man, it works!

This comment has been minimized.

Copy link
@codebot

codebot Dec 9, 2016

Author Member

ok Deanna has enlightened me as to why this is not the best idea I've ever had. We'll fix this up.

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Dec 9, 2016

Member

@codebot Btw. you should really compile the code with --isolated locally which you seem to be not doing.

This comment has been minimized.

Copy link
@codebot

codebot Dec 9, 2016

Author Member

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 😢

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

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.

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Dec 9, 2016

Member

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));

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

This line is under 100 characters without wrapping it.

This comment has been minimized.

Copy link
@codebot

codebot Dec 9, 2016

Author Member

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());
});

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

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());
    });
}

This comment has been minimized.

Copy link
@codebot

codebot Dec 9, 2016

Author Member

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()

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

I hate uncrustify sometimes because of how it reacts to lambda's.

#define VISIBILITY_LOCAL
#endif

#define VISIBILITY_PUBLIC_TYPE

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

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.

This comment has been minimized.

Copy link
@codebot

codebot Dec 9, 2016

Author Member

yeah. the bummer is that the macros get long, but I understand the point.

This comment has been minimized.

Copy link
@codebot

codebot Dec 9, 2016

Author Member

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.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

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.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Dec 9, 2016

Member

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".

This comment has been minimized.

Copy link
@codebot

codebot Dec 9, 2016

Author Member

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 (?).

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Dec 10, 2016

Member

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.

This comment has been minimized.

Copy link
@codebot

codebot Dec 10, 2016

Author Member

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");

This comment has been minimized.

Copy link
@dhood

dhood Dec 9, 2016

Member

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

@codebot

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2016

ok, version 0.0.2 of this package has now been submitted. Thanks everybody for the patience 😃

@wjwwood

This comment has been minimized.

Copy link
Member

commented Dec 16, 2016

Since we're going to hold off on #149, I'd suggest merging this.

@codebot

This comment has been minimized.

Copy link
Member Author

commented Dec 16, 2016

🎉

@codebot codebot merged commit 0e438da into master Dec 16, 2016

@codebot codebot removed the in review label Dec 16, 2016

@mikaelarguedas mikaelarguedas deleted the minimal_composition_example branch Sep 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.