Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Group tag block is only partially read since 0bba7e1 #103

Open
imsodin opened this issue Nov 6, 2023 · 6 comments
Open

Group tag block is only partially read since 0bba7e1 #103

imsodin opened this issue Nov 6, 2023 · 6 comments

Comments

@imsodin
Copy link

imsodin commented Nov 6, 2023

The properties on CommentBlockLine besides the parameterMap (e.g. groupId) aren't populated anymore since commit "Correct parsing of the messages in comment block parameters that contains G and g" / 0bba7e1. I discovered this when upgrading in a unit test that checked those values on example nmea lines.

I don't understand that commit. The commit message claims it affects lines containing g and G, but it actually acts on comments that start with \g:. And the new code seems to be an alernative implementation of the same thing (extract tag keys and values, old code by iteration, new code using String.split), with the difference that it skips handling g, G specially, checksum handling and group-id.

From my understanding the old code was working correctly, however I assume something was fixed by that commit - could you please explain what that is.

@dimitrispallas
Copy link
Contributor

We had a problem in our system parsing the messages with comment blocks that start with the \g: prefix. With that commit our problems has been resolved. Please feel free to apply a pull request in order to completely fix the issue! Thanks in advance!

@imsodin
Copy link
Author

imsodin commented Nov 10, 2023

Happy to contribute that, however I need to know what exactly was broken for you (I don't see anything obviously wrong with messages starting with \g:) - could you provide some test data that failed and what you'd expect from it (or a unit test)?

@imsodin
Copy link
Author

imsodin commented Nov 10, 2023

I just checked and the unit test that failed with the new version on my side is also a comment block starting with \g:, and it was working fine before. So I'll definitely need some example to reproduce your issue.

@dimitrispallas
Copy link
Contributor

this is an actual message 24 with comment block that starts with \g:

\g:1-1-21,s:47450,c:1699113260,i:SXSPA:1699115144 I:1699115146 D:1699115146 L:FM136 G:hndgs T:0 1261!AIVDM,1,1,,A,H7Oks<0hTtpE=<0000000000000,24C

Beforehand our system wasn't able to decode and produce the JSON output for that kind of sentence. And we are parsing millions of them in a daily basis. As I already said currently everything works properly in comparison with the previous code.

@imsodin
Copy link
Author

imsodin commented Nov 10, 2023

I got that, I just need to figure out what's wrong to produce a change that works in both cases - thanks for providing an example for that :)

I'll probably have a look at this next week.

@imsodin
Copy link
Author

imsodin commented Nov 23, 2023

Obviously that "next week" prediction didn't pan out xD.
It's basically in my backlog now, but given no-one else seems to be affected it anyway is hardly time-critical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants