NMEA extensions broken

[nmichaels]

Adding proprietary NMEA extensions appears to have been broken with 1.0.43.

How I added my messages:

#nmea/nmea.py:

from pynmeagps import *

get_dict = nmeatypes_get.NMEA_PAYLOADS_GET
msgids = nmeatypes_core.NMEA_MSGIDS_PROP

msgids["MYMSG"] = "My Special Message"

get_dict["MYMSG"] = {
    "data": IN,
}

# nmea/__init__.py:
from .nmea import *

In a Python shell:

>> import nmea
>> nmea.NMEAReader.parse(b'PMYMSG,0*01').data

What I see with 1.0.41:

0

What I see with 1.0.42:

nmea.NMEAReader.parse(b'PMYMSG,0*01').data
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'NMEAMessage' object has no attribute 'data'

With qlxnmea==1.0.42

Environment:

Debian bookworm

$ uname -a
Linux phase 6.1.0-22-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.94-1 (2024-06-21) x86_64 GNU/Linux
$ pip freeze
pynmeagps==1.0.42

I believe this was introduced by the addition of a default NMEA message type in 126dae2

[nmichaels]

nmea.zip
Here's the directory with the example code so you don't have to type it out.

[semuadmin]

Hi @nmichaels,

I can see how the changes in v1.0.41 & v1.0.42 would have affected your code.

In v1.0.41 a change was introduced to allow pynmeagps to parse any unknown (but structurally valid) NMEA sentence to a nominal NMEAMessage data structure, but the attributes of that nominal data structure will always be field_01, field_02, etc.

In v1.0.42 I basically split out the proprietary message definitions to a separate file nmeatypes_get_prop.py, as the original nmeatypes_get.py file was starting to get a bit unwieldy.

So, to take your example, it is now possible to parse any 'unknown' sentence like b'$PMYMSG,0*01\r\n' as follows:

(remember - a valid NMEA sentence must start with "$" (0x24) and end with "\r\n" (0x0D0A))

from pynmeagps import NMEAReader

msg = NMEAReader.parse(b"$PMYMSG,0*01\r\n")
print(msg)
print(msg.field_01)
msg = NMEAReader.parse(b"$PMYMSG2,0,THISFIELD,THATFIELD,2,3,4,5,*10\r\n")
print(msg)
print(msg.field_03)

<NMEA(PMYMSG, NOMINAL, field_01=0)>
0
<NMEA(PMYMSG2, NOMINAL, field_01=0, field_02=THISFIELD, field_03=THATFIELD, field_04=2, field_05=3, field_06=4, field_07=5, field_08=)>
THATFIELD

But I'm not sure your code was ever an intended use case for pynmeagps. You appear to be trying to dynamically create new 'proprietary' NMEA sentence types and then parse them - I'm not clear what you would be looking to achieve by doing this? If you can help me understand the objective here, I can evaluate whether it's best accomodated as a change to pynmeagps, or a change to your own code.

Thanks

[nmichaels]

nmea.zip

Whoops, there's the zip file with the files actually in it. I didn't change the files though; weird that you couldn't reproduce without changes. Anyway, it sounds like you do see what's going on so I'll explain my use case.

First, why I'm doing this: I'm developing a device that spits out NMEA in its normal course of operation, and I want to include some debug information in-band. This lets me use a single parser for all the messages which simplifies my test scripts.

So to do that, I added some proprietary NMEA sentences and a separate package that depends on pynmeagps. My scripts can just install my-nmea and through its parser they inherit all the standard NMEA stuff plus parsers for my debug messages. I read the extensibility section of the README and it seemed like what it suggested ("Additional message types can be readily added to the appropriate dictionary") was what I did. The only alternatives I see are to try to push my debug messages upstream to pynmeagps, which doesn't make sense, or to fork pynmeagps, which seems like too big a hammer.

Do you have any suggestions?

[semuadmin]

Whoops, there's the zip file with the files actually in it.

FYI I don't normally like to open *.zip or *.py files - it's easier and safer for me and other potential respondents if posters can embed their complete code in the body of the discussion.

"Additional message types can be readily added to the appropriate dictionary"

So, just to qualify that statement, the intention here was that new message types can be added to the dictionary via a PR and duly incorporated into the next public Release. There was never any intention to support dynamically-generated message definitions in real time, and I see no viable use case for that within the NMEA protocol. Obviously the expectation is that users will invoke pynmeagps through its documented public API rather than 'monkey-patching' its internal code.

The rationale behind the change in v1.0.41 was to allow pynmeagps to parse any structurally valid NMEA message to a usable data structure even if its official payload definition has not yet been incorporated into the dictionary. It's a kind of 'fail safe', if you like.

Do you have any suggestions?

If I understand your intention correctly, it sounds like you want to use custom NMEA sentences essentially as debug messages?

pynmeagps does support the standard Python logger, so you're at liberty to add standard logging statements during testing if that meets your need e.g. self._logger.debug("this is some debug information"). You can configure the logger to output to stderr or a log file, or any other output device you care to set up beforehand.

If you absolutely, positively have to use an NMEA message, could you not just (ab)use an existing standard NMEA sentence type which supports configurable text like, for example, EVE (General Event Message) with one of the permitted user-defined talker IDs:

from pynmeagps import NMEAMessage, GET

msg = NMEAMessage(
    "U0", "EVE", GET, tagcode="debug", eventtext="this is some debug information"
)
print(msg)

<NMEA(U0EVE, time=18:16:03.691838, tagcode=debug, eventtext=this is some debug information)>

