Conversation
fgallegosalido
requested review from
MiguelCompany,
fercsrti77,
fujitatomoya and
lobolanja
fercsrti77
left a comment
fercsrti77
left a comment
There was a problem hiding this comment.
I already reviewed these code changes offline with @fgallegosalido and @lobolanja
|
@fujitatomoya the pull request is ready to be reviewed. |
|
@fgallegosalido we will need to create a repos file to run CI, right? |
lobolanja
left a comment
lobolanja
left a comment
There was a problem hiding this comment.
@fgallegosalido Can you rebase the required branches from the original task to be able to run the tests and so, if you can't directly rebase those branches create a fork and rebase it in your personal repository, so we are able to run the CI.
|
I've opened two prs that let us run the CI with the new tests: |
|
@fujitatomoya could you run the CI for us? Use the following repos file: https://gist.githubusercontent.com/fgallegosalido/299ccbf18342a5b9a10cbe0b6bd88d2f/raw/2e2a14766a2129d6da1c27148eac5ad286e945be/ros2.repos |
|
CI with this repos file:
|
|
@fgallegosalido This fails to build on windows. Could you take a look? |
Signed-off-by: Francisco Gallego Salido <fgallego@rti.com>
|
@MiguelCompany solved |
|
New CI:
|
|
@MiguelCompany CI is green. Unless someone else has any objection, this should be ready to be merged. Waiting for @fujitatomoya 's feedback before closing. We should close this one along with ros2/rmw_fastrtps#753, ros2/rcl_interfaces#173 and ros2/system_tests#568 |
|
@fgallegosalido I agree. I guess we will have to wait till the team in America wakes up. |
|
@fgallegosalido @MiguelCompany 1st thank you very much for putting this together. i think you guys are waiting for the reviews, i can try weekend for rmws, but cannot promise i can complete..... |
|
@fgallegosalido i am not sure how much i can help this, but i will review this 1st thing in the morning tomorrow. @lobolanja can you approve this? |
|
New CI with this repos file:
|
|
@fujitatomoya this has been already reviewed internally, but we just wanted someone from outside the company to give their OK |
lobolanja
left a comment
lobolanja
left a comment
There was a problem hiding this comment.
Everything has been reviewed on RTI’s side, so we’re ready to proceed with the merge.
|
@MiguelCompany if everything looks good to you (changes in this repo, but most importantly the testing stuff), approve this pr |
ahcorde
left a comment
ahcorde
left a comment
There was a problem hiding this comment.
There are some missing includes, probably there are more, I don't want to delay this merge of this PR, we can include them in a follow up PR
Signed-off-by: Francisco Gallego Salido <fgallego@rti.com>
|
@ahcorde feedback applied. No functional change has really been added. If there is a need to run again the CI, go ahead, although the schedule is a bit tight to merge today if we need to rerun the CI. @fujitatomoya what do you think? I can also undo the last commit and cherry-pick it later to avoid running the CI again. |
| void ** data_buffer = nullptr; | ||
|
|
||
| // TODO(fgallegosalido): Use DDS_DataReader_read_or_take_instance_untypedI | ||
| // when ROS2 support for instances is added. |
There was a problem hiding this comment.
precisely ROS2 should be ROS 2, there are other comments already exist with `ROS2, i guess no need to fix 💬
There was a problem hiding this comment.
Will address that during the next release cycle
|
|
||
| static | ||
| void | ||
| RMW_Connext_TypePlugin_destroy_key(void ** key, void * user_data) |
There was a problem hiding this comment.
(not blocking PR, just a suggestion) i would do the following to keep the code cleaner, if that is unused for now.
| RMW_Connext_TypePlugin_destroy_key(void ** key, void * user_data) | |
| RMW_Connext_TypePlugin_destroy_key(void ** key, [[maybe_unused]] void * user_data) |
There was a problem hiding this comment.
That's a really nice addition! I'll make sure to slowly clean up the code after this is merged
| // By default, dispose samples do not contain the serialized key, so this should only be a | ||
| // problem if the DataWriterQos.protocol.serialize_key_with_dispose is set to true. |
There was a problem hiding this comment.
what if the user configure DataWriterQos.protocol.serialize_key_with_dispose as DDS, and comes to this situation? there will be errors when receiving dispose samples, correct? as source code, we can see that is the case it does not support yet, but any user notification can be added? or is this really a corner case, that user barely can configure in that way? just curious that user might or might not be affected with this constraint.
There was a problem hiding this comment.
I don't think many ROS 2 users will use this or find themselves in this codepath. However, we are planning to polish the Keyed Topics implementation later on to cover all these corner cases
| if (!serialize_key) { | ||
| // Not currently supported. As a result, users cannot enable batching by | ||
| // setting writer_qos.batch.enabled to TRUE. | ||
| return RTI_FALSE; | ||
| } |
There was a problem hiding this comment.
user still can configure RTI DDS to enable writer_qos.batch.enabled and ends up having this failure? if this happens, user can know why it is failing with clear error message from RTI connextdds? if not, it would be nice to add the error message that tells user about this limitation explicitly?
|
@fgallegosalido i just did the review, minor concerns for user perspective. can you take a look? i am okay to merge this without changes. |
|
@fgallegosalido @lobolanja is this good to go? |
Yes, good to go! @fujitatomoya I’ll take note of your questions and recommendations and discuss them with @fgallegosalido. We’ll address them in future PRs if needed. For now, we’re confident that the changes already in place are quite stable. |
6 participants
This PR adds the necessary code to support topic instances in
rmw_connextdds(not supported inrmw_connextddsmicro).@keyannotation rosidl#796@keyannotation rosidl_typesupport_fastrtps#116