Closed pvechersky closed 5 years ago
I think this is a good improvement.
I might suggesting the following - if you think it will help.
1) rename the microstrain.launch file to microstrain_45.launch. Then we'll have an example for the -45 model and one of the -25 in the repo. 2) Remove the microstrain_kf.launch and microstrain_pioneer.launch files. These were specific instances for our internal use (for Kingfisher and Pioneer robots respectively). Now that the driver is becoming more general purpose, I don't think they belong in the general repo.
Thoughts?
Brian
Thank you for your feedback, Brian!
Regarding (1)
If I understand correctly the only major difference between 45 and 25 that would be baked into the launch files would be 25 not using the GPS and Odometry, right? ... With that being said, for any potential users of this I think it is handy to see a launch file that matches your specific model and know that all you have to do is set the port name to be off and running.
It is also worth mentioning that I am actually working together with @ryanmeasel, with whom you had a discussion in the past regarding the GX5-10 model support #5. Bringing GX5-10 into the mix is still something we want to do next. Under the proposed launch file scheme that means also having a microstrain_10.launch. Would that be OK? Would it be different? As far as I can tell based on what we use now, the microstrain_25.launch file also works for 10, but keeping such nomenclature is clearly less than ideal.
Regarding (2)
Removing the kf and pioneer launch file would definitely be good. I will go ahead and include that change in this pull request already.
Just as a thought, we could make a generic launch file that loads a config yaml based on which model of IMU with the parameters/features of the specific model.
@tonybaltovski @bsb808 Hi guys, sorry for such delayed response. Finally getting back into the swing of things but still very much keen on doing some launch file cleanup.
@tonybaltovski I think having a generic launch file is a good idea. So if I understand you correctly, some of the changes would be:
1) microstrain.launch
2) Add a 'resouce' folder with yaml files for each model, for example ...
3) 'microstrain_25.yaml' file would look something like this:
device_setup: true
readback_settings: true save_settings: true auto_init: true
publish_imu: true imu_rate: 100 imu_frame_id: "imu_link" declination_source: 2 declination: 0.23
gps_rate: 4
gps_frame_id: "navsat_link"
publish_gps: false
publish_odom: false nav_rate: 10 dynamics_mode: 1 odom_frame_id: "wgs84_odom_link" odom_child_frame_id: "base_link"
Hey, I'm also interested in this PR. Can I do anything to help speed it along?
@athackst Thank you for the nudge to get this moving again.
Looking through the thread it seems like there are two proposals here.
1) Using device-specific launch files (e.g., microstrain_45.launch) and exposing a subset of the parameters as command line arguments.
2) Doing some additional refactoring so that there is a single launch file (microstrain.launch) that then uses device-specific yaml config files.
My personal preference is for 1). As said previously, I think it is a simpler solution for folks to get started with specific hardware variants. To my thinking the launch files in the package are meant to be working examples that can be used for folks to get up and running quickly.
I believe that the current PR achieves 1) - but that it would be an small amount of additional work to implement 2)
So - I'd propose we move forward with merging the PR as is. Sound good?
LGTM :+1:
Thank you all for your patience!
Exposing some of the node parameters as arguments in the launch files to make it easier to use existing launch files with different configurations, especially the port names.
The 'microstrain.launch' file is the most generic one with many parameters exposed. The other launch files are meant to be more specific so I kept them largely the same, with just a few arguments. I think ideally what would happen is that the more specific launch files would call 'microstrain.launch' internally with fewer arguments exposes (for example, 'publish_gps' + 'publish_odom' would be hard-coded to FALSE for microstrain_25.launch) but at the moment I don't know enough about the model specifics to safely generalize them.