Add latest time zero-order-hold sync policy #73

andermi · 2022-05-13T17:19:27Z

Rate logic added to #38.
Computes rates of received messages and pivots on fastest topic; essentially, upsamples slower topics with a zero-order hold.

Signed-off-by: Michael Anderson <[email protected]>

clalancette

I've left a few comments inline.

Besides that, it might be nice to add in some documentation to the top of latest_time.h that describes briefly what this sync policy does, and how to use it.

include/message_filters/sync_policies/latest_time.h

test/test_latest_time_policy.cpp

Co-authored-by: Chris Lalancette <[email protected]>

Signed-off-by: Michael Anderson <[email protected]>

andermi · 2022-05-16T22:14:14Z

Thanks @clalancette ! I added some some documentation at the top of the header.

chapulina

The functionality works for me. I have some questions and suggestions. Take it all with a grain of salt since it's my first time touching this repo 😄

include/message_filters/sync_policies/latest_time.h

test/test_latest_time_policy.cpp

include/message_filters/sync_policies/latest_time.h

Co-authored-by: Louise Poubel <[email protected]>

…ne is selected Signed-off-by: Michael Anderson <[email protected]>

…ndermi/message_filters into andermi/latest_time_zoh_policy

Signed-off-by: Michael Anderson <[email protected]>

test/test_latest_time_policy.cpp

Signed-off-by: Michael Anderson <[email protected]>

chapulina

Thanks for iterating, LGTM! I'd appreciate a review from one of the maintainers though.

clalancette

Overall, looks pretty good to me. I've left a few things inline that I think we should address.

include/message_filters/sync_policies/latest_time.h

Co-authored-by: Chris Lalancette <[email protected]>

…igs; other PR comments Signed-off-by: Michael Anderson <[email protected]>

andermi · 2022-06-06T22:33:34Z

@clalancette I think/hope I've addressed your comments.

clalancette

All right, one last pass. Once these things are fixed, we'll be ready to run CI on it.

include/message_filters/sync_policies/latest_time.h

test/test_latest_time_policy.cpp

Signed-off-by: Michael Anderson <[email protected]>

clalancette

Looks good to me! The next step here is to run CI, but we are currently having some infrastructure issues with CI. Once those are resolved I'll do the CI run.

clalancette · 2022-06-08T12:28:09Z

CI:

Linux
Linux-aarch64
Windows

andermi · 2022-06-08T18:56:41Z

Windows

ugh... timing in windows is unstable in general 😝 . I think I can fix it.

Signed-off-by: Michael Anderson <[email protected]>

andermi · 2022-06-08T19:25:29Z

I actually did find a small intermittent bug

clalancette · 2022-06-09T16:36:03Z

Let's run Windows CI again to see where we are at: * Windows

chapulina

It looks like there are some Windows warnings being introduced by this PR:

https://ci.ros2.org/job/ci_windows/17217/msbuild/new/#issuesContent

test/test_latest_time_policy.cpp

quarkytale · 2022-07-12T21:50:28Z

Retriggered CI

Linux
Linux-aarch64
Windows

quarkytale · 2022-07-28T20:56:39Z

This is currently blocked by timing issue in windows CI.
There are two EMA filters tuned for Linux:

mean rate for incoming message
mean error from 1 which is similar to stddev

In Linux, a threshold of 10 * EMA#2 is used for detecting if the message rate changed. On windows, the rate change is not being detected the same way.

Signed-off-by: Michael Anderson <[email protected]>

andermi · 2022-08-12T03:44:13Z

@clalancette, @quarkytale I gave it another shot... I spun up ros2 dev on my windows machine.
The test passes using sim /clock to control the test timing

quarkytale · 2022-08-15T21:26:30Z

Retriggered Windows CI

andermi · 2022-08-16T00:16:45Z

Looks like compiler warnings causing unstable. Working through them now.

Signed-off-by: Michael Anderson <[email protected]>

andermi · 2022-08-16T00:28:25Z

@quarkytale would you mind running another one? I fixed the 7 msbuild warnings related to conversion from size_t. Can you see anything else to fix first?

quarkytale · 2022-08-16T04:15:23Z

I'm not sure if the warnings were an issue, but good to have them cleared. Seems like some tests are being skipped, that might be the reason for unstable status. Other things that stood out to me:

[WARNING] [python.exe-3]: 'SIGINT' sent to process[python.exe-3] not supported on Windows, escalating to 'SIGTERM'
[-ERROR-] IO exception has been thrown: java.nio.file.NoSuchFileException: C:\ci\ws\src\ros2\message_filters\include\message_filters\sync_policies\latest_time.h

Let's see if any of these persist:

quarkytale

LGTM and CI is green!

adityapande-1995 · 2023-03-09T21:23:25Z

https://github.com/Mergifyio backport humble

mergify · 2023-03-09T21:23:29Z

backport humble

✅ Backports have been created

#89 Add latest time zero-order-hold sync policy (backport #73) has been created for branch humble

