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

Hotfix: cannot receive scan topic with best-effort QoS. #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skylerpan
Copy link

Original implementation uses message_filters to subscribe to the scan topic.
But message_filters default only passes topic with reliable QoS.
I modified the QoS setting in message_filters to pass both reliable and best-effort topics.
And I have merged some modifications from @bergercookie's fork.

Copy link
Member

@shiveshkhaitan shiveshkhaitan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @skylerpan. I have left a few comments. Please look into them at your convenience

@@ -501,6 +501,6 @@ void RecordList::printPath(ostream& os, unsigned int i, bool err, bool rawodom)
cout << "average error" << totalError/count << endl;
}

}; //gfsreader
Copy link
Member

Choose a reason for hiding this comment

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

Since openslam_gmapping has been directly adopted, we can keep the original formatting of the code. This also applies for all the other openslam_gmapping files that have been changed

Copy link
Author

@skylerpan skylerpan Feb 11, 2020

Choose a reason for hiding this comment

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

Hi @shiveshkhaitan. I agree. I have reverted this to adopt the original code.

@@ -100,16 +107,18 @@ class SlamGmapping : public rclcpp::Node{
std::shared_ptr<std::thread> transform_thread_;

std::string base_frame_;
std::string laser_frame_;
Copy link
Member

Choose a reason for hiding this comment

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

This line has been moved below. The change does not make any difference. We can keep the original declaration

Copy link
Author

@skylerpan skylerpan Feb 11, 2020

Choose a reason for hiding this comment

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

Hi @shiveshkhaitan.
I have reverted this to the original code.

Comment on lines 6 to 23
param_substitutions = {
'base_frame': 'base_link',
'odom_frame': 'odom',
'map_frame': 'map',
'map_update_interval': '3.0',
'maxUrange': '10.0',
'sigma': '0.05',
'kernelSize': '1',
'lstep': '0.05',
'astep': '0.05',
'iterations': '5',
'lsigma': '0.075',
'ogain': '3.0',
'lskip': '0',
'srr': '0.1',
'srt': '0.2',
'str': '0.1',
'stt': '0.2',
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the addition of parameters. But this can be added as a separate PR since it is unrelated

Copy link
Author

Choose a reason for hiding this comment

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

Hi @shiveshkhaitan.
I have removed these parameters in the launch file.

Comment on lines 29 to 34
#define LOAD_PARAMS(_name_,_val_) \
this->declare_parameter((#_name_),(_val_),\
rcl_interfaces::msg::ParameterDescriptor());\
this->get_parameter((#_name_), (_name_##_));\
std::cout<<(#_name_)<<" "<<(_name_##_)<<std::endl

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the modifications @skylerpan. Some of the parameter related changes are still not reverted. Can you please revert them as well. Please feel free to have them as a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @shiveshkhaitan. I reverted all the code excepting two bug fixes, and also rebased the commits. Many thanks.

nikoskoukis-slamcore and others added 2 commits March 2, 2020 00:33
This implementation uses message filter to sub topic.
But message filter default only passes topic with
reliable qos. I modified the qos setting in message
filter to pass both reliable and best-effort topic.
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