C typesupport tests #21
Conversation
wjwwood
force-pushed
the
c_typesupport_tests
branch
from
208ba02
to
772df53
Compare
|
@jacquelinekay I had to roll back your two commits locally since I'm not building with your Connext c type support branch yet. |
|
I merged Connext C typesupport, so I've added back building for all rmw implementations. |
|
get_rcl_information added in 1354647 I also messed around with how you build test_publisher as an example for myself, feel free to change that (it will probably conflict with your changes). |
| @@ -20,7 +20,7 @@ extern "C" | |||
| { | |||
| #endif | |||
|
|
|||
| #include "rosidl_generator_c/message_type_support.h" | |||
| #include "rosidl_generator_c/message_type_support_struct.h" | |||
There was a problem hiding this comment.
@wjwwood I added this in one of my commits--does this look correct to you?
There was a problem hiding this comment.
I had a discussion about this with @esteve because I think it doesn't make sense to include publisher.h without committing to a middleware implementation. message_type_support.h is provided by individual rosidl type support packages, e.g.:
- https://github.com/ros2/rmw_opensplice/blob/master/rosidl_typesupport_opensplice_c/include/rosidl_typesupport_opensplice_c/impl/rosidl_generator_c/message_type_support.h
- https://github.com/ros2/rmw_connext/blob/master/rosidl_typesupport_connext_c/include/rosidl_typesupport_connext_c/impl/rosidl_generator_c/message_type_support.h
Where as message_type_support_struct.h is provided by the rmw implementation agnostic rosidl_generator_c.
So I'd say no, it's not quite right and instead changing this just worked around another issue which was that something you were building needed a specific type support but wasn't using its include directories.
Separately we discussed whether or not it made since to do this in publisher.h and/or subscription.h but not in things like node.h. It is absolutely necessary in node.h since that will use messages from rcl_interfaces and once you include it you will need to be tied to a specific rmw implementation, but technically publisher and subscription do not have this restriction, but they could in the future.
There was a problem hiding this comment.
Got it. I'll revert it. I think I was trying to fix issues that were related to the linking problems we had earlier.
|
Ok, I think this is ready for review. It needs squashing still. Currently tests are only created for OpenSplice and Connext static as there are issues with type support introspect and C. There are two related pull requests:
Also the Here is CI:
Thanks @jacquelinekay for helping me get this sorted out today. |
wjwwood
added
in review
| @@ -28,7 +28,6 @@ set(${PROJECT_NAME}_sources | |||
| src/rcl/time.c | |||
| src/rcl/timer.c | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
Why remove the empty line? Seems to help to distinguish the following macro from inline code.
There was a problem hiding this comment.
I don't know, restored in 65aa36b
|
I only got partially through it. I have to read the actual tests again. |
|
The Linux job is green and the OS X job has 6 test failures, but they're all in rclcpp and look to be timeouts. I don't think the OS X test failures are related to these pr's. On Windows the I don't think we need to rerun Linux or OS X since on the |
|
@jacquelinekay could you have a look at the comments regarding the |
| if (!malloc_expected) { | ||
| MALLOC_PRINTF( | ||
| " malloc (%s) %p %" PRIu64 "\n", | ||
| malloc_expected ? " expected" : "not expected", memory, fw_size); |
There was a problem hiding this comment.
Can you move the expected into the string one line above and only insert the not or three spaces based on the condition?
Same below.
There was a problem hiding this comment.
I want to leave that for now, as I didn't always have the if statement around it and it's useful to remove it temporarily to see mallocs that are happening outside of the critical section.
|
Beside the comments this LGTM. |
|
I added rmw implementation-specific classnames as William suggested. Running a CI job to see that the reporting is correct on Jenkins. |
|
+1 |
|
I fixed the tests on Windows, waiting for CI: |
|
There are some failing tests on Windows, but based on the comments in #12 I think they might exist on master. I think we should merge this and #12 and address the test failure separately. Other than those failing tests, I believe this pr is good to go. I'm going to squash and run one more Linux CI after squashing and then merge. However, since #12 conflicts with this one, I'm going to review and merge that one first and then rebase this one. |
|
@wjwwood I confirmed that the rcl tests exist on master in http://ci.ros2.org/job/ci_windows/1054/#showFailuresLink I didn't realize you wanted to look at #12 I just merged it. If you have any more comments on it let me know and I can fix it up. Thanks for taking on the rebase, I think it should be relatively quick. |
wjwwood
force-pushed
the
c_typesupport_tests
branch
from
cee6d05
to
b2acdd1
Compare
|
Rebased, waiting on Linux CI to merge: http://ci.ros2.org/job/ci_linux/962 |
|
CI is good, merging. |
add set error msg macro and improve error api
…with-actions-support Add APIs to support actions on EventsExecutor
This pull request adds more tests to test the end to end functionality of C type support.
There are some temporary hacks to limit the tests to the type support implementations that support C properly (OpenSplice and Connext static).
There are two related pull requests:
Also the
examplesrepository has a branch that matches this one, but I do not propose we merge that right now. It was mostly a test to run locally and I believe the tests inrclnow cover the same functionality of the code.