sendPOSI command change (double for lat/lon/h) #111

jnz · 2017-06-26T20:10:08Z

I've noticed the movement with the POSI command can be jumpy. I've looked at the code and the latitude/longitude is transmitted as 32-bit float variables. Which is probably fine for many use cases. But I still suggest a new command POSD to transmit the lat/lon with a 64-bit double precision.

The machine epsilon at 180.0f is: 0.000015f
A 0.000015 degree minimum step in latitude or longitude equals a 1.6 meter step.

    min_step = Earth radius * epsilon * pi/180
    (6371000*0.000015*pi/180)

jason-watkins · 2017-06-26T22:33:09Z

Hi jnz,

Thanks for the pull request! After discussing this issue internally, we've decided that this should really be considered a bug. As such, instead of adding a new command we are going to change the POSI command to use doubles for lat/lon/alt.

If you would like to edit your pull request, I'd still be happy to accept it. On the client side, editing the existing sendPOSI function instead of adding sendPOSD will be sufficient. On the plugin side, we want to be able to continue handling messages from older clients, so I'd like to see the HandlePosi method look something like

if(size == 34)
{
    // Parse old format
}
else if(size == 46)
{
    // Parse new format
}
else
{
    // Log error
}

// Pass data to DataManager

Best,
Jason

jnz · 2017-06-27T09:22:52Z

Hi Jason,

thanks. I'll edit my pull request accordingly.
And thanks for the useful plugin. Btw. it works well with X-Plane 11 as far as I can tell (but I only use pauseSim and sendPOSI).

Cheers
Jan

jnz · 2017-06-27T21:50:42Z

So now I've removed POSD and changed POSI from float to double for lat/lon/h. I've automatically removed trailing whitespaces. This makes the diff a bit harder to read. Sorry for that.

All the Java tests are running fine (tested with X-Plane 11).
I've not added the binaries to the commit.
The C tests are all OK, except for this one:

Running test CTRL (non-player)... GEAR FAILURE... ARE YOU USING AN AIRCRAFT WITH FIXED GEARS?   Test CTRL (non-player) - FAILED
        Error: -31004

The CTRL (player) tests runs fine though. So I'm not sure if this is related to the POSI changes.
The XPlaneConnect.jar that I create with IntelliJ IDEA is not recognized by MATLAB (2017a). I don't know why. So the MATLAB tests are not verified. But I have written a small MATLAB function to send the POSI UDP packets directly and this works fine with X-Plane 11 and the updated plugin.

jason-watkins

Looks good for the most part. I will give it a more in depth review tomorrow. In the mean time, I've highlighted a few minor things I would like to tweaked if you can. If not, I'll accept the PR as-is and make the changes myself.

jason-watkins · 2017-06-27T22:57:48Z

C/src/xplaneConnect.c

@@ -584,23 +585,31 @@ int sendPOSI(XPCSocket sock, float values[], int size, char ac)
 	}

 	// Setup command
-	// 5 byte header + up to 7 values * 5 bytes each


I would just like to take this opportunity to ask my past self why I wrote "5 bytes each" here. I'm fairly certain that it wasn't because I though floats were 5 bytes in size... 🤔

jason-watkins · 2017-06-28T00:12:53Z

C/src/xplaneConnect.c

-		*((float*)(buffer + 6 + i * 4)) = val;
+		if (i < 3) /* lat/lon/h */
+		{
+			*((double*)(buffer + 6 + i * 8)) = val;


Although this follows the current code, I would actually like to see these changed to use memcpy. Fancy pointer casting works fine on x86, but since I originally wrote this code I've learned that this is actually undefined behavior, and it will crash on ARM processors.

jason-watkins · 2017-06-28T00:14:52Z

C/src/xplaneConnect.c

@@ -735,9 +744,9 @@ int sendTEXT(XPCSocket sock, char* msg, int x, int y)
 	size_t len = 14 + msgLen;
 	memcpy(buffer + 5, &x, sizeof(int));
 	memcpy(buffer + 9, &y, sizeof(int));
-	buffer[13] = msgLen;
+	buffer[13] = (unsigned char)(msgLen & 0xff);


Is the & 0xff necessary/useful here? As far as I know, the cast should already be truncating the value of msgLen.

jason-watkins · 2017-06-28T00:18:33Z

xpcPlugin/DataManager.cpp

@@ -668,17 +668,17 @@ namespace XPC
 			return;
 		}

