Working and calculating with geographic positions

Question

I was thinking about how to implement a GPS navigation for my quadcopter. I was writing some functions to calculate the angle between two positions (from home point to the target).

I'd like a review of this code. I am not very sure whether I was calculating the geographic position right.

/*
 * Geographic calculations
 */
float UAVNav::width_of_merid_m(const float fLat_deg) { 
 float fdX_m = _2PI * _RADIUS_EARTH_m * cos(ToRad(fLat_deg) );
 fdX_m /= 360.f;
 return fdX_m;
}
float UAVNav::dist_2_equat_m(const float fLat_deg) {
 return (_PERIMETER_EARTH_m / 360.f) * fLat_deg;
}
float UAVNav::dist_2_greenw_m(const float fLat_deg, const float fLon_deg) { 
 return width_of_merid_m(fLat_deg) * fLon_deg;
}
float UAVNav::home_2_target_deg() { 
 const float fMod = 10000000.f;
 GPSPosition target = m_pReceiver->m_Waypoint;
 GPSData home = m_pHalBoard->get_gps();
 float fLatHome_deg = home.latitude / fMod;
 float fLonHome_deg = home.longitude / fMod;
 float fLatTarg_deg = target.latitude / fMod;
 float fLonTarg_deg = target.longitude / fMod;
 float fXHome = dist_2_greenw_m(fLatHome_deg, fLonHome_deg);
 float fYHome = dist_2_equat_m(fLatHome_deg);
 float fXTarg = dist_2_greenw_m(fLatTarg_deg, fLonTarg_deg);
 float fYTarg = dist_2_equat_m(fLatTarg_deg);
 float dX = fXTarg - fXHome;
 float dY = fYTarg - fYHome;
 // Calculate the angle to the destination direction
 // NOTE: Here the system is inverted by 90° to align the magnetic north to 0°
 m_fDestin_deg = ToDeg(atan2((float)dX, (float)dY) );
 return m_fDestin_deg;
}

Answer 1

There are some temporary variables that you might not use, such as fLatTarg_deg and fLotTarg_deg: you can commit the operation along with the function call.

Also, the variables (which are actually costants, probably) _RADIUS_EARTH_m and _PERIMETER_EARTH_m are breaking the standard C++ naming conventions.

• edmz: @syb0rg Hope you won't mind, but did you seriously edit my answer to add a new line?
• SylvainD: It seems like he did. Isn't it better that way ?
• syb0rg: @no1 I was out of votes for the day, and I had used one of my last votes on your answer. Then I bought the unicorn animation, and I may have wanted to test it out. ;)
• Vogel612: @syb0rg shame on you, if you spend 40 votes, why not buy the unicorn animation before doing so ??

Answer 2

You naming convention is a bit weird. My assumption is that fLat stands for "float latitude". Because it is quite easy to know the type of a variable, there's no need for having the f prefix.

You don't need to cast to float in m_fDestin_deg = ToDeg(atan2((float)dX, (float)dY) );.

You don't need the temporary variable in UAVNav::width_of_merid_m especially as it makes the mathematical formula harder to read.

Your method UAVNav::home_2_target_deg() seems to be doing two things : returning a value and updating a member. It would be clearer to have a (const) method returning a value and another method using it to set a member.

Answer 3

• You don't have std:: in front of cos() and atan2() (which belong to <cmath>), which means you're using using namespace std. This is discouraged as it could cause bugs in cases of name-clashing with existing names used in this namespace (or any namespace, although std is most commonly used). See this for more information.

• I agree with @Josay about the casting being unnecessary in atan2(). Its two arguments are already float, so casting will do nothing.

  Moreover, when you do need casts like these, prefer static_cast<>() in C++:

  static_cast<type>(object);
  

  More info about different types of casts here.

• dgrat: I use math.h for reason and don't use: "using namespace whatever" in the whole program.
• Jamal: @dgrat: Okay. Without showing the #includes, I would not have been able to tell.
• dgrat: Well, I switched to static casts. The main reason why I was not using them is, that they look ugly, make code longer and contain even more brackets.
• Jamal: @dgrat: I do agree that they're quite a bit longer. It may not matter much for smaller programs, though.