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

Copy of Making repeated state publication optional #595 #42

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

servos
Copy link

@servos servos commented Feb 16, 2021

Since we have diverged from upstream rather significantly at this point I was not able to simply cherry pick this change without major conflicts, so I just copied it.

Original change: cra-ros-pkg#595

We should properly sync our fork with upstream at some point.

@servos servos requested review from efernandez, a user and nlunscher-cpr February 16, 2021 20:15
@servos servos self-assigned this Feb 16, 2021
Copy link
Member

@efernandez efernandez left a comment

Choose a reason for hiding this comment

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

I reviewed this while looking at the changes in https://github.com/cra-ros-pkg/robot_localization/pull/595/files and apart from the permit_corrected_publication ROS parameter and permitCorrectedPublication_ attribute, that you seem to have dropped from this PR, all looks fine and it's the same.

Maybe you could have cherry-picked the upstream commit, but I guess you already tried and that didn't work, so this is fine.

@@ -648,6 +648,10 @@ template<class T> class RosFilter
//!
geometry_msgs::TransformStamped worldBaseLinkTransMsg_;

//! @brief The time of the most recent published state
Copy link
Member

Choose a reason for hiding this comment

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

In https://github.com/cra-ros-pkg/robot_localization/pull/595/files the permit_corrected_publication ROS parameter and permitCorrectedPublication_ variables were created. Did you miss that part, or you simply don't want that part?

Copy link
Author

Choose a reason for hiding this comment

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

I just left it out since I don't think we would ever use those params and I was trying to keep it as minimal as possible.

I think we need to sync fully with upstream at some point so we can reintegrate properly then.

@servos
Copy link
Author

servos commented Feb 17, 2021

I reviewed this while looking at the changes in https://github.com/cra-ros-pkg/robot_localization/pull/595/files and apart from the permit_corrected_publication ROS parameter and permitCorrectedPublication_ attribute, that you seem to have dropped from this PR, all looks fine and it's the same.

Maybe you could have cherry-picked the upstream commit, but I guess you already tried and that didn't work, so this is fine.

Yes I tried to cherry pick it and it was massive conflicts not sure why but I didn't want to deal with it.

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