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.

Sign up for GitHub

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

Jump to bottom

Add Deadline, Liveliness, and Lifespan QoS Event (rcl_event_t type) and their rcl_take_* Function #396

Closed
wants to merge 16 commits into from
Closed

Add Deadline, Liveliness, and Lifespan QoS Event (rcl_event_t type) and their rcl_take_* Function #396

wants to merge 16 commits into from

Conversation

mm318
Copy link
Member

@mm318 mm318 commented Mar 8, 2019 •
edited
Loading

Implementation for Deadline, Liveliness, and Lifespan QoS. The design is at ros2/design#212

This change introduces a new rcl_event_t type as well as its respective rcl_take_event() function.

@tfoote tfoote added the in progress Actively being worked on (Kanban column) label Mar 8, 2019
@mm318 mm318 mentioned this pull request Mar 8, 2019
Closed
@mm318 mm318 changed the title Implementation for Deadline, Liveliness, and Lifespan QoS Add Deadline, Liveliness, and Lifespan QoS Event (rcl_event_t type) and their rcl_take_* Function Mar 13, 2019
@mm318 mm318 marked this pull request as ready for review March 13, 2019 20:35
@mm318
Copy link
Member Author

mm318 commented Mar 14, 2019

Hi @wjwwood, can you take a look at this PR?

@thomas-moulard
Copy link
Contributor

thomas-moulard commented Mar 14, 2019 •
edited
Loading

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

mm318 and others added 16 commits March 15, 2019 10:42
@mm318
initial qos interface changes
3d6c8b5 
Signed-off-by: Miaofei <[email protected]>
@mm318
add creation of event waitable type
d140b0b 
Signed-off-by: Miaofei <[email protected]>
@mm318
add event APIs to wait_set
bdf3cbe 
Signed-off-by: Miaofei <[email protected]>
@ross-desmond @mm318
Add rcl_take_event
4945c12 
* EDIT: alphabetical order cmakelist and imports

Signed-off-by: Miaofei <[email protected]>
@mm318
fix some ros2 build issues
2c97fcb 
Signed-off-by: Miaofei <[email protected]>
@mm318
add rcl event type enum
f8e54f4 
Signed-off-by: Miaofei <[email protected]>
@hidmic @mm318
Drops legacy launch API usage. (#387)
cdc4513 
* Drops legacy launch API usage.

Signed-off-by: Michel Hidalgo <[email protected]>

* Adds launch_testing as test dependency.

Signed-off-by: Michel Hidalgo <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@sloretz @mm318
Split test_token to avoid compiler note (#392)
4d470d2 
Signed-off-by: Shane Loretz <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@bhatsach @mm318
Set symbol visibility to hidden for rcl (#391)
a978d15 
* Set symbol visibility to hidden for rcl

Enabling symbol visibility feature in gcc and clang compilers.
This will hep find symbol export related issues in linux and
potentially reduce compile times.

Discourse topic link:
https://discourse.ros.org/t/set-symbol-visibility-to-hidden-for-rmw-and-rcl-packages/7981

Signed-off-by: Sachin Suresh Bhat <[email protected]>

* Remove WIN specific compiler definition in configure_rcl

Signed-off-by: Sachin Suresh Bhat <[email protected]>

* Rename macro name rcl_set_symbol_visibility_hidden

Signed-off-by: Sachin Suresh Bhat <[email protected]>

* Change macro to args for rcl_set_symbol_visibility_hidden

Signed-off-by: Sachin Suresh Bhat <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@hidmic @mm318
Add launch along with launch_testing as test dependencies. (#393)
3ce5bb8 
Signed-off-by: Michel Hidalgo <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@dirk-thomas @mm318
initialize sequence number of client (#395)
75fc947 
Signed-off-by: Dirk Thomas <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@jacobperron @mm318
[rcl_action] Minor documentation fixes (#397)
43af3c0 
* Update rcl_action Doxyfile

Add reference to rcl tagfile and fix generated tagfile name.

Signed-off-by: Jacob Perron <[email protected]>

* Fix minor doc errors

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@dirk-thomas @mm318
update tests to use separated action types (#340)
42d779b 
* update test to IDL-based action generation

* update test to IDL-based action generation

* fix merge conflict

* update new tests

* update action types

* match renamed action types

* style fix

* update types using unique_identifier_msgs

Signed-off-by: Miaofei <[email protected]>
@jacobperron @mm318
Fix typo in Doxyfile
20cd13f 
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@mm318
fix lint errors
fea95bd 
Signed-off-by: Miaofei <[email protected]>
@mm318
apply uncrustify
03c9efe 
Signed-off-by: Miaofei <[email protected]>
wjwwood
wjwwood reviewed Mar 27, 2019
View reviewed changes
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.

This needs a rebase, as there seems to be lots of off-topic changes that are also in master (e.g. updating python code to use non-legacy launch API).

It was kind of hard to reverse engineer the purpose of having a separate init/take for each "kind" of event, especially since there's no API docs for them. Generally, starting with API function prototypes and their API docs is the best place to start if you want comments on your approach.

rcl/include/rcl/event.h Show resolved Hide resolved
rcl/include/rcl/event.h
#include "rcl/subscription.h"
#include "rcl/visibility_control.h"


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's lots of redundant whitespace in this file, please just use one blank line in all these places.

rcl/include/rcl/types.h
@@ -99,6 +99,13 @@ typedef rmw_ret_t rcl_ret_t;
/// Argument is not a valid log level rule
#define RCL_RET_INVALID_LOG_LEVEL_RULE 1020

// rcl event specific ret codes in 20XX
/// Invalid rcl_event_t given return code.
#define RCL_RET_EVENT_INVALID 2000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not 1030?

rcl/src/rcl/types_impl.h Show resolved Hide resolved
rcl/test/rcl/test_timer.cpp Show resolved Hide resolved
rcl/include/rcl/event.h Show resolved Hide resolved
@mm318
Copy link
Member Author

mm318 commented Mar 27, 2019

Sorry that this PR has gone kind of stale. We will update as soon as possible.

The reason for separate init function for each "kind" of event is that the event needs to access the void * data of its respective rmw_publisher_t/rmw_subscription_t/etc.

There are no separate take functions for each "kind" of event.

@wjwwood
Copy link
Member

wjwwood commented Mar 27, 2019

There are no separate take functions for each "kind" of event.

You're right, sorry, I must have misread it.

@mm318
Copy link
Member Author

mm318 commented Mar 29, 2019

This PR is being replaced by #408. Closing this PR.

@mm318 mm318 closed this Mar 29, 2019
@tfoote tfoote removed the in progress Actively being worked on (Kanban column) label Mar 29, 2019
@jayhou
Copy link

jayhou commented Sep 10, 2020

Is thers samples or demos intruduce how to use liveness event by using rcl api?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.