Skip to content

Add method to take with message_info#542

Merged
wjwwood merged 7 commits intoros2:masterfrom
boschresearch:feature/message_info_timestamp
Apr 23, 2020
Merged

Add method to take with message_info#542
wjwwood merged 7 commits intoros2:masterfrom
boschresearch:feature/message_info_timestamp

Conversation

@iluetkeb
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member

@wjwwood wjwwood left a 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>
@iluetkeb
Copy link
Copy Markdown
Contributor Author

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>
@iluetkeb
Copy link
Copy Markdown
Contributor Author

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.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 22, 2020

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.

@iluetkeb
Copy link
Copy Markdown
Contributor Author

iluetkeb commented Apr 22, 2020

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?

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.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 22, 2020

Today is the deadline.

@iluetkeb
Copy link
Copy Markdown
Contributor Author

iluetkeb commented Apr 22, 2020

Alright. I'll get it done.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 22, 2020

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.

@iluetkeb
Copy link
Copy Markdown
Contributor Author

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>
@iluetkeb
Copy link
Copy Markdown
Contributor Author

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>
@iluetkeb
Copy link
Copy Markdown
Contributor Author

@wjwwood okay, with the last commit this should now also solve the warning problem on Windows.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 22, 2020

Yup I’m going to rerun ci and stuff ASAP.

Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>
@iluetkeb
Copy link
Copy Markdown
Contributor Author

🥳

result = _rclpy.rclpy_take(capsule, sub.msg_type, False)
if result is not None:
msg, info = result
self.assertNotEqual(0, info['source_timestamp'])
Copy link
Copy Markdown
Member

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.

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

Successfully merging this pull request may close these issues.

4 participants