1
\$\begingroup\$

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;
}
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ @syb0rg Hope you won't mind, but did you seriously edit my answer to add a new line? \$\endgroup\$ – edmz Apr 1 '14 at 16:33
  • 2
    \$\begingroup\$ It seems like he did. Isn't it better that way ? \$\endgroup\$ – SylvainD Apr 1 '14 at 16:50
  • \$\begingroup\$ @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. ;) \$\endgroup\$ – syb0rg Apr 1 '14 at 18:00
  • 1
    \$\begingroup\$ @syb0rg shame on you, if you spend 40 votes, why not buy the unicorn animation before doing so ?? \$\endgroup\$ – Vogel612 Apr 1 '14 at 18:42
2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$
  • 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.

\$\endgroup\$
  • \$\begingroup\$ I use math.h for reason and don't use: "using namespace whatever" in the whole program. \$\endgroup\$ – dgrat Apr 9 '14 at 10:29
  • \$\begingroup\$ @dgrat: Okay. Without showing the #includes, I would not have been able to tell. \$\endgroup\$ – Jamal Apr 9 '14 at 14:18
  • \$\begingroup\$ 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. \$\endgroup\$ – dgrat Apr 9 '14 at 15:03
  • \$\begingroup\$ @dgrat: I do agree that they're quite a bit longer. It may not matter much for smaller programs, though. \$\endgroup\$ – Jamal Apr 9 '14 at 16:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.