Fix: RMC output fails where degrees longitude > 99

[ph1l]

this commit replaced toNmeaDegrees with two separate functions, toNmeaDegreesLatitude and toNmeaDegreesLongitude. NMEA sentences expect slightly different formatting since longitude can go up to 180 and thus requires 3 places in the output.

Includes a test fot the original bug and for various usages and error states of new functions.

fixes #18

[tkurki]

How would you feel about adding tests for GLL and RMB that also use nmea.toNmeaDegrees?

[ph1l]

Since I switched to a different source of GPS data (NMEA2K via AIS Transceiver,) the TCP Stream of NMEA0183 data on port 10110 no longer contained position data. I enabled signalk-to-nmea0183 RMC output for OpenCPN, and my boat was in the Atlantic instead of the San Francisco Bay!

I only was attempting to fix the bug I found. I don't feel like I know enough about the other components of those sentences to write tests for them.

I have, however, re-written a more correct fix, that has a full set of tests for the helper function and will push it shortly

[tkurki]

Just fixing this is plenty, no worries.

So you should close this PR and open a new one? Or you can just force push over this one, just so we don't merge this if there's better stuff in the pipeline.

[ph1l]

already force pushed, see f4f6b5a ?

[ph1l]

PI? Oh I see you speaking in radians.. you mean longitudes in -pi to pi and latitudes from - pi/2 tp pi/2?

[tkurki]

In radians in sk data. Pi as in Math.PI.

[ph1l]

Is 961863f what you're getting at?

[tkurki]

Yes, except isn't the input from SK land and in radians => the thresholds should be ±Math.PI and ±Math.PI/2.

For longitude it would be possible to convert it to fit the range, but I guess throwing is ok here.

And a unit test for actual conversion handling invalid input would be in order.

[ph1l]

nope. signalk appears to track position as a json object of decimal degrees:

    "position": {
      "meta": {
        "description": "The position of the vessel in 2 or 3 dimensions (WGS84 datum)"
      },
      "value": {
        "longitude": 23.399533333333334,
        "latitude": 59.96445
      },
      "$source": "nmeaFromFile.GP",
      "timestamp": "2018-05-02T12:38:39.000Z",
      "sentence": "GLL"
    }

[ph1l]

For longitude it would be possible to convert it to fit the range, but I guess throwing is ok here.

And a unit test for actual conversion handling invalid input would be in order.

Convert Longitude when a non-existent value is specified? like +182 instead of -178 degrees? Not sure I agree this should be supported...

[tkurki]

Lol. For some reason i was thinking of headings. Silly me.

[ph1l]

I figured :-)

Well, This should be good to go!

[tkurki]

• squashed commits in #19 to #20
• added error handling so that the thrown errors are caught and logged but otherwise ignore, just no output generated
• added a test to exercise the error handling logic

@ph1l could you take a quick look that I did not misplace anything?

[ph1l]

It is your project and I don't really have the right to tell you how to manage it, however, I would prefer to have my commits merged as authored rather than squashed.

If you preferred that my PR be a single commit, I can force push and update my branch.

If you want to add something to it before it makes it to master, I'd prefer you just branched from my changes and put commits on top.

Either way, It looks like you got everything, and the tests pass over here..

[tkurki]

My sincere apologies.

I really appreciate your work. Instead of adding more comments to this PR where I already made a complete fool of myself I thought I'd help out by working out the final details and not bother you.

I wasn't sure how the thrown errors should be handled, so I created one more test. Before that I refactored the test, as my third test would have been more copypaste, and then figured out how to log & ignore the errors.

I did not know beforehand how this should be done, so I could not give you advice on how to do it without figuring that out myself. Once I had done the work it seemed it formed a coherent whole that should be one commit.

For my defence: I suggested earlier that there should be a test for a failing conversion, which you did not pick up. That would have probably lead to working out the details something similar as in #20.

[ph1l]

Hey man, I'm not offended! :-)

I'm actually new to javascript still, so I appreciate the code review process and the back and forth!

The streams/Bacon stuff and setting up the tests for the actual sentence code were a bit confusing, so I didn't quite know how to do that right. Your changes to test/RMC.js look more sane ;-)

[ph1l]

btw, I am okay with your changes on top of what I have here already. I was just asking if you minded adding a separate commit on top of this branch before merging..

I'm not super sure about how thowing and catching exceptions works ;-)

[tkurki]

Merged in #20 - thanks!