-
Notifications
You must be signed in to change notification settings - Fork 237
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 method to take with message_info #542
Add method to take with message_info #542
Conversation
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 think it would be nice to have a basic test for this, because right now it could segfault or something and CI would not show it.
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
I've reworked this to take comments into acount, but we still have several test failures that I can't really figure out right now. I'll look into it tomorrow. |
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
OK, I fixed the tests. It was a really simple issue because of a bad merge. btw, since I now integrated taking the message-info with the "normal" rcl_take, we can be sure it won't segfault at least. the correctness of the timestamps is not yet checked, because none of the existing tests can be easily modified, I will have to write a new one. |
I really think we need a basic test to show how it would be used in python and to ensure that the timestamps are getting converted correctly (whether they be 0 or some value). Do you have time to work on that still? I can start testing in the meantime. |
When is the deadline? If it has to be today, that'd be tough. It's already almost 9pm here and I'm also looking at the service timestamps. |
Today is the deadline. |
Alright. I'll get it done. |
There are also some compiler warnings on Windows: https://ci.ros2.org/job/ci_windows/10284/msbuild/new/ If you need to get off, that's fine, just tell me what's not done and I'll do my best to finish things. |
Thanks for the offer. So far, I'm planning to continue for a while, probably at least one more hour or maybe two. Those warnings look easy, I'll get to them. |
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
I have now added a test. It is really very basic, but should do the trick. |
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@wjwwood okay, with the last commit this should now also solve the warning problem on Windows. |
Yup I’m going to rerun ci and stuff ASAP. |
Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
🥳 |
result = _rclpy.rclpy_take(capsule, sub.msg_type, False) | ||
if result is not None: | ||
msg, info = result | ||
self.assertNotEqual(0, info['source_timestamp']) |
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.
This test always fails with Connext: see #615.
This implements the Python side of ros2/design#259 and needs ros2/rcl#619
The message info is simply returned as a dictionary. I figured if we want to encapsulate it into something else, we can always do that in Python. That shouldn't break API.
So far, only subscriptions have a method to take with info.