Mic-E & Compressed #397
Mic-E & Compressed #397
Conversation
There was a problem hiding this comment.
Thanks very much for the PR! I've left a bunch of comments marked as follows:
- [important] = this needs to be resolved before merging the PR
- [optional] = it would be nice, but I'm not insisting on them.
Please let me know if you would like to attempt the changes on your own, if you need some assistance, or if you'd prefer me to change the code accordingly.
| android:parentActivityName=".PrefsAct" | ||
| android:launchMode="singleTop" | ||
| android:configChanges="orientation|keyboardHidden|screenSize" /> | ||
|
|
There was a problem hiding this comment.
[important] It would be more intuitive to have the compression setting use a de.duenndns.ListPreferenceWithValue drop-down with the title "Beacon compression" and the values "uncompressed", "compressed" and "Mic-E".
Together with the "Mic-E Status" that's down to two menu items, so they can appear in the main preferences screen and not be hidden in a sub-menu. The "Mic-E Status" menu then can be enabled/disabled based on the value of the tri-state.
| <item>Special</item> | ||
| <item>Priority</item> | ||
| <item>EMERGENCY!</item> | ||
| </string-array> |
There was a problem hiding this comment.
[optional] From APRS12b I take it that the "Custom 0...6" status types do not need to be supported.
[important] I'm not sure right now if the standard status type should be translated into the respective user locale or kept in English. In the former case, the array should be moved into the strings.xml file.
| @@ -110,4 +110,14 @@ | |||
| <item>460800</item> | |||
| <item>921600</item> | |||
| </string-array> | |||
| <string-array name="compressed_mice_status"> | |||
There was a problem hiding this comment.
[optional] For consistency, the name should be p_mice_status_e (preferences, Mic-E status, entries)
| "W", "X", "Y", "Z" | ||
| ) | ||
|
|
||
| def statusToBits(status: String): (Int, Int, Int) = status match { |
There was a problem hiding this comment.
[important] the function should be called miceStatusToBits() for consistency, also using the English string value is not a robust way to obtain the value. You should add a android:entryValues= property to the drop-down, and then you can encode the bit values ("111", "110", ...) in the compressed_mice_status_ev array.
[optional] But I'd rather use the numeric IDs (position in the array), 0, 1, 2, 3, ... and convert them to the respective bits algorithmically. PrefsWrapper.getListItemIndex() will return the numeric ID, and the bit-ops should be:
val a = ((mice_status_id >> 2) & 1) ^ 1;
val b = ((mice_status_id >> 1) & 1) ^ 1;
val c = ((mice_status_id >> 0) & 1) ^ 1;
src/AprsPacket.scala
Outdated
| (degrees, minutesInt, minutesHundreths) | ||
| } | ||
|
|
||
| def encodeDest(dd: Double, longOffset: Int, west: Int, messageA: Int, messageB: Int, messageC: Int, ambiguity: Int): String = { |
There was a problem hiding this comment.
[important] should be called encodeMiceDest
| encoded.take(6 - validAmbiguity) + "Z" * validAmbiguity | ||
| } | ||
|
|
||
| def encodeInfo(dd: Double, speed: Double, heading: Double, symbol: String): (String, Int, Int) = { |
There was a problem hiding this comment.
[important] should be called encodeMiceInfo(). Also you are passing the latitude (as the dd parameter, but you are not encoding the ambiguity here. The code should also treat location ambiguity for latitude correctly.
| (sb.toString(), west, longOffset) | ||
| } | ||
|
|
||
| def altitude(alt: Double): String = { |
There was a problem hiding this comment.
[important] it's not clear what this function does. It should be called formatAltitude<something> where something tells the format. I guess it's formatAltitudeMice() but I'm not quite sure.
| val val3 = rem % 91 | ||
|
|
||
| // Ensure that the characters are treated as strings and concatenate properly | ||
| charFromInt(val1).toString + charFromInt(val2).toString + charFromInt(val3).toString + "}" |
There was a problem hiding this comment.
[optional] this line's indentation is not at the same level ;)
| val micePacketParsed = Parser.parse(micePacketString) | ||
|
|
||
| micePacketParsed | ||
| } | ||
| def formatLoc(symbol : String, status : String, location : Location) = { |
There was a problem hiding this comment.
[optional] not sure if it would look cleaner if we'd split the compressed packet encoding into formatLocCompressed and then call that separately, instead of treating both cases from one method.
| def postLocation(location : Location) { | ||
| var symbol = prefs.getString("symbol", "") | ||
| if (symbol.length != 2) | ||
| symbol = getString(R.string.default_symbol) | ||
| val status = prefs.getString("status", getString(R.string.default_status)) | ||
| val packet = formatLoc(symbol, status, location) |
There was a problem hiding this comment.
[important] I'd prefer to keep the logic in this method agnostic to the used compression mechanism, so my suggestion would be:
- rename
formatLoctoformatLocUncompressed - move the compressed location encoding to
formatLocCompressed - make a new
formatLocwrapper method that will call and return the appropriateformatLocXaccording to the preference value.
|
Fixed Ambiguity & MessageC problem. |
Initial commit for Mic-E and Compressed beacons.