search

ab0oo / javAPRSlib

Java APRS Parser Library
GNU Lesser General Public License v3.0
25 stars 17 forks source link

Restructure of APRSPacket #15

Open ab0oo opened 3 years ago

ab0oo commented 3 years ago

I'm unhappy with the data structures we're using for the base APRSPacket. Hear me out:

I've been working on adding weather data parsers. It's not too complex, but it highlights the different ways the data can be presented. If you refer to chapter 12 of the APRS Spec, there are at least 5 different ways to encode weather information. Some contain positions, some contain timestamps. This makes the object hierarchy get messy pretty quickly. Looking at the chart in Chapter 5 (page19) and the "APRS Data and Data Extension" table on page 18, it looks like there should be an interface called "APRSData" and an interface called "APRSDataExtension". An APRSPacket can contain multiple instances of the interface (i.e. a packet can contain Lat/Long Coordinates AND Raw Weather Station Data). I'm proposing that the APRSPacket contain a Set of APRSData objects, and a Set of APRSDataExtension objects. This is probably going to break some stuff downstream, so I'm looking for input here. I'm also thinking about how to indicate at a high-level what information a packet actually contains. I'm not sure we should have to walk an entire set to see if a packet contains a position. Perhaps a set of boolean values like hasPosition(), hasWeather(), hasTelemetry(), etc?

ge0rg commented 3 years ago

I've been pondering about this issue for a long while already, and I fully agree that we need a more flexible data structure.

So far we have:

We also have different extension elements already, of which currently only one can be inside the above InformationField:

I'm not sure if we should rename InformationField into APRSData, but I rather dislike all the *Packet field names because they are actually just Fields, so if we are going to do a major breaking change refactoring, let's axe that name.

I agree that having only one extension in a packet is not sufficient, but I'm not sure if it makes sense to have multiple information fields in the frame, or if we can use the extensions for complex data types. Also I think it is good to have a primary information field that defines what kind of packet this is - if it's a weather packet, maybe it doesn't make sense to treat it as something different? Also I'm not sure if it's enough to have one extension per extension type, or if we need to support lists. Maybe @hessu can provide insight on a good data structure to cover APRS packets?

My suggestion for a large-scale refactoring (for which I'm lacking the time) would be as follows, each item can be disputed / discussed individually:

  1. Move classes into according packages (directories) based on their function, i.e. get most things out of the parser package, have an aprsdata or field package, an extension package, and some package for individual tokens like Callsign and Digipeater (maybe "token"?)
  2. Rename *Packet into *Field
  3. Change InformationField's extension field into a HashMap<Class<DataExtension>, DataExtension> for one extension per type, or with a List<DataExtension> value type if we want to support multiple elements
  4. Convert Position, which is a custom element only contained in PositionPacket into a DataExtension so that it can be combined with everything else
  5. Add to InformationField a generic method <T extends DataExtension> getExtension(Class<T>) so that we can directly run code like this: CourseAndSpeedExtension cse = packet.getExtension(CourseAndSpeedExtension);
  6. Add a generic convenience method boolean hasExtension(Class<T>)

My Java generics knowledge is a bit rusty, so don't pin me on the exact syntax, so please consider the above as pseudo-code.

ab0oo commented 3 years ago

I just pushed a v2.0 branch that makes many of the changes you suggested, @ge0rg . I didn't do a whole lot with the POSITION stuff, but I basically created a hierarchy inside an APRSPacket that contains a Set of APRSData fields (i.e. position, timestamp, weather, object and item), a single APRS Data Extension, and a field for a comment. I didn't restructure the package per bullet 1, but I'm looking at that next. I took care of most of bullet 2. Bullet 3 is close. I used a Set instead of a List, and I need to implement the HashMap for DataExtension. Bullet 4 is done. Bullets 5 and 6... working on those. Definitely a step in the right direction. Also, there are a LOT of malformed packets floating around out there... :) I'm going to keep this issue open until I get all of your bullet points done, and maybe some feedback.

arodland commented 3 years ago

@ab0oo well, it looks like you've already done what I offered to do, and then some :) one thing I can offer: there are some test cases from the original Ham::APRS::FAP in https://metacpan.org/release/HESSU/Ham-APRS-FAP-1.21/source/t/30decode-wx-basic.t and https://metacpan.org/release/HESSU/Ham-APRS-FAP-1.21/source/t/31decode-wx-ultw.t

penguin359 commented 2 years ago

I have been getting back into Android development after a year hiatus and have been meaning to finish up my own attempt at supporting weather packets. I remember getting stalled out last time for exactly the reason about how to handle the many different weather formats specified in the APRS 1.01 specification. Some are natural extensions of the GPS location beacons, but other formats are clearly a completely different type, of course, but I don't want to re-iterate what's already been said.

It looks like quite a bit of work has been done since I was last here so I'll need a day or so to catch up. In the mean time, I've been working on updating the unit test frameworks so they can be used while working on these weather station packets. Now that GitHub can do it's own CI, I've added that as a pull request for both the master branch (#16) and this v2.0 branch (#17). In doing so, I noticed some commits did not make it onto the v2.0 branch including one commit that's needed to fix Apache Ant builds. Ant is currently broken on v2.0. I've included that commit cherry-picked into my pull request so that the CI tests will pass the build for both Maven and Ant.

Several other commits are missing related to the BEACON issue (#10) and my additional unit-tests (#13). I can work on pulling them in later.

penguin359 commented 2 years ago

While reviewing the v2.0 branch, my first question is, why bump the target version all the way to Java 15?

I did feel that supporting Java 6 was a bit tiresome, but 15 seems to limit the environments this can be used in. The Long-Term Support releases of Java are 8, 11, and now 17 which was just released in 2021. That means that only the latest Java is an option for those wanting to use javAPRSlib v2.0 with an LTS release.

Outside of Android development, I am not a big Java developer so I am not as certain what the ecosystem looks like, and I'm not exactly certain how Java versions relate to specific Android versions. From what I can tell, the recommended version of Java is Java 11 LTS, though, the default target/source for new version is 1.8 with the default build files that are generated.