Suggestions for improvement

[AlexanderFabisch]

I read the code of this package recently. There are some things that could be improved.

Memory leak: UTMConverter::coTransform is a pointer to an object that is allocated on the heap when you create an object of UTMConverter and there is no destructor that deletes it.

drivers-gps_base/src/UTMConverter.cpp

Lines 8 to 15 in 8483210

	UTMConverter::UTMConverter()
	: utm_zone(32)
	, utm_north(true)
	, origin(base::Position::Zero())
	, coTransform(NULL)
	{
	createCoTransform();
	}

Magic number: The number 1000000 is used in the conversion from UTM coordinates to NWU - why, what does it mean, and is it always 1000000 or is this an approximation that works only in some countries?

drivers-gps_base/src/UTMConverter.cpp

Lines 102 to 104 in 8483210

	position.position.x() = northing;
	position.position.y() = 1000000 - easting;
	position.position -= origin;

Unexpected dynamic memory allocation: For a robotic middleware that can be used in a real-time system it is not a good idea to hide dynamic memory allocations in member functions without any hint in the documentation. void UTMConverter::setUTMZone(int zone) and void UTMConverter::setUTMNorth(bool north) both call this function:

drivers-gps_base/src/UTMConverter.cpp

Lines 17 to 27 in 8483210

	void UTMConverter::createCoTransform()
	{
	OGRSpatialReference oSourceSRS;
	OGRSpatialReference oTargetSRS;
	oSourceSRS.SetWellKnownGeogCS("WGS84");
	oTargetSRS.SetWellKnownGeogCS("WGS84");
	oTargetSRS.SetUTM(this->utm_zone, this->utm_north);
	OGRCoordinateTransformation* newTransform =
	OGRCreateCoordinateTransformation(&oSourceSRS, &oTargetSRS);

Doing this in the constructor only would be OK because RAII.

Class design / default values: Why is it even necessary to change the UTM zone or the hemisphere at runtime? Shouldn't it be sufficient to give it to the constructor? Do we expect to change UTM zones or hemispheres frequently? Also the default UTM zone is 32 and the default hemisphere is northern. I see why this is the case. The code has been developed and tested in Germany. The problem is when I am not in Germany, I always have to construct a transformation that I won't ever use before I create a new transformation with two different setter functions in the worst case. My suggestion would be to create a constructor to set UTM zone and hemisphere once in the beginning and remove all setters and getters. When we need a new transformation, we can create a new object of UTMConverter.

Naming: The UTMConverter is called UTMConverter because it converts to the Universal Transverse Mercator (UTM) coordinate system, but it also converts from geodetic coordinates and to north west up. The name of the class does not tell me what it does and it is also not documented anywhere. I don't even know what UTM is when I only see the code.

Frame convention (actually a question): Why is the convention north west up used? From what I read (without having so much experience) it is a quite unusual coordinate system. More commonly used systems are east north up (ENU) and north east down (NED). When I search for "north west up", the first result on Google is this: https://n-w-up.com/

Standard deviations (actually a question): The covariance of the position in the RigidBodyState is computed here:

drivers-gps_base/src/UTMConverter.cpp

Lines 86 to 88 in 8483210

	position.cov_position(0, 0) = solution.deviationLongitude * solution.deviationLongitude;
	position.cov_position(1, 1) = solution.deviationLatitude * solution.deviationLatitude;
	position.cov_position(2, 2) = solution.deviationAltitude * solution.deviationAltitude;

Is this really valid? Shouldn't these be a standard deviations in degrees? Why do we have to apply a complex transformation to get the correct position but not for its standard deviations? I can't really find any documentation about this.

[doudou]

Hi Alexander. Thanks for the comments.

The whole GPS stuff needs a good cleaning up indeed.

Memory leak
Magic number
Class design / default values:

👍

Doing this in the constructor only would be OK because RAII.

I get your point, but RAII is pretty irrelevant. If you want to annotate what might allocate (a good goal) then constructors should be annotated as well.

I don't even know what UTM is when I only see the code.

Class documentation would be useful. In fine, however, if you are dealing with GPSes in a professional setting and don't know what UTM is, you have bigger troubles than understanding what this class does.

Frame convention (actually a question):

NWU is Rock's georeferenced coordinate frame.

Standard deviations (actually a question):

GPSes "latitude/longitude deviations" reported by GPSes are actually deviations in meters along the latitude and longitude axes.

Northing/Easting (from UTM) to NWU require to swap X/Y, apply an offset (the center of a UTM grid is the magic 1000000) and change directions to match the right-hand rule. There's no such need for deviations, only to swap X and Y (which the conversion does).