* adding latest time zoh policy Signed-off-by: Michael Anderson <[email protected]> * remove commented code * Update include/message_filters/sync_policies/latest_time.h Co-authored-by: Chris Lalancette <[email protected]> * updates from PR comments Signed-off-by: Michael Anderson <[email protected]> * cleanup includes Signed-off-by: Michael Anderson <[email protected]> * adding documentation Signed-off-by: Michael Anderson <[email protected]> * Update include/message_filters/sync_policies/latest_time.h Co-authored-by: Louise Poubel <[email protected]> * address PR comments; add new test for if pivot changes rate and new one is selected Signed-off-by: Michael Anderson <[email protected]> * adding message rate step change detection and EMA filter reset Signed-off-by: Michael Anderson <[email protected]> * whitespace Signed-off-by: Michael Anderson <[email protected]> * whitespace Signed-off-by: Michael Anderson <[email protected]> * detect if pivot message is late Signed-off-by: Michael Anderson <[email protected]> * add ChangeRateLeading test Signed-off-by: Michael Anderson <[email protected]> * whitespace Signed-off-by: Michael Anderson <[email protected]> * Update include/message_filters/sync_policies/latest_time.h Co-authored-by: Chris Lalancette <[email protected]> * add option to take instance of rclcpp::Clock; make magic numbers configs; other PR comments Signed-off-by: Michael Anderson <[email protected]> * clean up; handle zero period Signed-off-by: Michael Anderson <[email protected]> * check for late message after avg error is calculated Signed-off-by: Michael Anderson <[email protected]> * control test timing with sim time Signed-off-by: Michael Anderson <[email protected]> * fix msbuild compiler warnings Signed-off-by: Michael Anderson <[email protected]> Signed-off-by: Michael Anderson <[email protected]> Co-authored-by: Chris Lalancette <[email protected]> Co-authored-by: Louise Poubel <[email protected]> (cherry picked from commit e0055f3)

* adding latest time zoh policy Signed-off-by: Michael Anderson <[email protected]> * remove commented code * Update include/message_filters/sync_policies/latest_time.h Co-authored-by: Chris Lalancette <[email protected]> * updates from PR comments Signed-off-by: Michael Anderson <[email protected]> * cleanup includes Signed-off-by: Michael Anderson <[email protected]> * adding documentation Signed-off-by: Michael Anderson <[email protected]> * Update include/message_filters/sync_policies/latest_time.h Co-authored-by: Louise Poubel <[email protected]> * address PR comments; add new test for if pivot changes rate and new one is selected Signed-off-by: Michael Anderson <[email protected]> * adding message rate step change detection and EMA filter reset Signed-off-by: Michael Anderson <[email protected]> * whitespace Signed-off-by: Michael Anderson <[email protected]> * whitespace Signed-off-by: Michael Anderson <[email protected]> * detect if pivot message is late Signed-off-by: Michael Anderson <[email protected]> * add ChangeRateLeading test Signed-off-by: Michael Anderson <[email protected]> * whitespace Signed-off-by: Michael Anderson <[email protected]> * Update include/message_filters/sync_policies/latest_time.h Co-authored-by: Chris Lalancette <[email protected]> * add option to take instance of rclcpp::Clock; make magic numbers configs; other PR comments Signed-off-by: Michael Anderson <[email protected]> * clean up; handle zero period Signed-off-by: Michael Anderson <[email protected]> * check for late message after avg error is calculated Signed-off-by: Michael Anderson <[email protected]> * control test timing with sim time Signed-off-by: Michael Anderson <[email protected]> * fix msbuild compiler warnings Signed-off-by: Michael Anderson <[email protected]> Signed-off-by: Michael Anderson <[email protected]> Co-authored-by: Chris Lalancette <[email protected]> Co-authored-by: Louise Poubel <[email protected]> (cherry picked from commit e0055f3) Co-authored-by: andermi <[email protected]>

andermi added 2 commits May 13, 2022 10:04

adding latest time zoh policy

852e54e

Signed-off-by: Michael Anderson <[email protected]>

remove commented code

cf4e7b7

andermi marked this pull request as draft May 13, 2022 17:22

andermi marked this pull request as ready for review May 13, 2022 17:22

chapulina self-requested a review May 13, 2022 17:38

clalancette requested changes May 16, 2022

View reviewed changes

andermi and others added 4 commits May 16, 2022 11:55

Update include/message_filters/sync_policies/latest_time.h

7cbc152

Co-authored-by: Chris Lalancette <[email protected]>

updates from PR comments

c90bc60

Signed-off-by: Michael Anderson <[email protected]>

cleanup includes

65f63d7

Signed-off-by: Michael Anderson <[email protected]>

adding documentation

be580c1

Signed-off-by: Michael Anderson <[email protected]>

andermi requested a review from clalancette May 16, 2022 19:46

chapulina reviewed May 17, 2022

View reviewed changes

andermi and others added 3 commits May 17, 2022 12:39

Update include/message_filters/sync_policies/latest_time.h

6e7174d

Co-authored-by: Louise Poubel <[email protected]>

