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

geodetic_utils build version #1

Merged
merged 6 commits into from
Oct 15, 2015
Merged

geodetic_utils build version #1

merged 6 commits into from
Oct 15, 2015

Conversation

marija-p
Copy link

  • I've amended CMakeLists.txt and package.xml so that the package can be built by catkin and library used by other packages.
  • I've also made the reference variables (lat, long, height) as public variables as these are sometimes used by other classes (eg. reference altitude is important to determine the starting point of the copter, as the desired height is given relative to this, rather than as an absolute altitude value).
  • Finally, I've changed replaced the name of the manual constructor to a new method, 'initialiseReference', so that the parameters of the class can be set after its declaration.

The conversions were tested on path_publisher and validated with the old conversion functions (now removed) and polycoverage. Please let me know if there are any issues with this, or if the coding can be done more elegantly! :-)

@enricgalceran

@dymczykm
Copy link

@marija-p @enricgalceran do you guys prefer catkin over catkin_simple?


double _init_lat;
double _init_lon;
double _init_h;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the preceding _ here and above and I would prefer initial_x, init can always stand for initialize and initial.

@marija-p
Copy link
Author

@dymczykm Not necesarily, I just used catkin here because I was looking at other mav packages using it as an example. I can change to catkin_simple if preferred.

@marija-p
Copy link
Author

@ffurrer That's all been done now.

@enricgalceran

static const double semiminor_axis = 6356752.3142;
static const double first_eccentricity_squared = 6.69437999014 * 0.001;
static const double second_eccentricity_squared = 6.73949674228 * 0.001;
static const double flattening = 1 / 298.257223563;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch to constexpr (http://stackoverflow.com/questions/13346879/const-vs-constexpr-on-variables).
And if you use google style guide, constants are named as kConstantName (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names), but this is up to you :).

@marija-p
Copy link
Author

@ffurrer Thanks, done!

{
}

void initialiseReference(const double lat, const double lon, const double altitude)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for const prefacing these. It's copied over anyway, doesn't matter if the values are changed inside.

@helenol
Copy link
Collaborator

helenol commented Sep 26, 2015

Except for parameters as pointers for output, LGTM from my side.
--never mind what was here before, just really didn't read closely enough. :P haven't had coffee yet.--

@marija-p
Copy link
Author

@helenol I've made the changes without getting muddled up with the pointers, hopefully. :-) Seems to compile and work fine for me!

enricgalceran added a commit that referenced this pull request Oct 15, 2015
geodetic_utils build version
@enricgalceran enricgalceran merged commit fb8bcef into master Oct 15, 2015
@marija-p
Copy link
Author

I just added a README to master based on the current version of the code. Let me know if I should open a pull request for documentation stuff next time :P @helenol @enricgalceran

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.

5 participants