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.

Sign up for GitHub

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

Jump to bottom

Dashing: Remove redundant storage of configuration data #276

Merged
merged 23 commits into from
Aug 13, 2019
Merged

Dashing: Remove redundant storage of configuration data #276

merged 23 commits into from
Aug 13, 2019

Conversation

clalancette
Copy link
Contributor

This is a somewhat large and subjective PR, but the main goal here is to reduce the amount of duplicate storage of configuration parameters in the classes that make up the velodyne_pointcloud code. The main benefits of doing so are:

  1. Helps with readability
  2. Makes it easier to implement on-the-fly parameter changes (which will come in a follow-on PR)
  3. Reduces memory footprint slightly

There is also a bunch of cleanup of code in this PR, along with the removal of some unused functions. My suggestion for reviewing this is commit-by-commit, since each commit is fairly small and does one logical thing. @JWhitleyAStuff if you'd prefer a couple of smaller PRs, I can do that too; just let me know.

Thanks!

@clalancette
Remove calibrationFile from rawdata config.
33f7822 
It is never used outside of the setup* routines, so don't
bother storing it.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Remove unused setupOffline method.
0a0bc7c 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Remove unused calibration write method.
b6ea914 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Remove unnecessary ros_info parameter from Calibration.
5283d89 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Make the Calibration class only have the RAII-style constructor.
ea416c5 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Remove the calibration read method.
36207e3 
It is now redundant, and we can do everything in the constructor
for an RAII-style class.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Remove unused laser_corrections_maps.
47a8752 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Make RawData::setup not return the number of lasers.
6b6f217 
Instead, get the data directly from a method as necessary.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Make the RawData class RAII-style.
c41ff9f 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Don't store number of lasers in the Transfrom/Convert node.
0815ebf 
The lower layers already have the information.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
container_ptr -> container_ptr_
f5f39da 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Switch container_ptr to a unique_ptr.
abda46a 
There is no reason for it to be a shared pointer.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Remove unused tf_ptr.
20c2443 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Remove tmp_{min,max}_angle from stored RawData config.
ab1172c 
They are both only needed as temporaries.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Switch argument order to always be (min,max)
c06a46e 
At least it is then always consistent.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Make DataContainerBase::configure const reference string parameters.
c116ce1 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Stop storing {min,max}_range in the Node classes.
894867e 
They don't actually need to know the information; their helper
'has-a' classes do, but we can just pass along the information
as we get it.  No need to store it.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Stop storing view_{direction,width} in the Node classes.
fbac72a 
The helper 'has-a' classes need it, but the Node classes
themselves don't need to hold onto them.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Remove organize_cloud, fixed_frame, and target_frame configs.
87a02e2 
The node classes don't need it, so just pass along the values
to the 'has-a' classes that do.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Add explicit, final, and override to classes where appropriate.
84a9045 
This just gives the compiler and reader a few more hints as
to how the classes are used.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Throw an exception for an invalid Velodyne model.
5129633 
It's much better to fail early here and let the user know
they've done something we don't expect.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Make sure to only print the "slower generic method" message once.
a74d3d6 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@IanTheEngineer
Copy link

Looks great @clalancette! @JWhitleyAStuff have you had a chance to take a look at this?

@clalancette
Switch to RCLCPP_WARN_ONCE.
c73461b 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

@JWhitleyAStuff I know we are probably going to start concentrating on the other driver from Autoware, but in the meantime I would still like to get this merged (and potentially released into Dashing). Any thoughts on this?

@JWhitleyWork
Copy link
Contributor

@clalancette I'm sorry I've been slow to respond. I'll review it today.

@JWhitleyWork
Copy link
Contributor

@clalancette - I've looked over everything and it seems pretty logical. It also seems like something we would want to do for the ROS1 Melodic driver. Have you tested this on a device yet?

@clalancette
Copy link
Contributor Author

@clalancette I'm sorry I've been slow to respond. I'll review it today.

No worries! Thanks for looking.

@clalancette - I've looked over everything and it seems pretty logical. It also seems like something we would want to do for the ROS1 Melodic driver. Have you tested this on a device yet?

Yeah, I ran this on the VLP-16 I have (using Dashing). There didn't seem to be much of a difference, which is what I was shooting for :).

I agree that this is something that would be nice to have in Melodic as well. I suspect there will be a bunch of merge conflicts, though, both because of the port and because of some of the subsequent cleanup that I've done on the dashing branch. What do you want to do here?

@JWhitleyWork
Copy link
Contributor

I'll go ahead and approve/merge this for now and we can implement it on Melodic later.

@JWhitleyWork JWhitleyWork merged commit 11d0ea5 into ros-drivers:dashing-devel Aug 13, 2019
@clalancette
Copy link
Contributor Author

@JWhitleyAStuff Thanks for reviewing and merging!

@clalancette clalancette deleted the remove-configs branch August 14, 2019 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IanTheEngineer IanTheEngineer IanTheEngineer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@clalancette @IanTheEngineer @JWhitleyWork