-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fix LaserScan metadata #17
Conversation
{ | ||
return std::string("start_angle"); | ||
params_->sampling_rate_max = parser_utils::to_long(resp["sampling_rate_max"]); | ||
params_->scan_time_factor = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the value layer_count read from the device (R2300) should be assigned here.
params_->scan_time_factor = parser_utils::to_long(resp["layer_count"]);
If a R2300-4-layer (device_family==5) is connected, then params_->scan_time_factor = num_layers is first set to 4 which comes from config/r2300_params.yaml (call stack: main() ---> pf_interface.init()/PFInterface::init() ---> PFInterface::handle_version()) and later overwritten with params_->scan_time_factor = 1 (call stack: main() ---> pf_interface.init()/PFInterface::init() ---> protocol_interface_->update_scanoutput_config() ---> PFSDP_2300::get_scan_parameters()).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scan_time_factor
depends on whether there are separate LaserScan
topics for each layer (R2300 4-layer). The current driver emits separate topics for R2300 4-layer but not R2300 1-layer, although both report layer_count=4
. It probably should become a user option to choose separate or unified output for R2300 1-layer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 9b97265 the scan_time_factor
setting is done only once near PacketReader instantiation, to avoid later overwrite.
The purpose of num_layers
YAML parameter is unclear to me. Until now it just seems to be expected to replicate the actual number of layers in sensor (which can be read from sensor as layer_count
parameter).
Some metadata in the
LaserScan
messages seems to be inconsistent with themsg/LaserScan
specification or not set at all.These changes try to improve the content of the fields
scan_time
,angle_min
,angle_max
,angle_increment
andtime_increment
. A more detailed description of the changes is included indocs/metadata.md
(which might be a bit too much about internals to be included as is?)Note that especially the changes to the angle information might not match expectations of existing subscriber implementations, while
scan_time
andtime_increment
weren't usable before anyway - as a side effect this probably fixes #4The changes additionally try to avoid deduction of the metadata from static driver configuration. Instead, as much as possible information is gathered from the received packet header.