ge0rg / aprsdroid

APRSdroid - Geo-Location for Radio Amateurs
https://aprsdroid.org/
GNU General Public License v2.0
514 stars 100 forks source link

bug: Improper data/comment field construction #325

Open Tyler-2 opened 2 years ago

Tyler-2 commented 2 years ago

At this line, a space is inserted between the status var (Referred to as Comment field in the UI, I think), and the preceding speed, freq, alt vars.

https://github.com/ge0rg/aprsdroid/blob/2041ba3497647c279b1bd61a37f98042c1329cc7/src/AprsService.scala#L242

The comment in APRS terminology is always the last field: Screenshot_2022-05-27_14-52-15 (APRS 1.0 doc, page 94, APRS Data Formats)

Going forward I will refer to the comment meaning the user-provided comment, consistent with the APRS spec documentation of comment. APRSDroid internally refers to (I think) the comment as status. And erroneously (again, I think) calls the variable which contains APRS data extensions and the APRS comment, "comment". I believe this is a mistake.

The frequency and altitude data is not data extension but are actually just text that is treated as part of the comment field.

APRSDroid uses un-timestamped position reports.

Course/speed is an optional data extension component in the APRSDroid packets.

Lat/Long reports without timestamps or data extension components

This is a Lat/Long position report without timestamp, sent from APRSDroid: =3742.94N\07749.29Wk Some comment Screenshot_2022-05-27_14-59-14

Note the space between the Wk and Some comment. W is the end of the Long field. k is my symbol code. The comment then would be Some comment. Wrong.

Screenshot_2022-05-27_15-23-23 Page 32, "Position and DF Report Data Formats". Unfortunately their examples in the above are a little unclear. In their first example Test 001234 is the comment. In the second example, the comment is Test /A=001234.

Screenshot_2022-05-27_15-28-09

The altitude is ALWAYS 6 characters long in the comment. Therefore it does not need to be followed by a space, but there might be other reasons to do that.... If you see /A= in the comment field, the next 6 characters are the altitude in feet.

So it seems to me that the space is inappropriate in the case of =3742.94N\07749.29Wk Some comment which should instead read =3742.94N\07749.29WkSome comment

and packets that contain altitude data maybe shouldn't look like: =3742.94N\07749.29Wk/A=000169 Some comment but should instead look like =3742.94N\07749.29Wk/A=000169Some comment or =3742.94N\07749.29WkSome comment /A=000169 if the example is to be followed without explanation.

Section conclusion: The space preceding the APRSDroid comment is not appropriate except maybe when the /A=xxxxxx precedes it.

Lat/Long reports without timestamps, with data extension components

APRS data extension fields are always 7 byte. Which means they do not need a delimiter (like a space) to terminate them. But they do occasionally need a space sometimes to pad them out. But that's not what's happening in APRSDroid.

Confusion when using the frequency object

http://www.aprs.org/info/freqspec.txt APRS FREQUENCY FORMATS section adds some confusion here. The frequency object is also part of the comment field. Some effort was made to make sure this was easily human readable but also parse-able.

A simplified summary of the rules for the frequency object in APRSDroid is:

They include this example: FFF.FFFMHz comment...

This is the only reference to a trailing space for the frequency object, and the space is never mentioned otherwise as far as I can tell.

However there is reference to an optional "not in the spec" leading space:

OTHER COMBINATIONS: It is important to note that the TEXT field used for this frequency already has several possible options. Normally the comment-TEXT field begins after the SYMBOL byte. But there are already several defined 7 byte Data-Extensions that then push the comment-TEXT field further to the right as shown here. In many applications, an optional delimiter is added to make the packet more readable. This optional delimiter (usually a " " or "/") is not explicitly called out in the spec, but should be considered in all following parsing of the text field. All of these can be valid use of Data-Extensions and following FREQUENCY info.

!DDMM.mmN/DDDMM.MMW$FFF.FFFMHz ... begins after symbol ($) !DDMM.mmN/DDDMM.MMW$CSE/SPD/FFF.FFFMHz ... !DDMM.mmN/DDDMM.MMW$PHGphgd/FFF.FFFMHz ... !DDMM.mmN/DDDMM.MMW$DFSshgd/FFF.FFFMHz ... !DDMM.mmN/DDDMM.MMW$DFSshgd FFF.FFFMHz ... with SPACE insted of / !DDMM.mmN/DDDMM.MMW$CSE/SPDFFF.FFFMHz ... w/o delimiter !DDMM.mmN/DDDMM.MMW$PHGphgdFFF.FFFMHz ... w/o delimiter

LEADING SPACE ISSUE: Althout the optional delimiter shown here is a "/" it can also be a SPACE character too. The original intent of APRS was that any free text field that begins with either of these two delimiters, then they can be ignored and the beginning of the text field begins after them. We believe that Kenwood and maybe others did not allow for this option, and so a leading space will not work. We are trying to clarify this.... now in 2012...

It isn't clear to me what they meant to do with the leading delimiter on the frequency data. The leading delimiter isn't necessary for decoding...

The reference to the intent there implies that if you use the free form text comment field to contain interesting custom attributes like frequency, you can delimit those with a space or /. That way, maybe, APRS clients that don't understand them are able to skip over them. But I don't understand how you're supposed to know that the field is "over". Spaces can't be delimiters all the way through the comment, in this way....

So there can be a space or / or other delimiter before the frequency object "to make it more readable" (for who?). That's a leading space.

A packet with a frequency object and altitude in APRSDroid currently looks like this: =3742.94N\07749.29Wk146.520MHz/A=000158 Some comment and I believe it should look like this, if the "leading space" is actually desirable: =3742.94N\07749.29Wk146.520MHz /A=000158Some comment

Impact

Suggested fixes

Make the comment field consist of one of, depending on which fields are defined:

status_spd +...

I wouldn't actually even want to put a leading or following space in front of status_alt but based on the example packets it seems allowable.

TLDR

It appears APRSDroid is unconditionally inserting a space between the comment field and the symbol code, when there should ~not be a space there at all~ only maybe be spaces involved between the altitude, frequency, and user provided comment fields.

Tyler-2 commented 2 years ago

There may be all sorts of errors there since I wrote it while perusing the spec.... The suggested fix and TLDR are probably sufficient to get the gist.