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

Invalid Heartbeat messages generated under some circumstances [2635] #177

Closed
eboasson opened this issue Jan 8, 2018 · 8 comments
Closed

Comments

@eboasson
Copy link

eboasson commented Jan 8, 2018

In ros2/rmw_opensplice#205 it is reported that OpenSplice sometimes reports:

Description : malformed packet received from vendor 1.15 state parse:heartbeat <52545053
0201010f 76050f01 00005536 00000000 0e010c00 3dd442e5 0000023d @0x24 07011c00
04050000 03010000 00000000 02000000 00000000 00000000 01000000> (note: maybe
partially bswap'd) {{7,1,28},504,103,2,0}

Vendor code 1.15 is Fast-RTPS and OpenSplice is in my view correct in rejecting the message, hence my filing it as an issue here. The problem is with the "first sequence number" and "last sequence number" fields of the messages (the last two numbers in the dump). According to the spec, 1 <= first <= last, which obviously isn't the case here. The spec furthermore requires dropping it when it doesn't meet this requirement ...

I have added workarounds over the years for cases I ran into, but I wasn't aware that Fast-RTPS sent them like this ... The case it does accept has first >= 1 and the last >= first-1, and I suspect it would be no problem for Fast-RTPS to generate that instead. Moreover, that particular form is going to be allowed in the next update of the spec, but it still won't cover the (2,0) received here.

@richiware
Copy link
Member

Thanks for the reporting, I have to investigate this

@dirk-thomas
Copy link
Contributor

@richiprosima Any news on this?

@richiware
Copy link
Member

I'm testing a solution.

@richiware
Copy link
Member

Commit 3514b09 tries to solve those warnings. I cannot check it against OpenSplice.

@eboasson
Copy link
Author

@richiware, I've tried to verify it but I have essentially the same problem: I don't have Fast-RTPS test code that triggers the issue, I have only analyzed the report from someone else. I thought I might get away with changing your throughput test, but because that test uses a transient-local, NO_KEY-based writer, I can't get rid of the data in the writer history. Before writing the first sample, the writer's heartbeat would be [1,0] and that would be accepted anyway.

What I need is a sequence of operations on Fast-RTPS that would previously have generated a heartbeat [N,0] with N > 1. Perhaps you could provide a small program to do that, or perhaps you have a (somewhat) generic tool to perform arbitrary sequences of writes, disposes and unregisters? (Like my https://github.com/ADLINK-IST/opensplice-tools.) That would be most convenient.

That said, I have also looked at the changes you did and I do indeed expect them to address this issue, although I think it is a little more aggressive in suppressing heartbeats than is necessary, which in turn has negative consequences as well. All implementations that I am aware of accept heartbeats of the form [N,N-1] with N >= 1 to indicate that the WHC is empty, and in the DDSI RTF process, it has decided to allow this in a minor release, even though it technically breaks backwards compatibility.

@MarcelJordense
Copy link

When testing ros2 in combination with OpenSplice the problem still occurs.

@MiguelCompany
Copy link
Member

Hi @MarcelJordense. I have tried a new approach in PR #330 that I think should work. Please note that to test against that PR on ROS2, you should also use ros2/rmw_fastrtps#233

@richiware richiware changed the title Invalid Heartbeat messages generated under some circumstances Invalid Heartbeat messages generated under some circumstances [2635] Dec 5, 2018
@MiguelCompany
Copy link
Member

The changes to solve this issue have been merged on master as part of v1.7.0 release. Closing this now. Feel free to reopen if still an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants