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

Set state fix #785

Open
wants to merge 6 commits into
base: galactic-devel
Choose a base branch
from
Open

Conversation

Max191
Copy link

@Max191 Max191 commented Dec 17, 2022

No description provided.

geometry_msgs/PoseWithCovarianceStamped pose
geometry_msgs/TwistWithCovarianceStamped twist
geometry_msgs/AccelWithCovarianceStamped accel
bool[15] update_vector
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing newline.

@@ -51,7 +51,7 @@
#include <utility>
#include <memory>
#include <vector>

#include <Eigen/Dense>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed line break here. Please replace.

@@ -1035,13 +1036,26 @@ void RosFilter<T>::loadParams()
"set_pose", rclcpp::QoS(1),
std::bind(&RosFilter<T>::setPoseCallback, this, std::placeholders::_1));

// Create a subscriber for manually setting/resetting state
set_state_sub_ =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to support this via messages? This feels like a service call should suffice.

@@ -0,0 +1,3 @@
State state
---
bool success
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing newline.

@@ -2243,6 +2257,148 @@ bool RosFilter<T>::setPoseSrvCallback(
return true;
}

template<typename T>
void RosFilter<T>::setStateCallback(const robot_localization::msg::State::SharedPtr msg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this duplicates a lot of code. Why can't we just call reset() and then set the state? As much as I appreciate the input sanitising, I think the user needs to be ensuring that they are sending valid states to the filter. This class is already too bloated, and this method adds a lot of new stuff.

all_real_and_nonnegative = all_real_and_nonnegative
&& eigval.real()>0.0 && std::abs(eigval.imag())<1E-6;
}
if(all_real_and_nonnegative){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing space after if.

all_real_and_nonnegative = all_real_and_nonnegative
&& eigval.real()>0.0 && std::abs(eigval.imag())<1E-6;
}
if(all_real_and_nonnegative){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing space after if.

}else{
RCLCPP_INFO(get_logger(), "Not setting acceleration because acceleration covariance is not positive definite");
}
}else{
Copy link
Collaborator

Choose a reason for hiding this comment

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

All these else cases should have spaces between the braces and the else.

RCLCPP_INFO(get_logger(), "Setting acceleration");
std::shared_ptr<sensor_msgs::msg::Imu> imu_ptr(new sensor_msgs::msg::Imu);
std::vector<bool> update_vector_imu(msg->update_vector.begin(), msg->update_vector.end());
update_vector_imu[StateMemberVroll]=update_vector_imu[StateMemberVpitch]=update_vector_imu[StateMemberVyaw]=
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an ugly line. Can we clean this up?

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.

3 participants