semuconsulting / pynmeagps

Python library for parsing and generating NMEA 0183 GNSS/GPS protocol messages.
BSD 3-Clause "New" or "Revised" License
78 stars 28 forks source link

IN* messages not getting parsed from stream in NMEAReader #46

Closed jsainio closed 10 months ago

jsainio commented 10 months ago

In an environment where the talker is not e.g. GP but IN instead, the NMEAReader stream reading refuses to parse anything silently. However parse() function works for these out of the box. The reason is that currently NMEAReader uses NMEA_HDR in its read() function which completely ignores many talkers such as IN. There is no easy way to augment this list from scripts using the pynmeagpslibrary, and you have to resort to patching the library instead.

As originally discussed in https://github.com/semuconsulting/pynmeagps/issues/45#issuecomment-1817639008

You need to provide a little more information on the rationale behind the change (e.g. what is the origin of the INGGA messages?) [...] Re. the change you've made, I'm not sure this is necessarily the safest approach. "IN" is a recognised Talker ID (Integrated Navigation). I would be inclined to simply add "$I" to the list of acceptable NMEA_HDR values. But happy to be challenged in the Discussions channel.

Sorry, I don't have all the information about the source. I'm not sure exactly what is the source as I was troubleshooting an issue for a friend and I don't work with or have available the actual environment where this is used. All I know it involves a large ship and probably some integrated navigation equipment related to it. For testing I just created a quick local mock server. All the messages received in this environment are from an IN talker source.

I did consider augmenting the NMEA_HDR list, but it felt a bit wrong to add just one more thing there. The thing is, there are so many many talker IDs listed in here, but basically the default NMEA_HDR list ignores most of them without explained rationale or a way to configure it without having to patch the library source, so that's why I'm suggesting to open it up in the NMEAReader constructor instead as in my original PR. Basically, I mean, the library user should be able to choose what talkers to listen to.

semuadmin commented 10 months ago

@jsainio Happy to take this on board. The pynmeagps library (as opposed to some other generic NMEA libraries) was primarily intended to be used with standard NMEA GNSS receivers, which are generally limited to Talker IDs Gx and Px (though there are some exceptions) - hence the restricted NMEA_HDR values. But it's a fair point that the same NMEA GNSS sentences can originate from a limited number of other Talker IDs, including 'IN', and there's no reason not to accommodate them here. FYI other GNSS libraries, including pyubx2, have a dependency on pynmeagps and the NMEA_HDR value to parse mixed-protocols streams (i.e. those containing NMEA mixed in with other protocols like UBX or RTCM) so I need to ensure any change does not have negative upstream impacts.

I'd be interested to know which specific device your friend is using and, if possible, obtain some real sample GGA data from that device if that's possible.

jsainio commented 10 months ago

Thanks a lot!

FYI other GNSS libraries, including pyubx2, have a dependency on pynmeagps and the NMEA_HDR value to parse mixed-protocols streams (i.e. those containing NMEA mixed in with other protocols like UBX or RTCM) so I need to ensure any change does not have negative upstream impacts.

Got it. At least my originally proposed change is kind of backwards compatible with that but I understand you're working in a more complex dependency environment so I wholeheartedly understand the testing needs and appreciate your testing approach.

I'd be interested to know which specific device your friend is using and, if possible, obtain some real sample GGA data from that device if that's possible.

I'll ask if they're at a liberty to tell what equipment it is and also show some sample data. It might need some sanitation.

semuadmin commented 10 months ago

@jsainio FYI v1.0.30 includes a change to cater specifically for your 'IN' Talker ID, but the next version (v1.0.31) will include a change to cater for all current valid Talker IDs i.e. NMEA_HDR will be set as follows:

NMEA_HDR = {b"\x24" + i[0:1].encode("utf-8") for i in NMEA_TALKERS}

Note that, contrary to popular wisdom, NMEA 0183 is actually a proprietary protocol. pynmeagps is open-source software. I'm able to include the GNSS-related sentence definitions (GNGGA, etc.) in nmeatypes_get.py because these definitions are already in the public domain, but the same is not necessarily true for a variety of other standard and proprietary NMEA sentence types.