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

Fix for liveliness changed struct [5765] #578

Merged
merged 5 commits into from
Jun 28, 2019
Merged

Fix for liveliness changed struct [5765] #578

merged 5 commits into from
Jun 28, 2019

Conversation

raquelalvarezbanos
Copy link
Contributor

No description provided.

@raquelalvarezbanos raquelalvarezbanos changed the title Fix for liveliness changed struct Fix for liveliness changed struct [5765] Jun 27, 2019
MiguelCompany
MiguelCompany requested changes Jun 27, 2019
View reviewed changes
include/fastrtps/rtps/builtin/liveliness/WLP.h Outdated
const GUID_t& writer,
const LivelinessQosPolicyKind& kind,
const Duration_t& lease_duration);
const Duration_t& lease_duration,
int alive_change,
Copy link
Member

Choose a reason for hiding this comment

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

This parameter and the next one should be int32_t, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is changed now

include/fastrtps/rtps/writer/LivelinessManager.h Outdated
const GUID_t&,
const LivelinessQosPolicyKind&,
const Duration_t&)>& liveliness_recovered_callback,
const Duration_t&,
Copy link
Member

Choose a reason for hiding this comment

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

An alias for the std::function type will improve readability.

include/fastrtps/rtps/writer/LivelinessManager.h Outdated
std::function<void(
const GUID_t&,
const LivelinessQosPolicyKind&,
const Duration_t&)> liveliness_recovered_callback_;
const Duration_t&,
Copy link
Member

Choose a reason for hiding this comment

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

Use alias type here

src/cpp/rtps/builtin/liveliness/WLP.cpp Show resolved Hide resolved
src/cpp/rtps/builtin/liveliness/WLP.cpp Show resolved Hide resolved
src/cpp/rtps/writer/LivelinessManager.cpp Outdated
const GUID_t&,
const LivelinessQosPolicyKind&,
const Duration_t&)>& liveliness_recovered_callback,
const Duration_t&,
Copy link
Member

Choose a reason for hiding this comment

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

Use alias type here

