search

ros-drivers / microstrain_mips

Other
26 stars 111 forks source link

Cleanup #21

Closed samkys closed 5 years ago

samkys commented 5 years ago

This PR is a cleanup of the microstrain_3dm file. It had various tabs and indentation levels making it very hard to read and follow. Also curly brace location was corrected according to: http://wiki.ros.org/CppStyleGuide section 6.

Tabs were replaced with spaces. Everything compiles and it was tested with the -25 model and it works with that model.

bsb808 commented 5 years ago

A quick look at the source looks encouraging. Thank you for taking the time to add the functionality and help with the maintenance!

I'll try to find some time to test the driver with a -45 model later this week.

samkys commented 5 years ago

I was trying to separate out the two pull requests, the cleanup and the parameterization but did them in backwards order. I will close #20 since it is included in this PR anyhow.

samkys commented 5 years ago

I just came across an interesting case that I cannot test with the -25. Can someone who has a -45 or -35 please test this prior to merging this PR.

1) Set: publish_imu = false 2) Set: publish_odom = true

Does your nav message contain orientation information? I think there might be a setting that we are required to have both either to true or fix some things in the driver. (ie set some AHRS settings are required for the filtered data that is being received from the filtered 0x82 packet data set.)

tonybaltovski commented 5 years ago

@samkys would you mind adding roslint test for the formatting? Otherwise, the co-variance change looks good.

samkys commented 5 years ago

@tonybaltovski Do you want me to leave roslint in the CMAKE and in the package.xml? Do you want me to clean all of the lines of code that roslint says need cleaned up? (Some of the early ones I currently see are things like: #ifndef header guard has wrong style, please use: .... and Should have a space between // and comment

(Just wanted to check prior to spending a lot of time cleaning everything up and find out you had something else in mind.)

bsb808 commented 5 years ago

I ran the test @samkys asked for with a -45 on my desk.

The Odometry messages on nav/odom included orientation information, which is what we would want.

Good to go on this end.

tonybaltovski commented 5 years ago

@samkys that would be ideal to clean-up the package. I can if you just want to add the roslint package.

samkys commented 5 years ago

@tonybaltovski I added roslint and ran it. I cleaned up as much as I could but there were a few lines that I'm not sure what to do with. A lot of the errors it was throwing were too long of lines so I wrapped them around. Take a look and see what you think.

tonybaltovski commented 5 years ago

Awesome, thanks for that!