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

Add policy to store the latest messages #38

Closed
wants to merge 1 commit into from

Conversation

jcmonteiro
Copy link
Contributor

Add policies to store the most recent received messages (based on their timestamp or on the order of arrival); These policies do not call any callback automatically. It is up to the programmer to call the call method when it is time to process the information.

These policies are meant to be used in networks where information is processed periodically.

@ZhenshengLee
Copy link

It is up to the programmer to call the call method when it is time to process the information.

@jcmonteiro

Could you provide some docs to explain the motivation of this policy? or more info about why, when and how we should use this policy?

@jcmonteiro
Copy link
Contributor Author

Hi @ZhenshengLee, it's been a long while since I came up with the proposal, so I lack the exact context. I can say that, back then, there was no mechanism in message_filters to sort messages based on timestamp and ensure the correct order of processing. Back in 2019, I used it for exactly the purpose stated in the description.

These policies are meant to be used in networks where information is processed periodically

Meaning in our robot we had a fixed 25 ms control loop such that, every 25 ms, a node would fetch all its messages and act accordingly.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I know this is really old, so sorry for the long delay in looking at it. If you'd like to give up on getting this in, I'd totally understand.

If you'd still like to move forward, I've left some stuff inline to fix. Also, I'm not a huge fan of the terminology "latest" to describe this policy. While it is accurate, I don't think it will be hugely meaningful to people, and seems too close to "latest_time" (which we already have). Maybe we can rename it to "user controlled", or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please rename this header file to .hpp? I know that isn't consistent with everything else that is in here, but I'd like to move the rest of these headers to .hpp, so for new things we should probably just do that now.

@@ -108,6 +108,11 @@ if(BUILD_TESTING)
target_link_libraries(${PROJECT_NAME}-test_approximate_time_policy ${PROJECT_NAME})
endif()

ament_add_gtest(${PROJECT_NAME}-test_latest_policy test/test_latest_policy.cpp)
if(TARGET ${PROJECT_NAME}-test_latest_policy)
target_link_libraries(${PROJECT_NAME}-test_latest_policy ${PROJECT_NAME})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
target_link_libraries(${PROJECT_NAME}-test_latest_policy ${PROJECT_NAME})
target_link_libraries(${PROJECT_NAME}-test_latest_policy ${PROJECT_NAME} rclcpp::rclcpp)

#ifndef MESSAGE_FILTERS__SYNC_Latest_H_
#define MESSAGE_FILTERS__SYNC_Latest_H_

#include <vector>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include <vector>
#include <mutex>
#include <tuple>

Comment on lines +35 to +36
#ifndef MESSAGE_FILTERS__SYNC_Latest_H_
#define MESSAGE_FILTERS__SYNC_Latest_H_
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#ifndef MESSAGE_FILTERS__SYNC_Latest_H_
#define MESSAGE_FILTERS__SYNC_Latest_H_
#ifndef MESSAGE_FILTERS__SYNC_POLICIES__LATEST_H_
#define MESSAGE_FILTERS__SYNC_POLICIES__LATEST_H_

#ifndef MESSAGE_FILTERS__SYNC_LatestStamped_H_
#define MESSAGE_FILTERS__SYNC_LatestStamped_H_

#include <vector>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include <vector>
#include <mutex>
#include <tuple>

Comment on lines +35 to +36
#ifndef MESSAGE_FILTERS__SYNC_LatestStamped_H_
#define MESSAGE_FILTERS__SYNC_LatestStamped_H_
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#ifndef MESSAGE_FILTERS__SYNC_LatestStamped_H_
#define MESSAGE_FILTERS__SYNC_LatestStamped_H_
#ifndef MESSAGE_FILTERS__SYNC_LATESTSTAMPED_H_
#define MESSAGE_FILTERS__SYNC_LATESTSTAMPED_H_

Comment on lines +112 to +113
bool has_old = (bool)evt_old.getMessage();
if (has_old)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool has_old = (bool)evt_old.getMessage();
if (has_old)
if (evt_old.getMessage() != nullptr)

Comment on lines +117 to +118
if (stamp_new > stamp_old)
evt_old = evt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (stamp_new > stamp_old)
evt_old = evt;
if (stamp_new > stamp_old) {
evt_old = evt;
}

Comment on lines +120 to +121
else
evt_old = evt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
else
evt_old = evt;
else {
evt_old = evt;
}

Comment on lines +206 to +207


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@ZhenshengLee
Copy link

@jcmonteiro Friendly ping for updates.

@ZhenshengLee
Copy link

@jcmonteiro Would you agree that I continue to work with your branch and complete with the PR?

I'll begin with following the change requests in this PR, then create a new PR with the forked branch with my commits.

@jcmonteiro
Copy link
Contributor Author

@jcmonteiro Would you agree that I continue to work with your branch and complete with the PR?

I'll begin with following the change requests in this PR, then create a new PR with the forked branch with my commits.

Hi @ZhenshengLee. I've been out of the loop of the robotics and ROS communities for a while now. Please feel free to take this PR in the direction that you see fit.

@ZhenshengLee
Copy link

Close this and continue with #107

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants