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

Add method to take with message_info #542

Merged
merged 7 commits into from Apr 23, 2020
Merged

Add method to take with message_info #542

merged 7 commits into from Apr 23, 2020

Conversation

iluetkeb
Copy link
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.

@hidmic hidmic added this to In progress in Foxy via automation Apr 16, 2020
@wjwwood wjwwood self-assigned this Apr 21, 2020
@wjwwood wjwwood removed this from In progress in Foxy Apr 21, 2020
@wjwwood wjwwood added this to In review in Foxy API Freeze Apr 21, 2020
Copy link
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.

rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
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
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
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
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.

@wjwwood wjwwood removed this from In review in Foxy API Freeze Apr 22, 2020
@iluetkeb
Copy link
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
Member

wjwwood commented Apr 22, 2020

Today is the deadline.

@iluetkeb
Copy link
Contributor Author

iluetkeb commented Apr 22, 2020

Alright. I'll get it done.

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

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

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

🥳

@wjwwood wjwwood merged commit 6fb1696 into ros2:master Apr 23, 2020
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
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.

None yet

3 participants