Prevent AP $ECRMB from being more than 82 characters

[Hakansv]

A suggestion to solve this CF report
If RMC >80 chars we truncate WP's name further one step until <=80 chars.
This truncation will also affect APB but the philosophy is to transfer the same WP name by all sentences.

No need to check the size for other sentences. They are not close to the 80 chars limit.
Tested here:

14:17:37.310 MESSAGE routeman.cpp:555 AP-$ECRMB size is more than 80 char.: maxWPNameLength decreased to 5
14:17:37.310 MESSAGE routeman.cpp:561 RMB size 82 chars
14:17:37.310 MESSAGE routeman.cpp:622 RMC size 74 chars
14:17:37.310 MESSAGE routeman.cpp:697 APB size 70 chars
14:17:37.310 MESSAGE routeman.cpp:728 XTE size 25 chars

14:17:38.323 MESSAGE routeman.cpp:561 RMB size 80 chars
14:17:38.323 MESSAGE routeman.cpp:622 RMC size 74 chars
14:17:38.323 MESSAGE routeman.cpp:697 APB size 69 chars
14:17:38.323 MESSAGE routeman.cpp:728 XTE size 25 chars

[Hakansv]

NMEA0183 standard says max length is 82 including '$' and "LF" not 80. Corrected.

[leamas]

IMHO, we should add a unit test here. It's easy to test, and having it in the test suite handles regressions. Might be able to do this later.

[leamas]

hm... can't sleep, digging a little.

The need to truncate the waypoint name is obvious for RMB messages when they become too long. However, is decreasing g_maxWPNameLength the right to handle it?

From a formal POV it seems questionable to change an overall configuration value like this.

From a more practical standpoint won't this mean that all message waypoints, not just RMB, will be truncated? Is this what we want?

FWIW, my gut feeling is that the instead of updating g_maxWPNameLength too long RMB messages should be fixed before they are sent around line 532 in routemanager.cpp.

Thoughts?

[Hakansv]

Yes, my first action was to only change truncation for RMB sent to the AP.
The reason to instead change a common parameter was to not confuse a receiver by different names for the same object.
" g_maxWPNameLength" is used to create (EC)RMB and (EC)APB both sent to the AP. It's also used when sending RTE with the function "Send to GPS". For this O session we may assume both devices are on the same ship. (The new value of g_maxWPNameLength is not saved to config.)

But it could be I was "too smart" and over-reacting? To change only (EC)RMB when transferring it to an AP is an easy fix. Thoughts?

[leamas]

Seems like none of us are really sure here. In that situation I think it might be better to just fix RMB messages for two reasons.

The first is that I'm pretty sensitive about modifying configuration values like this. Such values are designed to be maintained by the user only. Modifying them in runtime like this adds some hard to understand and maintain code paths.

The other reason is that in a situation where we don't really understand the consequences it's probably better to change as little as possible.

Might add that I don't really see anything bad coming from having different waypoint names in RMB compared to other messages. But that's just me.

Did a quick implementation hack before I wrote this. Let me know if you want me to share it.

[leamas]

One more thing: Auto-generated waypoints typically has names like W00001, W00002, etc. truncating these is then certain to create duplicate names.

Would it be better to strip the prefix in all or some cases, creating names like "0001", "0002" instead?

[Hakansv]

When OCPN make routes the WP's names are like 001, 002, 003. Your W00001 may be a MBS example?
I now agree to change RMB only. If you have an efficient patch to this I'm so happy and will instantly scrap my PR. ("Efficient" - not a big function every second)

[leamas]

Here, untested:

 // RMB
  {
    int wp_len = maxName;
    SENTENCE snt;
    do {
      snt = SENTENCE();
      m_NMEA0183.TalkerID = "EC";
      m_NMEA0183.Rmb.IsDataValid = bGPSValid ? NTrue : NFalse;
      m_NMEA0183.Rmb.FAAModeIndicator = bGPSValid ? "A" : "N";
      m_NMEA0183.Rmb.To = pActivePoint->GetName().Truncate(wp_len);
      m_NMEA0183.Rmb.From =
          pActiveRouteSegmentBeginPoint->GetName().Truncate(wp_len);
      m_NMEA0183.Rmb.RangeToDestinationNauticalMiles = CurrentRngToActivePoint;
      m_NMEA0183.Rmb.BearingToDestinationDegreesTrue = CurrentBrgToActivePoint;
      m_NMEA0183.Rmb.DestinationClosingVelocityKnots =
          r_Sog * cos((r_Cog - CurrentBrgToActivePoint) * PI / 180.0);
      m_NMEA0183.Rmb.CrossTrackError = CurrentXTEToActivePoint;
      m_NMEA0183.Rmb.DirectionToSteer = XTEDir < 0 ? Left : Right;
      m_NMEA0183.Rmb.IsArrivalCircleEntered = m_bArrival ? NTrue : NFalse;

      m_NMEA0183.Rmb.Write(snt);
      wp_len -= 1;
    } while (snt.Sentence.size() > 82 && wp_len > 0);
    // What do we do if message still is too long here?
    BroadcastNMEA0183Message(snt.Sentence, m_nmea_log, on_message_sent);
  }

[Hakansv]

"What do we do if message still is too long here?" - Unlikely but this code will then make an endless loop I think. - Risky?
Instead of a do-while we can just adjust a static wp_len (>0) for next occasion. We can allow one or two oversized RMB's. I think the AP can stand a couple of seconds without RMB.

[leamas]

Not an endless loop, wp_len will always terminate it. But risk for an oversized message anyway. Should probably at least be logged.

[leamas]

Could be optimized somewhat, but I doubt it is necessary.

[leamas]

