rmw
rmw copied to clipboard
ros2
Add matched & unmatched event
@Barry-Xu-2018 thanks for the effort, i will take a look.
CC: @ivanpauno @wjwwood @MiguelCompany @asorbini
In order to make it easier to understand the definition in PR, I modify rmw_fastrtps to explain how to use.
https://github.com/Barry-Xu-2018/rmw_fastrtps/commit/93e294aa2cb2b638f761724ce508f7c99c3b3ab9
This would be a very useful feature. If this gets merged this additionally needs to be exposed to rcl/rclcpp/rclpy API, right?
Thanks for your comments.
I will remove "total_count/total_count_change".
@Barry-Xu-2018 can you resolve the comments?
and i just add the list https://github.com/ros2/rmw/issues/330#issue-1371797574 which repositories to be updated.
One point needs to be discussed.
Now use the same status info for subscription matched/unmatched event and publication matched/unmatched event
User can get rmw_matched_status_t via rmw_take_event.
typedef struct RMW_PUBLIC_TYPE rmw_matched_status_s
{
/**
* For publisher, the number of subscribers currently matched to the concerned publisher.
*
* For subscriber, the number of publishers currently matched to the concerned subscriber.
*/
size_t current_count;
/**
* The change in current_count since the last time the status was read.
*/
size_t current_count_change;
} rmw_matched_status_t;
About current_count_change, there are 2 ways to set.
- current_count_change = matched_count - unmatched_count (from last read status)
- for matched event, current_count_chagne = matched_count from last read status (not care unmatched event) for unmatched event, current_count_chagne = unmatched_count from last read status (not care matched event)
Which way is better ? I should hear real requirement.
If 2 is selected, I think the name of rmw_matched_status_t maybe unsuitable.
@fujitatomoya @ivanpauno
@Barry-Xu-2018 I don't understand the question. aren't we just setting what DDS provides? e.g. for FastDDS
aren't we just setting what DDS provides? e.g. for FastDDS
Yes.
Here, current_count_change means matched_count - unmatched_count from last read status.
If matched/unmatched appears in pairs, the current_count_change is 0.
Now we have matched and unmatched event separately. For example, user calls rmw_take_event() for matched event and current_count_change is 0. But it doesn't mean matched event doesn't occur from last read status.
So I provide a second way. It can get matched or unmatched count from last read status (Of cause, I am not sure if this is useful for user.) from onPublicationMatched and on_subscription_matched
Ah okay, I see now. Maybe we could have:
current_count = <DDS current_count>
matched_events_change = total_count_change
unmatched_events_change = total_count_change - current_count_change
We could also keep the DDS style, and provide current_count/current_count_change/total_count_change.
Docs should be clear about their meaning though.
notifying underlying RMW Tier I implementation maintainers, @MiguelCompany @asorbini @eboasson
if you guys have any thoughts, please share here 👍
If matched/unmatched appears in pairs, the current_count_change is 0.
I thought this is one of the condition, and event itself means there were matched or unmatched event.
I mean current_count_change could be 0, if someone comes and someone else goes, but event comes up.
but I believe https://github.com/ros2/rmw/pull/331#issuecomment-1311778905 would be better and clear understanding for user application.
@ivanpauno @fujitatomoya
Thanks for your comments.
current_count = <DDS current_count> matched_events_change = total_count_change unmatched_events_change = total_count_change - current_count_changeWe could also keep the DDS style, and provide current_count/current_count_change/total_count_change. Docs should be clear about their meaning though.
Totally agree.
I will update code.
After updating below repository, I will add test at rmw_implementation to do full test. And then make PR for these repositories at the same time.
- rmw_fastrtps (Done. Not make a PR)
- rmw_cyclonedds (Ongoing)
- rmw_connextdds (ToDo)
During developing test case (Refer to test_event.cpp), I think current design (rmw_matched_status_t) makes user confused since matched & unmatched event all use the same structure.
Current design refers to DDS definition. But for user, he maybe doesn't know it.
So we can use more clear definition.
e.g.
For matched event, it returns
typedef struct RMW_PUBLIC_TYPE rmw_matched_status_s
{
/**
* For publisher, the number of subscribers currently matched to the concerned publisher.
*
* For subscriber, the number of publishers currently matched to the concerned subscriber.
*/
int32_t current_matched_count;
/**
* Matched count since the last time the status was read.
*/
int32_t current_matched_count_change;
} rmw_matched_status_t;
For unmatched event, it returns
typedef struct RMW_PUBLIC_TYPE rmw_unmatched_status_s
{
/**
* For publisher, the number of subscribers currently matched to the concerned publisher.
*
* For subscriber, the number of publishers currently matched to the concerned subscriber.
*/
int32_t current_matched_count;
/**
* Unmatched count since the last time the status was read.
*/
int32_t current_unmached_count_change;
} rmw_unmatched_status_t;
What do you think ? @ivanpauno @fujitatomoya
Now, below changes has been done
- rmw_fastrtps (https://github.com/Barry-Xu-2018/rmw_fastrtps/tree/topic-matched-unmatched-event-support)
- rmw_cyclonedds (https://github.com/Barry-Xu-2018/rmw_cyclonedds/tree/topic-add-match-unmatch-event-support)
- rmw_connextdds (https://github.com/Barry-Xu-2018/rmw_connextdds/tree/topic-matched-unmatched-event-support)
- rmw_implementation (https://github.com/Barry-Xu-2018/rmw_implementation/tree/topic-matched-unmatched-event-test)
Fastdds and Cyclonedds can pass all tests.
Connextdds can pass some tests since it doesn't spport rmw_event_set_callback.
Wait for the opinions for https://github.com/ros2/rmw/pull/331#issuecomment-1321135801.
If interface isn't changed, I will start to implement rcl, rclcpp and rclpy.
During developing test case (Refer to test_event.cpp), I think current design (rmw_matched_status_t) makes user confused since matched & unmatched event all use the same structure.
Current design refers to DDS definition. But for user, he maybe doesn't know it. So we can use more clear definition. e.g. For matched event, it returns [...] What do you think ? @ivanpauno @fujitatomoya
What I suppose I would like to be able to find out easily is: how many new matches showed up, and how many did I lose. DDS doesn't really help with that, and I think this definition makes more sense.
I had a quick look at your code for the Cyclone RMW layer. It looks fine but it did make me wonder whether it really is computing the desired data in all cases. I'd say one can model the total_count and current_count with a count of the cumulative number of matches and of the cumulative number of unmatches:
(edit)
In the original version of the comment I made a completely stupid mistake in what total_count is 🤦♀️ ... it increments every time there is a reader/writer matches, not when it disappears (e.g., https://github.com/eclipse-cyclonedds/cyclonedds/blob/d37952bd64007e5412f1a1eb734dbefc645086cf/src/core/ddsc/src/dds_reader.c#L341-L353). Rather than leaving the wrong info here and possibly causing confusion later on, I am editing it.
total_count = M
current_count = M-U
I think you want M and U (or ΔM and ΔU), so for U you would simply need to use total_count-current_count)
(end edit)
and then you can easily compute the changes locally. I'm just not quite sure that that is what the code does, but that may be because I haven't looked very carefully 😆
@eboasson
Thanks for sharing your thoughts.
According to my understanding, you mean rmw_take_event() takes the same event information regardless of the match or mismatch event.
typedef struct RMW_PUBLIC_TYPE rmw_matched_status_s
{
int32_t total_count;
int32_t current_count;
} rmw_matched_status_t;
User can get match/unmatch count according to these 2 values.
match/unmatched count means count from application launching.
This will lose 'change' information.
I mean that it loses match/unmatch change information from last call rmw_take_event() to get match/unmatched.
DDS provides this information.
Whether is this information necessary to be exposed to user ?
@eboasson Thanks for sharing your thoughts.
According to my understanding, you mean rmw_take_event() takes the same event information regardless of the match or mismatch event.
typedef struct RMW_PUBLIC_TYPE rmw_matched_status_s { int32_t total_count; int32_t current_count; } rmw_matched_status_t;User can get match/unmatch count according to these 2 values. match/unmatched count means count from application launching.
This will lose 'change' information. I mean that it loses match/unmatch change information from last call rmw_take_event() to get match/unmatched. DDS provides this information. Whether is this information necessary to be exposed to user ?
I am sorry I created confusion: I like what you proposed to provide to the user, providing number of new matches separately from number of new unmatches.
What I think is missing is being able to poll the event and detect cases where a match appeared and a match disappeared in between reading the event twice. The "current count" will then not change, and you would expect that accumulating all returned "current_count_change"s would equal the most recent "current count".
So just by monitoring the current count, there are some cases where something interesting happened that you can't detect from looking at the counters. If you also provide a cumulative version of the number of matches (so incremented each time a new match is created) and a cumulative version of the number of unmatches (incremented when a match is removed), then that becomes visible.
The matching part is easy, that you can get straight from DDS (my previous comment was in error, I edited it). For the unmatching part, you have to do something more, but it is available, and so you can provide the data to the user. All I was going on about is that I thought you made a mistake in that calculation.
So just by monitoring the current count, there are some cases where something interesting happened that you can't detect from looking at the counters. If you also provide a cumulative version of the number of matches (so incremented each time a new match is created) and a cumulative version of the number of unmatches (incremented when a match is removed), then that becomes visible.
Yes. User may want to get the count of match/unmatch change between read status twice.
The matching part is easy, that you can get straight from DDS (my previous comment was in error, I edited it). For the unmatching part, you have to do something more, but it is available, and so you can provide the data to the user.
Yes. About unmatch change, we can get by total_count_change - current_count_change. This formula may confuse users since the user doesn't understand detailed information about total_count_change and current_count_change. So I want to directly provide current_unmached_count_change to user.
All I was going on about is that I thought you made a mistake in that calculation.
Could you tell me where is a mistake?
Could you tell me where is a mistake?
Only if there actually is a mistake 😜 Of this I'm not sure. What it saw looks fine in principle, I was hunting for my total_count_change - current_count_change and that I couldn't find. But whether you need it depends on what guarantee you want to provide:
- If you're content with seeing the net change reflected in the event counts, i.e., we went from 3 to 5 matched readers, then I think it is fine
- If in the case you went from 3-to-8-to-5 readers, you want to inform the publisher that there are currently 5, but you gained 8 and lost 3, then I'm either overlooking something or you're not quite guaranteeing that.
I'm not sure how it interacts with listener callbacks, but I think that doesn't affect it. In any case, in DDS there is no guarantee that you will see each event separately (even if, currently, you will in Cyclone).
One other point of concern that I have, probably entirely unnecessary, is that the mechanism is not re-entrant. I don't know if that is a concern for the wider community, it is just that I make an effort to keep Cyclone thread-safe and so I am always checking that sort of thing 🤓
What it saw looks fine in principle, I was hunting for my total_count_change - current_count_change and that I couldn't find.
Current codes only return total_count_change and current_count_change. If user want to know unmatched count, he has to calculate it by himself.
But whether you need it depends on what guarantee you want to provide:
- If you're content with seeing the net change reflected in the event counts, i.e., we went from 3 to 5 matched readers, then I think it is fine
- If in the case you went from 3-to-8-to-5 readers, you want to inform the publisher that there are currently 5, but you gained 8 and lost 3, then I'm either overlooking something or you're not quite guaranteeing that.
For unmatched case, 3-to-8-to-5 leads to (assume status has been read while reader count is 3. total_count_change and current_count_change will be set to 0 once reading status.)
total_count_change = 5
count_change = 5
current_count_change = 2
So, matched count is total_count_change (5) and unmatched count is total_count_change - current_count_change (5-3 = 2)
These 3 items are from DDS.
In any case, in DDS there is no guarantee that you will see each event separately (even if, currently, you will in Cyclone).
Actually, we don't need to care about each event. Instead, we care about total_count_change, count_change and current_count_change from DDS. It can show what change (matched or unmatched) occurs from last read status.
One other point of concern that I have, probably entirely unnecessary, is that the mechanism is not re-entrant. I don't know if that is a concern for the wider community, it is just that I make an effort to keep Cyclone thread-safe and so I am always checking that sort of thing
Thanks for your kind reminder. I should check code to keep thread-safe.
https://github.com/ros2/rmw/pull/331#issuecomment-1321135801
About this comments, I'd like to hear your opinions.
@ivanpauno @fujitatomoya
@Barry-Xu-2018 sorry i am pretty occupied, but i will try to catch up in this week.
@Barry-Xu-2018 i might be mistaken...
total_count_change is required to user application, what is the use case? this is just an incremental number every time detects the corresponding endpoints, i am not really sure what this is for...
simplified interface suggested https://github.com/ros2/rmw/pull/331#issuecomment-1321135801 makes sense to me. but can we use the same structure for match and unmatch event?
@eboasson appreciate your comments, that helps!
total_count_change is required to user application, what is the use case? this is just an incremental number every time detects the corresponding endpoints, i am not really sure what this is for...
I think total_count_change is only used for calculating unmatched count. User should don't care this value.
simplified interface suggested https://github.com/ros2/rmw/pull/331#issuecomment-1321135801 makes sense to me. but can we use the same structure for match and unmatch event?
Yes. We can use the same structure as the below.
struct RMW_PUBLIC_TYPE rmw_matched_unmatched_status_s
{
/**
* For publisher, the number of subscribers currently matched to the concerned publisher.
*
* For subscriber, the number of publishers currently matched to the concerned subscriber.
*/
int32_t current_matched_count;
/**
* Matched or unmatched count since the last time the status was read.
*/
int32_t current_count_change;
};
typedef struct rmw_matched_unmatched_status_s rmw_matched_status_t;
typedef struct rmw_matched_unmatched_status_s rmw_unmatched_status_t;
Yes. We can use the same structure as the below.
IMO, this makes sense to me.
@fujitatomoya @ivanpauno
I update codes to simplify status structure.
Not use definition used in DDS to this status structure.
If you accept this change, I will update other repositories.
Discuss this interface at https://github.com/ros2/rmw_fastrtps/pull/645#discussion_r1048473839