Adding required structs and methods to get a list of publishers or subscribers with their respective qos#186
Conversation
b05810b to
1610f36
Compare
1610f36 to
f138591
Compare
49af797 to
36e4a68
Compare
6143c32 to
25d4236
Compare
25d4236 to
f3b5b72
Compare
ivanpauno
left a comment
There was a problem hiding this comment.
I don't get why rmw_get_qos_for_publishers and rmw_get_qos_for_subscribers return a list of rmw_qos_profile_t (with the participant name), instead of just returning one.
IMO, both of them should return only one rmw_qos_profile_t, using the data discovered by the rmw_node_t passed as an argument.
Sorry, now I see why you need a list (there may be many publishers/subscription on each topic, in different nodes). |
For eg: the command What do you suggest is a better term to |
It should say node name, |
Ah, I see what you mean. Alright, so |
We currently have two types of graph introspection functions:
I think that getting the list of Does that sound reasonable? |
I am a little confused. The only data available is the handle to |
|
@jaisontj I'm thinking in something like this: rmw_get_publisher_qos_by_node(
const rmw_node_t * node,
rcutils_allocator_t * allocator,
const char * node_name,
const char * node_namespace,
rmw_qos_profiles_t * qos_profiles);I think that matches better the current API. And actually, I would go with a method like this: rmw_get_publisher_names_types_and_qos_by_node(
const rmw_node_t * node,
rcutils_allocator_t * allocator,
const char * node_name,
const char * node_namespace,
bool demangle,
rmw_names_and_types_t * topic_names_and_types,
rmw_qos_profiles_t * qos_profiles);
If you want to go with something similar to your first proposal, I would rather go with: rmw_ret_t
rmw_get_qos_for_publishers(
const rmw_node_t * node,
const char * topic_name,
const char * type_name,
bool demangle,
rmw_node_names_t * node_names,
rmw_qos_profiles_t * qos_profiles);I added |
|
Thank you for this @ivanpauno. After looking at your suggestions, I have slight modifications in mind rmw_get_publisher_names_and_qos_for_topic(
const rmw_node_t * node,
rcutils_allocator_t * allocator,
const char * topic_name,
rcutils_string_array_t * node_names,
rmw_qos_profiles_t * qos_profiles);I've renamed the function from Parameters:
I've taken off:
Let me know if I've gotten this right. If so, I will update this PR with the same :) |
|
@ivanpauno I think some of the confusion here is that we are querying for information for everything except this node. There isn't a precedent in the graph API of "information_by_topic", but we need that info for this new functionality. @jaisontj For naming consistency, it may make more sense to name it Note: the name convention in ROS2 is So to summarize that, I think you're on the right track and we're looking at @ivanpauno does that sound about right? EDIT: more clarification - the purpose of this code change is to gain information about the rest of the running system, with the goal of later exposing QoS mismatch when we try to subscribe to a topic whose publisher has an incompatible QoS to our subscription. |
wjwwood
left a comment
There was a problem hiding this comment.
Mostly looks good to me, there was only one thing that I think must be fixed (related to not checking if allocate fails in one case), but there were lots of other comments that may or may not need to be addressed.
| * The topic_name parameter must not be `NULL` and must follow the topic naming rules | ||
| * mentioned at http://design.ros2.org/articles/topic_and_service_names.html | ||
| * Non existent topic names are allowed. | ||
| * In that case, this function will return an empty array. |
There was a problem hiding this comment.
Sounds like they are allowed? Why not an error case?
There was a problem hiding this comment.
I am not sure what error code to use in this case (a generic RMW_RET_ERROR? RCL_RET_TOPIC_NAME_INVALID migrated over to rmw?).
Also, this function shouldn't be used for detecting the existence of a topic; that feels like it is encroaching on the purpose of rmw_get_topic_names_and_types().
There was a problem hiding this comment.
Sorry this comment doesn't make since if "non-existent topic" just means a topic that isn't on the graph. I was still thinking an empty string, in which case it should create an error, just like if it violated any of the other rules in the topic name rules set.
I think this can be resolved without changes. Though a follow up question is what happens if the value for topic_name violates one of the topic name rules, e.g. empty string, starts with a number, etc.
There is already this function:
rmw/rmw/include/rmw/validate_full_topic_name.h
Lines 37 to 90 in 813b94d
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
|
I see |
|
@mm318 Can you check the macOS compiler warnings? https://ci.ros2.org/job/ci_osx/7362/warnings11Result/ |
|
Hmm, interesting this post here and these examples in Anyway, I'll implement the solution found here: https://stackoverflow.com/a/9293087 |
Signed-off-by: Miaofei <miaofei@amazon.com>
That's strange, ignoring the warning sounds reasonable. |
|
Thank you for merging, @ivanpauno! Let's continue the conversation at ros2/rclcpp#960. |
|
Thank you @mm318 for following up on these :) |
#186 introduces two functions to the RMW interface which are not present when including `rmw/rmw.h`. For convenience to the rmw implementer, I think it makes sense to include all functions when including `rmw/rmw.h`.
Includes creating relevant data structures to enable the feature mentioned here. Relevant information about implementation is discussed here
Summary:
rmw_participant_qos_profile_tto store participant name and their qos policy in one structrmw_participants_tto store a list ofrmw_participant_qos_profile_trmw_participant_qos_profile_allocatermw_participant_qos_profile_freeNote:
colcon build --packages-up-to rmw && colcon test --packages-up-to rmw && colcon test-result --verbose --test-result-base build/rclpasses without any errors or failures.