src/cpp/rtps/writer/LivelinessManager.cpp Outdated
@@ -137,34 +154,53 @@ bool LivelinessManager::assert_liveliness(
{
if (w.kind == wit->kind)
{
if (w.alive == false)
if (callback_ != nullptr)
Copy link
Member

Choose a reason for hiding this comment

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

This code is repeated in several places. I propose to include methods inside LivelinessData struct to handle the status transitions. They should either receive the callback as a parameter or have out parameters to fill the increments and return a boolean indicating if the callback should be performed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved some common code into a function

@MiguelCompany MiguelCompany mentioned this pull request Jun 27, 2019
Merged
@MiguelCompany MiguelCompany added this to the v1.8.1 milestone Jun 27, 2019
@raquelalvarezbanos
Copy link
Contributor Author

raquelalvarezbanos commented Jun 27, 2019 •
edited
Loading

Testing this branch against release/1.8.1:

  • All tests passed on Ubuntu
  • All tests passed on mac
  • All tests passed on Windows (2015 and 2017)

@nuclearsandwich
Copy link
Contributor

This branch resolved the issues in ROS 2's test suite ros2/rmw_fastrtps#296 (comment)

@MiguelCompany MiguelCompany merged commit 17fee82 into release/1.8.1 Jun 28, 2019
@MiguelCompany MiguelCompany deleted the bugfix/liveliness-changed-struct branch June 28, 2019 05:24
raquelalvarezbanos pushed a commit that referenced this pull request Jun 28, 2019
@MiguelCompany @raquelalvarezbanos
Release 1.8.1 [5693] (#574)
0835a86 
* Refs #5496 Reordering code to avoid data race on StatefulWriter destructor. (#546)

* Fix lroundl error on Windows. [5505] (#549)

* Refs #5313 Fix lroundl error on Windows.

* Refs #5505 Sugested changes

* Refs #5505 Removed unnecessary parenthesis and clang warnings.

* Adds tests for ReaderHistory [5503] (#547)

* Refs #5486. First tests for ReaderHistory

GetRepeatedSequenceNumber test will fail until the bug is fixed

* Refs #5486. Added more tests for ReaderHistory

GetChange test will fail until the bug is fixed

Also fixed a bug in ReaderHistory::remove_changes_with_guid function. The comparison between writerGUIDs was wrong. operator == is used instead.

* Refs #5486. Fixed memory leaks

* Refs #5486. Fixed Windows warnings

* Refs #5486. Code Style

* Refs #5486. Fixed Mac Warning

* Refs #5486. Fixed Mac Warning

* Refs #5486. Removed set_reader function from ReaderHistory. Added new constructor to the StatefulReader mock instead.

* Refs #5486. Added default constructors

* Refs #5486. Removed code needed

* Refs #5486. Fix error in RTPSReader mock (security did not pass)

* Fixing data race on UDPTransportInterface [5458] (#540)

* Refs #5458. Fixing data race on UDPTransportInterface.

* Refs #5458. Fixing timeouts on Mac.

* Refs #5458 Fixing data race on ReceiverResource dtor.

* Refs #5458 Clang-tidy warnings

* Refs #5458 Fixing Mac timeout.

* Refs #5458 Fixing valgrind invalid read.

* Refs #5458 Requested changes.

* Refs #5458 Fixing situation when socket doesn't close nicely.

* Refs #5458 Trying to fix Mac issue.

* Refs #5458 Trying to fix Mac issue.

* Refs #5458 Trying to fix Mac issue.

* Refs #5458 Fixed Mac issue.

* Refs #5458 Avoid loop when multicast. Much lesser wait time until continue.

* Refs #5458 Requested changes.

* Tests for CacheChangePool [5599] (#557)

* Refs #5551. First tests for CacheChangePool

* Ignoring files for Eclipse and Visual Studio Code

* Refs #5551. Tests for reserve_Cache function

* Refs #5551. Improved tests for reserve_Cache function. Deleted CacheChangePoolAttributes, it uses a tuple and the Combine method instead.

* Refs #5551. Added two more tests for CacheChangePool
Checks the initialized status of a reserved cache_changed
Checks the size of allChanges and freeChanges vectors

* Refs #5552. Blackbox run with prealloc memory mode only

* Refs #5552. Fixed certificate xml files and regenerated smime files
Also fixed bug in BlackboxTestsPersistence. Byte for Prealloc Memory Mode is '3'

* Refs #5551. Fixed gtest comparisons between signed and unsigned types
Replaced EXPECT with ASSERT

* Refs #5551. Fixed Instanciation tests for Mac

* Added discovery regression test [5479] (#542)

* Refs #5453 Adding a test for discovery with thirty participants

* Refs #5453 Updating the test to check for matched readers/writers

* Refs #5453. Added test for multiple participants with multiple topics.

* Refs #5479. Fixing warnings on Mac.

* Refs #5479. Changed test to fail by timeout pattern.

* Refs #5479. Taking removal and unmatching into account.

* Refs #5479. Styling and no copy.

* Refs #5479. Fixing valgrind.

* Refs #5479. Avoiding possible port collisions.

* Refs #5479. Removing debug print methods.

* Refs #5479. Adding debug messages to persistence tests.

* Refs #5479. Closing participants.

* Refs #5479. Adding more info to persistence tests.

* Refs #5479. Fix build errors after rebase.

* Refs #4579. Addressing review comments.

* Log error when default profiles xml loads with error + unittest [5596] (#556)

* Log error when default profiles xml loads with error + unittest

* More verbose error

* log opening error also when the file is DEFAULT_FASTRTPS_PROFILES

* combine xml loading and error checking

* Using java 1.8 [5705] (#567)

* Refs #5693. Updating idl and fastcdr submodules

* Refs #5697. Targetting java 1.8

* Refs #5697. Adding linter options.

* Refs #5697. Fixing warnings.

* Do not print the error if the filename is the default (#568)

* Refs #5746: delete the periodic HB after stopping reader proxies (#572)

* Fix closing multicast UDP channel with whitelist [5732] (#569)

* Refs #5714. Added unit test.

* Refs #3463. On releasing UDP resources, if the closing of the UDP fails 10 times, shut it down manually.

* Refactor without sending close message

* Refs #5714 remove UDPTransportInterface::ReleaseInputChannel

* Wait until the UDPChannelResource::perform_listen_operation has joined before deleting the channel resource

* Refs #5714 Simplify channel resource release mechanism. Eliminates condition variable and mutex

* Adds liveliness QoS [5621] (#560)

* Refs #5183 Adding C++ example of liveliness with one AUTOMATIC publisher

* Refs #5183 Extending C++ examples to allow using a participant with two publishers

* Refs #5183 Extending C++ example to allow configuration of liveliness kind

* Refs #5060 Format changes only

* Refs #5060 Removing WriterProxyLiveliness and adding new classes

* Refs #5060 Adding liveliness manager to RTPSReader and WLP

* Refs #5060 Removing liveliness information from WriterProxyData

* Refs 5060 Adding functionality to liveliness manager + unit tests

* Refs #5060 Adding liveliness methods at the reader-writer layer

* Refs #5060 Updating WLP so that it invokes writer listener when liveliness of writer is lost

* Refs #5060 Adding callbacks to pub-sub layer

* Refs #5060 Implementing getters for liveliness status

* Refs #5060 Changes to implement assert_liveliness() method

* Refs #5060 Adding option to liveliness manager to exclude automatic writers

* Refs #5060 Fixing WLP so that liveliness manager is created when WLP is initialized

* Refs #5060 Fixing WLP so that liveliness manager is created when WLP is initialized. Also liveliness is not changeable

* Refs #5060 Changes to WLP to make AUTOMATIC (writer) - AUTOMATIC (reader) work

* Refs #5060 Changes to WLP to make MANUAL_BY_PARTICIPANT (w) - MANUAL_BY_PARTICIPANT (r) work

* Refs #5060 Changes to WLP to make MANUAL_BY_TOPIC (w) - MANUAL_BY_TOPIC (r) work

* Refs #5060 Changes to stateless reader and writer

* Refs #5060 Implementing assert_liveliness method

* Refs #5060 Changes to allow different liveliness kinds between writer/reader

* Refs #5060 Combining some blackbox tests

* Refs #5060 Code clean-up

* Refs #5060 Testing more than one pub/sub in the same participant

* Refs #5060 Code clean up

* Refs #5060 Fixing deadlock

* Refs #5060 More code clean up

* Refs #5060 Trying to fix builds

* Refs #5060 Trying to fix build of tests

* Refs #5060 Fixing error message that shouldn't be logged

* Refs #5060 Fixing warnings and trying to make tests for stable when running valgrind

* Refs #5060 Addressing review comments

* Refs #5060 Adding a python test for automatic liveliness

* Refs #5058 Fixing windows builds

* Refs #5060 Fixing mac warnings

* Refs #5060 Fixing mac build

* Refs #5060 Fixing failing test on mac

* Refs #5060 Trying to make some tests more stable when using valgrind

* Refs #5060 Trying to make tests more stable by using condition variables

* Refs #5060 Resolving remaining review comments

* Refs #5060 Making some tests more stable on windows

* Refs #5060 Fixing remaining tabs

* Refs #5060 Fixing mac warning

* Refs #5060 Making some ests more stable on mac

* Refs #5060 Trying to fix remaining test that is failing on mac

* Refs #5060 Removing asserts and letting timers manage non-positive intervals

* Refs #5060 Adding explanation about non-positive intervals

* Refs #5693. Bump version to 1.8.1

* Refs #5761 Fixing liveliness manager tests that would fail with GTEST_INDIVIDUAL OFF (#577)

* Fix for liveliness changed struct (#578)

* Refs #5765 Adding a test

* Refs #5765 Fix for liveliness changed status

* Refs #5765 Adding more tests

* Refs #5765 Addressing some review comments

* Refs #5765 Adding indentation

* Adding assert_liveliness to participant (#579)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiguelCompany MiguelCompany MiguelCompany approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.8.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@raquelalvarezbanos @nuclearsandwich @MiguelCompany