make rmw_implementation a meta rmw implementation #17
Conversation
dirk-thomas
added
in progress
There was a problem hiding this comment.
Changes lgtm, I didn't try it out locally though.
I think we should consider moving some of these utilities functions which are used here and in the components stuff to a common place, maybe in rmw for now, so that they're not duplicated everywhere, e.g. split and perhaps others.
| @@ -16,38 +16,63 @@ | |||
|
|
|||
| find_package(rmw_implementation_cmake REQUIRED) | |||
| set(default_rmw_implementation "@RMW_IMPLEMENTATION@") | |||
| set(available_rmw_implementation "@RMW_IMPLEMENTATIONS@") | |||
| message(FATAL_ERROR | ||
| "The RMW implementation has been specified as " | ||
| "'${requested_rmw_implementation}' through CMake, " | ||
| "however the RMW implementation should be " |
There was a problem hiding this comment.
"however the RMW implementation was already set as "? I you want to further imply that what was specified via cmake should match this could say:
"however this needs to match the RMW implementation which was already set as "
"'${default_rmw_implementation}', "
"when the 'rmw_implementation' package was built.")
There was a problem hiding this comment.
I replaced line 36 and 60 with however this needs to match the RMW implementation to make it more clear: see 4ba229d
| free(value); | ||
| #endif | ||
| } | ||
| // printf("get_env_var(%s) = %s\n", env_var, value_str.c_str()); |
There was a problem hiding this comment.
I would keep them since they they make it easier to debug the code when necessary. These functions might be removed in the future anyway when being available from a single location for reuse.
|
|
||
| #include "rmw/rmw.h" | ||
|
|
||
| std::string get_env_var(const char * env_var) |
There was a problem hiding this comment.
I'd recommend we try to do ros2/rmw#87 (and related pr's) to avoid duplicating this function around.
There was a problem hiding this comment.
We can for sure use functions from a single location once they are available. I would rather not wait for that to happen for this PR though. Also I don't think rmw is a good location for those since it has nothing to do with the ROS middleware interface.
There was a problem hiding this comment.
That's fine, but we should not continue to do this otherwise replacing all instances is just going to get harder and harder.
Also I don't think rmw is a good location for those since it has nothing to do with the ROS middleware interface.
Well, it does have something to do with it, so far as in the functions are used by many of the rmw implementations (like this meta one). By the same logic, the utilities in rclcpp (like scoped exit) should be in a separate library because it doesn't have anything to do with rclcpp. While that's technically true, it's not very practical.
| #define CALL_SYMBOL(symbol_name, ReturnType, error_value, ArgTypes, arg_values) \ | ||
| void * symbol = get_symbol(symbol_name); \ | ||
| if (!symbol) { \ | ||
| return error_value; \ |
There was a problem hiding this comment.
Should this set an appropriate error message?
There was a problem hiding this comment.
Good point. I added rmw error messages for two cases and also a try / catch block around the library loading: see 12e12f1
dirk-thomas
force-pushed
the
meta_rmw_impl
branch
from
12e12f1
to
52a46ba
Compare
This patch makes
rmw_implementationan actual rmw implementation. If only a single rmw impl. is available or Poco is unavailable it simply redirects to the default rmw implementation. Otherwise it acts a dynamic dispatch to any rmw implementation selected by the environment variableRMW_IMPLEMENTATION(similar asrosidl_typesupport_c|cpp).The next step after this has been merged will be to remove all rmw specific targets and only build targets against this "meta" rmw implementation (see ros2/ros2#302).
Ready for review.