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

Implement QoS: liveliness, deadline, lifespan #171

Merged
merged 30 commits into from
May 3, 2019

Conversation

ross-desmond
Copy link
Contributor

@ross-desmond ross-desmond commented Mar 29, 2019

Summary
Provide init/fini and take functions for events. Modify wait_set to include event handles for notification of status changes.

Details:

  • ADD initial qos interface changes
  • ADD rmw take_event interface
  • ADD rmw event type enum
  • MODIFY events type from void** to rmw_event_t**
  • ADD RMW QoS Event Definitions
  • ADD section about DCO to CONTRIBUTING.md
  • EDIT liveliness policy types to reflect actual settings
  • ADD an invalid enum in rmw_event_type_t for variables to default to
  • MOVE rmw_*_event_init implementation to rmw
    ** Event initialization should require no modifications from the underlying rmw layer and therefore can be initialized in the rmw layer.

Signed-off-by: Ross Desmond 44277324+ross-desmond@users.noreply.github.com

Depends on #173

@tfoote tfoote added the in review Waiting for review (Kanban column) label Mar 29, 2019
CONTRIBUTING.md Outdated Show resolved Hide resolved
rmw/include/rmw/event.h Outdated Show resolved Hide resolved
rmw/include/rmw/event.h Outdated Show resolved Hide resolved
rmw/include/rmw/event.h Outdated Show resolved Hide resolved
rmw/include/rmw/event.h Show resolved Hide resolved
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
rmw/src/event.c Outdated Show resolved Hide resolved
rmw/src/event.c Outdated Show resolved Hide resolved
rmw/src/event.c Outdated Show resolved Hide resolved
@dabonnie
Copy link

dabonnie commented Apr 2, 2019

@wjwwood I believe we have addressed all of your comments. Please let us know if there is anything else outstanding given the latest changes.

@dabonnie dabonnie force-pushed the qos_reviewed branch 2 times, most recently from ff6246d to 074c30a Compare April 2, 2019 21:48
@wjwwood
Copy link
Member

wjwwood commented Apr 3, 2019

This branch has conflicts and cannot be merged.

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.

Unfortunately I cannot review it in the current state, as it has lots of commits from master mixed into it.

CONTRIBUTING.md Outdated Show resolved Hide resolved
rmw/cmake/get_rmw_typesupport.cmake Outdated Show resolved Hide resolved
mm318 and others added 17 commits April 4, 2019 13:36
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
…eration of the feature

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Removed unsupported QoS types

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
**Summary**
Event initialization should require no modifications from the underlying rmw layer and therefore can be initialized in the rmw layer.

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Signed-off-by: Burek <burekn@f45c89c6612b.ant.amazon.com>
@mm318
Copy link
Member

mm318 commented May 2, 2019

@thomas-moulard - please run the following CI job:

@thomas-moulard
Copy link

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

@wjwwood
Copy link
Member

wjwwood commented May 2, 2019

Let me know if I can help. Sorry about the last minute collisions, I know they're a pain.

Signed-off-by: Miaofei <miaofei@amazon.com>
@emersonknapp
Copy link
Contributor

Thanks @wjwwood - we're running through it and will let you know - most of the rebases have been fairly straightforward but a few things are throwing us for a loop - hopefully have a good build ready to go soon

@thomas-moulard
Copy link

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

@mm318
Copy link
Member

mm318 commented May 2, 2019

  • Windows Build Status

@thomas-moulard
Copy link

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

@mm318
Copy link
Member

mm318 commented May 3, 2019

HI @wjwwood and @sloretz, can this be merged now based on the #171 (comment) CI run? We will have quick follow up to fix the warnings.

@wjwwood
Copy link
Member

wjwwood commented May 3, 2019