-		if (IsDefault(pos[0]))


Instead of casting these, lets just change the signature of IsDefault. It should still work just fine with floats

jnz · 2017-06-28T18:56:12Z

Ok, all valid points. I've changed the lines. The 5 bytes per item comment is indeed puzzling. Maybe the original message had an additional byte per item to indicate "used/unused" and you've changed it later to the -998 thing to save one byte?

jason-watkins · 2017-06-28T19:03:29Z

Okay, this looks good to me. I'll merge the PR and start working on getting everything tested for a new release.

Thanks again for the contribution!

jnz · 2017-06-28T19:07:28Z

Thanks for accepting the PR.

Due to my limited Python knowledge I've skipped over the Python functions. The Python sendPOSI is still only sending floats for lat/lon/h.

jason-watkins · 2017-06-28T19:08:27Z

Yep, no worries. I did notice and updating it will be part of working on getting this ready for release. :)

* Updated POSI to use doubles for lat/lon/alt, as step size for floats was unacceptably large at high longitudes.

* Minor improvements to the handling of landing gear by the SetGear and HandlePOSI functions * sendPOSI command change (double for lat/lon/h) (#111) * Updated POSI to use doubles for lat/lon/alt, as step size for floats was unacceptably large at high longitudes. * Updated Java library for MATLAB, updated Java project, and updated Windows plugin binaries * Rolled back Java version to 1.7 for MATLAB compatibility * Update MessageHandlers.cpp * Update DataManager.cpp * Added pause functionality for individual a/c Adds new cases such that: 0: Unpauses all a/c 1: Pauses all a/c 2: Switches case for all a/c 100:119: Pauses a/c 0:19 200:219: Unpauses a/c 0:19 Updates log messages. Keeps the 0,1,2 arguments as they previously were. * Finished individual pause functionality * Update DataManager.cpp * Updated flags to allow for individual pause commands * Individual pause command documentation * Updated flags to allow for individual pause commands * Adding individual pause to documentation * Updated flags to allow for individual pause commands * Requested changes, cleaning * Update CMakeLists.txt Include "-fno-stack-protector" for linking and compiling for systems that do not have this have this flag as a default. * Enabling AI after setting position (#118) Previously, the code enabled the AI before setting the position of a/c, which negated its purpose. * Updated XPlane SDK to version 2.1.3 * Resolve function ambiguity for std::abs on mac Fixes #126 * Update copyright notice * Update Windows binaries * Update Linux binaries * Update version numbers * fix osx abs ambiguity (#142) * fix indexing error (#143) * Runway camera location control (#144) * update ignore * basics working * set cam pos remotely * log cam position * keep default behaviour, if short view message is received Compatibility with existing software * all to tabs * rename variable * option to use camera direction fields * Added UDP multicast for plugin discovery (#153) * Added Timer * Added UDPSocket::GetAddr * Added MessageHandlers::SendBeacon() * PoC of starting a timer on PluginEnable * C++11 * Added Timer to xcode project * C++11 in cmake * added Timer to cmake * use function pointer in Timer * moved Timer to namespace + wait for thread to join when stopping the timer * Windows: changed uint to unisgned short * Windows: Added Timer.h/cpp to project * GetAddr static * Send xplane and plugin version with BECN * SendBeacon with params * fixed file copyrights * Include functional to fix Linux compile error * review fixes * Send plugin receive port in BECN * review fixes * Fixed tabs vs spaces indentations (#157) * Java client BECN implementation (#155) * Added MS Azure Dev Ops CI integration (#162) * Do not build for i386 on macOS Use ARCHS_STANDARD to avoid the error “The i386 architecture is deprecated. You should update your ARCHS build setting to remove the i386 architecture.” * Fixed missing include The Visual Studio solution was not compiling * Fixed isnan ambigious refernce Ambigious reference when compiling on travis https://stackoverflow.com/questions/33770374/why-is-isnan-ambiguous-and-how-to-avoid-it * Set MSVC warning level to 3 Too many warnings were generated when building a Release build making Travis job to fail because of too much output * Use default toolset for Visual Studio AppVeyor recommends to set the default toolset * #include <cstdint> * Use MSVC ToolsVersion="14.0" # Conflicts: # xpcPlugin/xpcPlugin/xpcPlugin.vcxproj * Removed binaries from repository # Conflicts: # xpcPlugin/XPlaneConnect/64/win.xpl # xpcPlugin/XPlaneConnect/win.xpl * Added ms azure ci xcode linux linux + macos linux + macos removed branch filter win tests win tests Test all platfroms artifacts tests * output all binaries to ‘XPlaneConnect’ All platforms produce a binary in xpcPlugin/XPlaneConnect/ xpcPlugin/XPlaneConnect/64/ * Added ms azure GH deploy deploy stage GH release test trigger tags * Clean up yml file - Added variables - Added job decriptions * Ignore script to ignore .exp files + fixed output path * Renamed ms azure GH connection * Update service connection for azure pipeline * Added Python3 compatible xpc client. (#156) * Update Azure Pipelines service connection

added sendPOSD command (double for lat/lon)

77f1510

POSD removed, POSI is using double for lat/lon/h

0bb41bb

jnz changed the title ~~added sendPOSD command (double for lat/lon)~~ sendPOSI command change (double for lat/lon/h) Jun 27, 2017

jason-watkins requested changes Jun 28, 2017

View reviewed changes

change requests added

b6abbd4

jason-watkins added the type: feature label Jun 28, 2017

jason-watkins approved these changes Jun 28, 2017

View reviewed changes

jason-watkins merged commit 965aaa4 into nasa:develop Jun 28, 2017

jason-watkins mentioned this pull request Oct 20, 2018

Version 1.3-rc.1 #138

Merged

jason-watkins pushed a commit that referenced this pull request Oct 20, 2018

sendPOSI command change (double for lat/lon/h) (#111)

0e49392

* Updated POSI to use doubles for lat/lon/alt, as step size for floats was unacceptably large at high longitudes.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sendPOSI command change (double for lat/lon/h) #111

sendPOSI command change (double for lat/lon/h) #111

jnz commented Jun 26, 2017

jason-watkins commented Jun 26, 2017

jnz commented Jun 27, 2017

jnz commented Jun 27, 2017

jason-watkins left a comment

jason-watkins Jun 27, 2017

jason-watkins Jun 28, 2017 •

edited

Loading

jason-watkins Jun 28, 2017

jason-watkins Jun 28, 2017

jnz commented Jun 28, 2017

jason-watkins commented Jun 28, 2017

jnz commented Jun 28, 2017

jason-watkins commented Jun 28, 2017

sendPOSI command change (double for lat/lon/h) #111

sendPOSI command change (double for lat/lon/h) #111

Conversation

jnz commented Jun 26, 2017

jason-watkins commented Jun 26, 2017

jnz commented Jun 27, 2017

jnz commented Jun 27, 2017

jason-watkins left a comment

Choose a reason for hiding this comment

jason-watkins Jun 27, 2017

Choose a reason for hiding this comment

jason-watkins Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

jason-watkins Jun 28, 2017

Choose a reason for hiding this comment

jason-watkins Jun 28, 2017

Choose a reason for hiding this comment

jnz commented Jun 28, 2017

jason-watkins commented Jun 28, 2017

jnz commented Jun 28, 2017

jason-watkins commented Jun 28, 2017

jason-watkins Jun 28, 2017 •

edited

Loading