-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add matched event support #331
Add matched event support #331
Conversation
@Barry-Xu-2018 thanks for the effort, i will take a look. |
In order to make it easier to understand the definition in PR, I modify rmw_fastrtps to explain how 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.
overall, this is what i was expecting to extend based on rmw_event
interface. this is really straight forward to extend for each Tier-I RMW implementation.
This would be a very useful feature. If this gets merged this additionally needs to be exposed to rcl/rclcpp/rclpy API, right? |
@AndreasR30 yes, that is the idea. |
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.
LGTM!
But based on the comments we probably don't need to expose total_count
/total_count_change
Thanks for your comments. |
154078d
to
a13bcfe
Compare
@Barry-Xu-2018 can you resolve the comments? and i just add the list #330 (comment) 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 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.
Which way is better ? I should hear real requirement. |
@Barry-Xu-2018 I don't understand the question. |
Yes. 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.
We could also keep the DDS style, and provide |
notifying underlying RMW Tier I implementation maintainers, @MiguelCompany @asorbini @eboasson if you guys have any thoughts, please share here 👍 |
I thought this is one of the condition, and event itself means there were matched or unmatched event. but I believe #331 (comment) would be better and clear understanding for user application. |
Thanks for your comments.
Totally agree. |
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.
|
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. 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
Fastdds and Cyclonedds can pass all tests. Wait for the opinions for #331 (comment). |
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)
I think you want M and U (or ΔM and ΔU), so for U you would simply need to use 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 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. This will lose 'change' information. |
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. |
Yes. User may want to get the count of match/unmatch change between read status twice.
Yes. About unmatch change, we can get by
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
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 🤓 |
Current codes only return
For unmatched case, 3-to-8-to-5 leads to (assume status has been read while reader count is 3.
So, matched count is total_count_change (5) and unmatched count is
Actually, we don't need to care about each event. Instead, we care about
Thanks for your kind reminder. I should check code to keep thread-safe. |
The cause for unstable on window is related to this warning
But PRs didn't modify this file. |
That was fixed in ros2/rclcpp#2117 , and can be ignored in this PR. |
typedef struct rmw_matched_unmatched_status_s rmw_matched_status_t; | ||
typedef struct rmw_matched_unmatched_status_s rmw_unmatched_status_t; |
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.
After a discussion in the middleware working group, there is an important decision to make here about whether or not this should be two events or one (matched and unmatched events vs a single event).
On the one hand, it's perhaps more intuitive for users to see them as separate events, but at the DDS level the information comes at us in a single event, which could be a coalescence of multiple match and unmatch events. It is possible to represent any single DDS event with multiple of the rmw events as proposed here, without missing anything, but there is one corner case which is that you may produce the events in the wrong order.
Consider:
- DDS event with current change = 0, but total change = 4 (previous was 0)
- you can infer four separate events, two match and two unmatch, and
- you can split these into two separate rmw events, but you don't know the order, but you must present the rmw events in some order (arbitrarily perhaps), leading the user to believe they happened separately
- Send rmw matched event, with matched count of 2 and current count change of 2
- Send rmw unmatched event, with matched count of 0 and current count change of -2 (or 2 if we use it absolutely)
This might mistakenly imply to the user that we got two matched and then two unmatched, but in reality we may have gotten "match, unmatch, match, unmatch", or something else.
The single DDS event also does not have this information, but because it is a single event it also does not imply there was an order. Which seems more, not sure the right word, maybe "honest"? "correct"?
I see a few things we could do about this:
- use a single event to encapsulate both matched and unmatched at the same time, like DDS
- document this issue and pick a strategy, e.g. always do matched then unmatched
- always return this event as a sequence of events, so multiple can "occur at the same time", kind of similar to having a single event that encapsulates both
Personally, I like the idea of having a structure like DDS (containing both kinds of events) and breaking it into single events further up the stack, so the user can have the whole story with this interface, or get them one at a time (with the caveat about arbitrarily serializing the events), as they choose. But I don't feel strongly about 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.
@wjwwood thanks for your information.
So the decision is to use one structure like DDS for matched and unmatched event and use one event.
About event order, user can depend on rmw_event_set_callback(). If set callback for matched/unmatched event, user can get the nonfiction while the event occurs.
If only one event, user cannot know which matched or unmatched event happen. User must use rmw_take_event() to get status structure to further check.
Matched status structure returned rmw_take_event() means status changes from last calling rmw_take_event. As you said, it cannot know the order of event.
Whether we keep the same items in matched event structure like DDS event ? (All information from DDS are all provided to user.)
struct matched_status_s {
uint32_t total_count;
int32_t total_count_change;
uint32_t current_count;
int32_t current_count_change;
};
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.
After a discussion in the middleware working group, there is an important decision to make here about whether or not this should be two events or one (matched and unmatched events vs a single event).
I missed the working group because I had another meeting that overlapped. In my opinion, the only useful information for an application (even more if we take into account the motivation in #330) is the value of current_count
. So I think it would be enough with a single matched_count_change
event with the current_count
value.
A value of '0' means the topic is "disconnected". A possitive value means the topic is "connected".
I think this matches the expectations mentioned in #330, is easier to implement, and the meaning will always be correct.
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.
DDS event with current change = 0, but total change = 4 (previous was 0)
(not really the point, but just checking my understanding)
I think in this case, there has been 4 increase, and 4 decrease, since current change is 0.
The single DDS event also does not have this information, but because it is a single event it also does not imply there was an order.
true. if we have 2 events, we have to create the order which we do not really know.
So the decision is to use one structure like DDS for matched and unmatched event and use one event.
yes, i believe so. i think what we can do is to create the event tells difference
.
About event order, user can depend on rmw_event_set_callback(). If set callback for matched/unmatched event, user can get the nonfiction while the event occurs.
the problem is this order generated in rmw, right? as mentioned before, if there is a single event mixed with matched and unmatched, there should be no order.
If only one event, user cannot know which matched or unmatched event happen. User must use rmw_take_event() to get status structure to further check.
this is true, callbacks are mapped to event type, so interface does not allow us to determine event type inside the callback.
user callback can know the either events happened, then taking data via rmw_take_event() to figure out which event happened.
So I think it would be enough with a single matched_count_change event with the current_count value.
A value of '0' means the topic is "disconnected". A possitive value means the topic is "connected".
I think this matches the expectations mentioned in #330, is easier to implement, and the meaning will always be correct.
probably we can take this as is_connected
not matched
event, i think this proposal almost covers requirement.
but i think we need to consider the following cases?
- DDS current_count: 0 -> 1 or greater ,
connected
event. - DDS current_count: 1 -> 2 or greater , no event? cz already
connected
. - DDS current_count: 2 or greater -> 1 , no event? cz still
connected
. - DDS current_count: 1 or greater -> 0 ,
disconnected
event. - DDS current_count: X -> X , no event? cz still nothing seems to change?. (connectivity map could be changed.)
to cover #330 (comment), obviously we do not need 2,3 and 5 cases.
on the other hand, we can pass all 1-5 cases then application can figure out what happened with this event.
if we take the path with the former, the following pseudo code makes sense?
if (current_count_change == 0) {
// skip generating event, since there is no change for number of connection.
}
if (current_count_change > 0) {
if (current_count - current_count_change == 0) {
// CONNECT event, transition 0 to current_count_change
} else {
// skip generating event, since it bas been already connected.
}
} else {
if (current_count == 0) {
// DISCONNECT event, transition X to 0
} else {
// skip generating event, since it is being still connected.
}
}
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 missed the working group because I had another meeting that overlapped. In my opinion, the only useful information for an application (even more if we take into account the motivation in #330) is the value of current_count. So I think it would be enough with a single matched_count_change event with the current_count value.
A value of '0' means the topic is "disconnected". A possitive value means the topic is "connected".
I think this matches the expectations mentioned in #330, is easier to implement, and the meaning will always be correct.
@MiguelCompany About matched_count_change
, how to get this value ? Do you means matched_count_change is current_count_change ?
If yes, this will lose some information. I think total_count_change
is important.
User can get how many matched event happened by total_count_change. Similarly, the number of unmatched event is gotten by total_count_change - current_count
.
Of course, your suggestion can match the expectations mentioned in #330. However, I am concerned that there may be other requirements later on. So I want to provide users with as much information as possible that DDS can offer. Based on the information we provide, users can obtain the information they want.
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 single DDS event also does not have this information, but because it is a single event it also does not imply there was an order.
true. if we have 2 events, we have to create the order which we do not really know.
rmw_event_set_callback() can set callback to DDS layer. If we use 2 events, I think we can make sure the sequence of calling callback is the order of matched/unmatched event.
if we take the path with the former, the following pseudo code makes sense?
...
Yes. It can find the first connection and last disconnection.
as far as i can see, we got 3 options (all single event.).
IMO, option 2 looks good to me. @Barry-Xu-2018 @iuhilnehc-ynos @clalancette @wjwwood @MiguelCompany @asorbini @eboasson what do you think? |
Yeah, I also incline this option. For |
I vote for option 1 😃 if we named the structure with e.g. a case using |
yeah, with on the other hand, option 3 can reduce the number of events for user application significantly, user application does not need to be notified with the event for any thoughts? |
Yeah. First connection event and last disconnection event is real use case from user. |
@Barry-Xu-2018 @iuhilnehc-ynos I think we can go with edit, that means option 3 described in #331 (comment) |
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
f2c0310
to
6276281
Compare
Do rebase |
Signed-off-by: Barry Xu <barry.xu@sony.com>
I modify codes in rmw_fastrtps and rcl. |
CI: Jenkins Job for Linux is not run before, we can use the old job id which can fetch the latest commit id for all PRs. |
Failed cases on Windows are projectroot.test_n_nodes__rmw_cyclonedds_cpp rcl.TestLogRosoutFixtureGeneral__rmw_connextdds.test_multi_add_remove_sublogger_message test_rclcpp.TestNNodes.test_10_nodes test_rclcpp.TestNNodesAfterShutdown.test_10_nodes projectroot.test.test_logging_rosout__rmw_connextdds These test cases aren't related to PRs' change. |
@Barry-Xu-2018 thanks for working on this. @clalancette the following PRs need to be merged together.
But i do not have merge permission for https://github.com/ros2/rmw_cyclonedds, could you help us to merge those? |
Related to #330