CI with (https://gist.githubusercontent.com/mm318/582ceeae9b2d2efc4c48bfa7c2b98f12/raw/cc86a231fd3f6c20c03e576290a1968fa97ef6c8/ros2.repos and --packages-up-to rcl and --packages-select rcl for testing):

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

@wjwwood
Copy link
Member

wjwwood commented May 3, 2019

We're investigating the test failure on macOS, but they look like flaky (new) tests.

@rutvih
Copy link

rutvih commented May 3, 2019

@wjwwood what are your thoughts on disabling failing tests for now and our team submitting a separate PR for the fix so we can get this merged?

@wjwwood
Copy link
Member

wjwwood commented May 3, 2019

@rutvih I think we're going to do that, I just started a macOS only job to see if that works:

Build Status

@wjwwood
Copy link
Member

wjwwood commented May 3, 2019

Ok, here's full CI:

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

EDIT: The macOS and Windows jobs got disconnected and re-queued due to the router reboot...

I'll merge if it is green 🎉.

@mm318 can you open an issue about fixing that test we disabled, just so we don't loose track of it.

@mm318
Copy link
Member

mm318 commented May 3, 2019

@mm318 can you open an issue about fixing that test we disabled, just so we don't loose track of it.

Sure, will do.

EDIT: Tracked at ros2/rcl#431

@dirk-thomas
Copy link
Member

We're investigating the test failure on macOS, but they look like flaky (new) tests.

How exactly is the failing test flaky? The tests were run with --retest-until-pass 10 and the error output contains:

> failed to resolve symbol 'rmw_get_serialized_message_size' in shared library

which doesn't sound flaky to me. Does it work for people in local testing?

I am fine disabling the new test for now - I would just like to be clear about why / how.

@mm318
Copy link
Member

mm318 commented May 3, 2019

That error output is unrelated to the test failures. rmw_fastrtps_cpp, rmw_fastrtps_dynamic_cpp, and rmw_opensplice_cpp all have rmw_get_serialized_message_size implemented, but rmw_connext_cpp/rmw_connext_shared_cpp does not (and implementing it is not supposed to be part of this set of PRs).

The test is flaky because it passes 90% of the time on @wjwwood's macOS laptop and actually only the RTI Connext version of the test kept failing on the buildfarm while the OpenSplice version did not.

@wjwwood
Copy link
Member

wjwwood commented May 3, 2019

Does it work for people in local testing?

Yes, for what ever reason that message only prints when it fails. I think it might be cached when they are pre-fetched, and only prints when the test fails.

That symbol was added in https://github.com/ros2/rmw_implementation/pull/51/files#diff-0e10897bf5aa86e527bbfbb0e10759e9R299 and not in rmw_connext's pr (https://github.com/ros2/rmw_connext/pull/353/files).

@wjwwood
Copy link
Member

wjwwood commented May 3, 2019

@mjcarroll fyi

@wjwwood
Copy link
Member

wjwwood commented May 3, 2019

Unfortunately the linters are now failing, at some point, around 4 hours ago, code was pushed to the rmw connext branch and it has a line that's too long, see: ros2/rmw_connext#352 (comment)

I cannot get this retested before nightlies start, so we'll have to re-evaluate in the morning.

@mm318
Copy link
Member

mm318 commented May 3, 2019

@wjwwood Would you be able to kick off CI to run overnight?

@tfoote
Copy link

tfoote commented May 3, 2019

Here's a retrigger of the builds

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

@emersonknapp
Copy link
Contributor

@tfoote I'm a bit confused about what we're seeing there - the linux aarch64 one is one that I've seen periodically that seems like it's a buildfarm quirk

projectroot.test_params_yaml
test_cli.test_params_yaml.xunit.missing_result

and the windows has a bunch of test failures for the CLI, which also seems unrelated to what we've done here - considering the previous builds did pass those tests?

@mm318
Copy link
Member

mm318 commented May 3, 2019

@wjwwood @sloretz Can we merge this based on these results? #171 (comment)

@clalancette
Copy link
Contributor

projectroot.test_params_yaml
test_cli.test_params_yaml.xunit.missing_result

This one is actually a bug, but a long running, known bug: ros2/build_farmer#166 . So you can safely ignore that one.

@wjwwood
Copy link
Member

wjwwood commented May 3, 2019

We've decided to merge after looking at the CI manually. 👍

@wjwwood wjwwood merged commit e7ae806 into ros2:master May 3, 2019
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label May 3, 2019
@mm318
Copy link
Member

mm318 commented May 3, 2019

Awesome! Thank you!

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.