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

Adding arguments to launch files #8

Merged
merged 2 commits into from
Dec 7, 2018
Merged

Adding arguments to launch files #8

merged 2 commits into from
Dec 7, 2018

Conversation

pvechersky
Copy link
Contributor

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.

@bsb808
Copy link
Collaborator

bsb808 commented Jul 18, 2018

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

@pvechersky
Copy link
Contributor Author

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.

@tonybaltovski
Copy link
Member

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.

@pvechersky
Copy link
Contributor Author

@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
  • would add something like
  • remove almost all the parameters except port, baudrate, model number, debug/diagnostics (true/false)
  • then inside <node ... type="microstrain_3dm_gx5_45_node" ...> can add
  1. Add a 'resouce' folder with yaml files for each model, for example ...

  2. '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"

@athackst
Copy link

Hey, I'm also interested in this PR. Can I do anything to help speed it along?

@bsb808
Copy link
Collaborator

bsb808 commented Dec 1, 2018

@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?

@tonybaltovski
Copy link
Member

LGTM 👍

@bsb808 bsb808 merged commit 6b89dc8 into ros-drivers:master Dec 7, 2018
@bsb808
Copy link
Collaborator

bsb808 commented Dec 7, 2018

Thank you all for your patience!

wxmerkt pushed a commit to wxmerkt/microstrain_mips that referenced this pull request May 22, 2019
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.

4 participants