Somewhat faster and easier to read:

 // RMB
  {
    SENTENCE snt;
    m_NMEA0183.TalkerID = "EC";
    m_NMEA0183.Rmb.IsDataValid = bGPSValid ? NTrue : NFalse;
    m_NMEA0183.Rmb.FAAModeIndicator = bGPSValid ? "A" : "N";
    m_NMEA0183.Rmb.RangeToDestinationNauticalMiles = CurrentRngToActivePoint;
    m_NMEA0183.Rmb.BearingToDestinationDegreesTrue = CurrentBrgToActivePoint;
    m_NMEA0183.Rmb.DestinationClosingVelocityKnots =
        r_Sog * cos((r_Cog - CurrentBrgToActivePoint) * PI / 180.0);
    m_NMEA0183.Rmb.CrossTrackError = CurrentXTEToActivePoint;
    m_NMEA0183.Rmb.DirectionToSteer = XTEDir < 0 ? Left : Right;
    m_NMEA0183.Rmb.IsArrivalCircleEntered = m_bArrival ? NTrue : NFalse;
    int wp_len = maxName;
    do {
      snt = SENTENCE();
      m_NMEA0183.Rmb.To = pActivePoint->GetName().Truncate(wp_len);
      m_NMEA0183.Rmb.From =
          pActiveRouteSegmentBeginPoint->GetName().Truncate(wp_len);
      m_NMEA0183.Rmb.Write(snt);
      wp_len -= 1;
    } while (snt.Sentence.size() > 82 && wp_len > 0);
    // What do we do if message still is too long here?
    BroadcastNMEA0183Message(snt.Sentence, m_nmea_log, on_message_sent);
  }

[Hakansv]

Thanks, I'll will adapt your suggestions.
(Been out with the boat all day. Now moored.)

[leamas]

Please don't take my untested code for granted. I discovered a bug, now fixed, but there could certainly be more. The overall design is however ok IMHO

[Hakansv]

I don't see what you have changed?
But I've added lat/lon and tested.

Edit: I see. you added snt = SENTENCE(); but for any reason it's not needed. Wasn't there before.

[Hakansv]

Adapted and rebased

[leamas]

hm... I see this:

    if (pActivePoint->m_lat < 0.)
      m_NMEA0183.Rmb.DestinationPosition.Latitude.Set(
        pActivePoint->m_lat, _T("S"));
    else
      m_NMEA0183.Rmb.DestinationPosition.Latitude.Set(
        pActivePoint->m_lat, _T("N"));

     if (pActivePoint->m_lon < 0.)
       m_NMEA0183.Rmb.DestinationPosition.Longitude.Set(
         pActivePoint->m_lon, _T("W"));
     else
       m_NMEA0183.Rmb.DestinationPosition.Longitude.Set(
         pActivePoint->m_lon, _T("E"));

First of all, it looks wrong to me: "W" longitudes are negative, the same for "S" latitudes.

Also, we should not use the useless, disturbing _T() macro in new code.

Isn't it better written like:

 m_NMEA0183.Rmb.DestinationPosition.Latitude.Set(std::fabs(pActivePoint->m_lat),
                                                 m_lat < 0 ? "S" : "N");
 m_NMEA0183.Rmb.DestinationPosition.Longitude.Set(std::fabs(pActivePoint->m_lon),
                                                  m_lon < 0 ? "W" : "E");

EDIT: abs() -> fabs() -> fabsf()

EDIT2: Sorry for the mess, reading c++ docs is what it is. fabs() seems to fit the bill, though.

[Hakansv]

I didn't touched the lat/lon. Just copied from before.
This is the result here at home. N/E is correct
$ECRMB,A,0.127,R,001tes,002tes,5740.194,N,01149.413,E,0.311,270.363,9.429,V,A*64

[leamas]

Either it was already buggy, or you missed something when copying. Compare to for example around line 533:

    if (gLat < 0.)
      m_NMEA0183.Rmc.Position.Latitude.Set(-gLat, _T("S"));
    else
      m_NMEA0183.Rmc.Position.Latitude.Set(gLat, _T("N"));

    if (gLon < 0.)
      m_NMEA0183.Rmc.Position.Longitude.Set(-gLon, _T("W"));
    else
      m_NMEA0183.Rmc.Position.Longitude.Set(gLon, _T("E"));

Note that when gLat < 0 it is using -gLat rather than gLat . It is about that when using N and S all latitudes are positive, they cannot be negative. Same goes for longitude. Also note that this only matters being W and/or S, in our parts of the world which is N/E it doesn't make any difference

Now, my four lines makes the same thing as the snippet above, and I think it is overall better. But in any case, the bug needs to be fixed, and removing _T() whenever found is the right thing to do.

[Hakansv]

About RMC lat/lon.
Yes you're correct. (As usual). I made the bug for (S) and (W) by taking away (-)minus signs by falsely assume they where Git's deleting sign when they where placed on new rows by VS. ("fast and fail")
Good you're here. I'll fix.
(Even though I'm not a proper developer I've though very fast learned to say "I didn't touched that!") :)

[Hakansv]

Now I think it's finally ready. A long story for a small change but now good.

[leamas]

hrmpf... 13 lines instead of four to do the same thing ;) But this is just a matter of style, I guess.

Don't worry about the error. This is the kind of error anyone can do from time to time. And it is always great when someone else looks at the code, four eyes sees so much more than two.

I you could rebase and just change the commit comment to read Prevent AP $ECRMB from being more than 82 characters (#3914) I am ready to merge this as soon as all builds are done. Having a link to the bug in the commit makes the history so much easier to read.

[Hakansv]

Done! Thanks.

[leamas]

Thanks for the finding and the PR!