Added support for TagBlocks #59
Conversation
TagBlock is now parsed before any other messages and is a part of BaseSentence
| TypeSourceID = "s" | ||
| // TypeTextString valid character string, parameter -t | ||
| TypeTextString = "t" | ||
| ) |
There was a problem hiding this comment.
I don't think we need these constants in the public API. Since they're only used in the one spot, I think it would be better to just inline them into the switch.
Move the comments from these types to the TagBlock struct fields.
| } | ||
|
|
||
| // Timestamp can come as milliseconds or seconds | ||
| func validUnixTimestamp(timestamp int64) (int64, error) { |
There was a problem hiding this comment.
This seems hacky. I think we should leave it up to the user of the package to know what units their device is emitting. I'd prefer to just pass along the value we receive instead of trying to be clever with it.
There was a problem hiding this comment.
There was some debate about how to handle this on the gpsd mailing list. ESR's document (https://gpsd.gitlab.io/gpsd/AIVDM.html) says:
As of May 2014 no NMEA 4.10 relative time fields have been observed in the wild. It is unknown whether the unit is seconds or milliseconds.
I've worked with tagblock timestamps from AISHub.net, as well as a commercial satellite AIS provider, and so far have only seen seconds, never milliseconds. In light of this, beyond agreeing with @icholy about just passing the value along as an int64, I'd suggest documenting that the value should be seconds, not milliseconds (but warn the user that YMMV).
|
|
||
| items := strings.Split(tags[:sumSepIndex], ",") | ||
| for _, item := range items { | ||
| if len(item) == 0 { |
There was a problem hiding this comment.
Shouldn't this be if len(item) < 2 { ?
Actually, I think a cleaner way would be to just use strings.Split on it.
parts := strings.SplitN(item, ":", 2)
if len(parts) != 2 {
continue
}
key, value := parts[0], parts[1]| Text: "helloworld", | ||
| LineCount: 13, | ||
| }, | ||
| //err: "nmea: tagblock checksum mismatch [25 != 49]", |
There was a problem hiding this comment.
don't commit commented-out code
| case TypeUnixTime: | ||
| tagBlock.Time, err = parseInt64(item) | ||
| if err != nil { | ||
| return tagBlock, raw, err |
There was a problem hiding this comment.
return "" instead of raw and TagBlock{} instead of the partially constructed tagBlock variable. This makes it explicit that these aren't being used by the caller. Do this at all the other error returns too.
|
@klyve thanks a lot for the contribution, you did a great job adding support for the TagBlock! The PR has been inactive for quite some time and I was wondering if you would be willing to address @icholy comments (and maybe a few more review rounds) and eventually get it merged. If you don't have cycles or are not interested anymore please let us know and I'll probably take over the work. Thanks you! |
Signed-off-by: Adrián Moreno <adrian@morenomartinez.com>
* 'master' of github.com:klyve/go-nmea: Fix checksum ref after branch update Correct copy paste typo Add VHW water speed and heading functions returning a direction sign have been added added latitude range checking added estimated fix which is returned by MTK3339 add mtk test expose xorChecksum revert xorChecksum expose use the parser att support for parsing MTK commands and update go mod files
|
@adrianmo @klyve in the interest of getting this very useful code merged, we have implemented the requested changes in a fork: https://github.com/xerra-eo/go-nmea What would be the best way to proceed? A new PR? |
|
@simeonmiteff yes |
* Added support for TagBlocks TagBlock is now parsed before any other messages and is a part of BaseSentence * Added tests for linecount and relative time * Early exit if no matches in tagblock * Fixed PR comments * Renamed parseUint to parseInt64 * Added validation for timestamp according to nmea spec * changed go mod to fork * Fix checksum ref after branch update Signed-off-by: Adrián Moreno <adrian@morenomartinez.com> * Remove tag string constants Addresses #59 (comment) * Pass through timestamp, don't try to normalise Addresses #59 (comment) * Clean up tag key/value parsing Addresses #59 (comment) * Remove commented code Addresses #59 (comment) * Explicitly blank values on error returns Addresses #59 (comment) * Revert module name to github.com/adrianmo/go-nmea * Update readme: note tag blocks feature and code example * Reference better resource for TAG Blocks info Addresses #78 (comment) * Make a malformatted field and error Addresses #78 (comment) * Remove blank lines Adresses #78 (comment) * Find tag block with strings.SplitN instead of regexp Addresses #78 (comment) * Strip tagblocks in parseSentence, remove prefix support * Add tagblock parsing tests for parseSentence() * Test parseTagblock directly (without the full sentence) Co-authored-by: Bjarte Klyve Larsen <bjarteklarsen@gmail.com> Co-authored-by: bjarte <bjarte@maritimeoptima.com> Co-authored-by: Adrián Moreno <adrian@morenomartinez.com>
Satellites and some sources apply a tag block to the messages.
TagBlock is now parsed before any other messages and is a part of BaseSentence
Tagblocks:
https://www.nmea.org/Assets/may%2009%20rtcm%200183_v400.pdf
https://rietman.wordpress.com/2016/09/17/nemastudio-now-supports-the-nmea-0183-tag-block/