But I trust your proposed device would not be outputting these debug messages in production operation?

[nmichaels]

Whoops, there's the zip file with the files actually in it.

FYI I don't normally like to open *.zip or *.py files - it's easier and safer for me and other potential respondents if posters can embed their complete code in the body of the discussion.

For what it's worth, I did include the complete code in the body. The zip file's contents are just the code in the thread pasted into files in a directory.

"Additional message types can be readily added to the appropriate dictionary"

So, just to qualify that statement, the intention here was that new message types can be added to the dictionary via a PR and duly incorporated into the next public Release. There was never any intention to support dynamically-generated message definitions in real time, and I see no viable use case for that within the NMEA protocol. Obviously the expectation is that users will invoke pynmeagps through its documented public API rather than 'monkey-patching' its internal code.

I had assumed that since NMEA_MSGIDS and NMEA_MSGIDS_PROP were explicitly mentioned in the documentation that they were part of the public API. I don't think that's unreasonable; Python is a dynamic language.

The rationale behind the change in v1.0.41 was to allow pynmeagps to parse any structurally valid NMEA message to a usable data structure even if its official payload definition has not yet been incorporated into the dictionary. It's a kind of 'fail safe', if you like.

Do you have any suggestions?

If I understand your intention correctly, it sounds like you want to use custom NMEA sentences essentially as debug messages?

pynmeagps does support the standard Python logger, so you're at liberty to add standard logging statements during testing if that meets your need e.g. self._logger.debug("this is some debug information"). You can configure the logger to output to stderr or a log file, or any other output device you care to set up beforehand.

I think there's a misunderstanding here. I'm not debugging Python code; I'm debugging the thing on the other end of the serial port. Python's logging functionality doesn't help me here.

If you absolutely, positively have to use an NMEA message, could you not just (ab)use an existing standard NMEA sentence type which supports configurable text like, for example, EVE (General Event Message) with one of the permitted user-defined talker IDs:

I suppose I could, but that might clash with a legitimate use my device might have for that message type in the future. I think it's better to have a completely orthogonal message type (or set of message types) that only I generate. We will actually be outputting some of these debug messages in production operation (after the protocol stabilizes) at least some of the time. Since NMEA allows manufacturer-specific messages to be added, that seems like a better approach than hacking in an overload of an existing message.

The rationale behind the change in v1.0.41 was to allow pynmeagps to parse any structurally valid NMEA message to a usable data structure even if its official payload definition has not yet been incorporated into the dictionary. It's a kind of 'fail safe', if you like.

That seems reasonable, but it looks like the way it was implemented precludes additions to the dictionary. I guess what I'd most like is an officially sanctioned mechanism for extending the set of messages supported by the library without forcing them on the upstream project. Of course if we do end up stabilizing the protocol and shipping hardware that outputs new proprietary messages, we would want to submit them back here so that our users would be able to use them easily.

I'm happy to keep pushing messages into a dictionary, but a function call API would be good too. If you'd like, I can dig deeper into the code and try to come up with a way to do the default message thing that doesn't break my extensions, or maybe propose an alternate extension mechanism.

[semuadmin]

Hi again @nmichaels

I suppose I could, but that might clash with a legitimate use my device might have for that message type in the future

absolutely! - it would have been a "bodge".

NMEA_MSGIDS and NMEA_MSGIDS_PROP are global constants (hence upper case, as per PEP8).

I can consider adding functionality to allow temporary dynamic additions to these core dictionaries (e.g. by allowing users to reference an externally-defined dictionary rather than the internal constant). I doubt it would be a particularly difficult change - I just never foresaw a need for it. Let me look into this. cpython optimises constant attributes at runtime so changing these attributes to mutables would have a (slight) negative affect on performance, which I'd prefer to avoid.

In the meantime, of course, pygnmeagps will happily parse any novel proprietary NMEA sentence you care to throw at it to a nominal data structure (field_01 etc.). Would this not be acceptable for debug information or as an interim solution - just means you'd need to access msg.field_01 rather than, say, msg.data?

As you say, if the intention is that your device will output new proprietary NMEA sentences in production in due course, I would still advocate adding these to the dictionary constants via PR once stable.

[nmichaels]

I think I could work with wrapping field_01 etc. It would mean wrapping the pynmeagps parser interface, since I want my tests to have a single interface for parsing messages (that's the half the point of using NMEA).

An externally-defined dictionary that adds temporarily to the core dictionaries would certainly be preferable from my point of view. I suspect it wouldn't have any performance impact unless it was used, but that's the kind of thing I'd need a profiler to prove.

[semuadmin]

@nmichaels some further thoughts on this...

Adding support for a "user-defined" payload definition dictionary is, in itself, fairly straightforward provided each user-defined message type has a unique identifier as dictionary key e.g. "MYMSG": {}.

While this is true for all standard NMEA messages, it's not true for most proprietary messages, which typically use a common manufacturer's mnemonic code ("UBX", "FEC", etc.) with individual message types being identified by a msgId (or similar) attribute within the payload itself (so the unique key is in effect mnemonic+msgId). Some manufacturers (e.g. Trimble) use a combination of unique and non-unique message identifiers, necessitating special processing within pynmeagps.

Implementing a user-defined dictionary facility which catered for all possible proprietary unique message identifier schemes would, imho, be more trouble than its worth.

I'm minded, therefore, to stick with the recommendation to use pynmeagps's 'nominal' payload parsing facilty for unknown message types and/or simply fork the library and merge the new definitions upstream once they're stable.

But open to other opinions.