address PR comments; add new test for if pivot changes rate and new o…

d602a47

…ne is selected Signed-off-by: Michael Anderson <[email protected]>

Merge branch 'andermi/latest_time_zoh_policy' of https://github.com/a…

1dbdf65

…ndermi/message_filters into andermi/latest_time_zoh_policy

andermi requested a review from chapulina May 18, 2022 00:34

andermi added 3 commits May 18, 2022 11:39

adding message rate step change detection and EMA filter reset

72fad82

Signed-off-by: Michael Anderson <[email protected]>

whitespace

3b068af

Signed-off-by: Michael Anderson <[email protected]>

whitespace

f1b973c

Signed-off-by: Michael Anderson <[email protected]>

chapulina reviewed May 18, 2022

View reviewed changes

test/test_latest_time_policy.cpp Outdated Show resolved Hide resolved

andermi added 3 commits May 18, 2022 17:23

detect if pivot message is late

c834584

Signed-off-by: Michael Anderson <[email protected]>

add ChangeRateLeading test

d15f2de

Signed-off-by: Michael Anderson <[email protected]>

whitespace

dcac3bd

Signed-off-by: Michael Anderson <[email protected]>

chapulina approved these changes Jun 2, 2022

View reviewed changes

clalancette requested changes Jun 3, 2022

View reviewed changes

andermi and others added 2 commits June 3, 2022 12:56

Update include/message_filters/sync_policies/latest_time.h

51d5343

Co-authored-by: Chris Lalancette <[email protected]>

add option to take instance of rclcpp::Clock; make magic numbers conf…

c61a951

…igs; other PR comments Signed-off-by: Michael Anderson <[email protected]>

andermi requested a review from clalancette June 6, 2022 22:33

clalancette requested changes Jun 7, 2022

View reviewed changes

clean up; handle zero period

1e5e7cb

Signed-off-by: Michael Anderson <[email protected]>

clalancette approved these changes Jun 7, 2022

View reviewed changes

check for late message after avg error is calculated

869b31e

Signed-off-by: Michael Anderson <[email protected]>

chapulina reviewed Jun 15, 2022

View reviewed changes

test/test_latest_time_policy.cpp Outdated Show resolved Hide resolved

audrow changed the base branch from master to rolling June 28, 2022 14:19

clalancette assigned quarkytale Jul 28, 2022

control test timing with sim time

b02300f

Signed-off-by: Michael Anderson <[email protected]>

andermi requested review from clalancette and chapulina August 12, 2022 03:44

fix msbuild compiler warnings

86c7440

Signed-off-by: Michael Anderson <[email protected]>

quarkytale approved these changes Aug 16, 2022

View reviewed changes

gbiggs merged commit e0055f3 into ros2:rolling Aug 16, 2022

mergify bot mentioned this pull request Mar 9, 2023

Add latest time zero-order-hold sync policy (backport #73) #89

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add latest time zero-order-hold sync policy #73

Add latest time zero-order-hold sync policy #73

andermi commented May 13, 2022

clalancette left a comment

andermi commented May 16, 2022

chapulina left a comment

chapulina left a comment

clalancette left a comment

andermi commented Jun 6, 2022

clalancette left a comment

clalancette left a comment

clalancette commented Jun 8, 2022

andermi commented Jun 8, 2022

andermi commented Jun 8, 2022

clalancette commented Jun 9, 2022

chapulina left a comment

quarkytale commented Jul 12, 2022

quarkytale commented Jul 28, 2022

andermi commented Aug 12, 2022 •

edited

Loading

quarkytale commented Aug 15, 2022

andermi commented Aug 16, 2022

andermi commented Aug 16, 2022

quarkytale commented Aug 16, 2022

quarkytale left a comment

adityapande-1995 commented Mar 9, 2023

mergify bot commented Mar 9, 2023 •

edited

Loading

Add latest time zero-order-hold sync policy #73

Add latest time zero-order-hold sync policy #73

Conversation

andermi commented May 13, 2022

clalancette left a comment

Choose a reason for hiding this comment

andermi commented May 16, 2022

chapulina left a comment

Choose a reason for hiding this comment

chapulina left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

andermi commented Jun 6, 2022

clalancette left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented Jun 8, 2022

andermi commented Jun 8, 2022

andermi commented Jun 8, 2022

clalancette commented Jun 9, 2022

chapulina left a comment

Choose a reason for hiding this comment

quarkytale commented Jul 12, 2022

quarkytale commented Jul 28, 2022

andermi commented Aug 12, 2022 • edited Loading

quarkytale commented Aug 15, 2022

andermi commented Aug 16, 2022

andermi commented Aug 16, 2022

quarkytale commented Aug 16, 2022

quarkytale left a comment

Choose a reason for hiding this comment

adityapande-1995 commented Mar 9, 2023

mergify bot commented Mar 9, 2023 • edited Loading

✅ Backports have been created

andermi commented Aug 12, 2022 •

edited

Loading

mergify bot commented Mar 9, 2023 •

edited

Loading