semuconsulting / pyubx2

Python library for parsing and generating UBX GPS/GNSS protocol messages.
BSD 3-Clause "New" or "Revised" License
165 stars 65 forks source link

It should be possible to *read* CFG-VALSET packets #73

Closed alinsavix closed 2 years ago

alinsavix commented 2 years ago

Is your feature request related to a problem? Please describe.

When reading a stream of ubx data, pyubx2 is currently unable to parse CFG-VALSET packets. To some degree this makes sense, because a device will never send those packets... but being able to take a saved ubx file on disk that contains a bunch of CFG-VALSET packets, and being able to read it to see what config values are actually getting set, would be an extremely useful thing.

At the moment, trying to parse a file with CFG-VALSET commands gives the following traceback:

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/pyubx2/ubxreader.py", line 379, in parse
    return UBXMessage(
  File "/usr/local/lib/python3.10/site-packages/pyubx2/ubxmessage.py", line 81, in __init__
    self._do_attributes(**kwargs)
  File "/usr/local/lib/python3.10/site-packages/pyubx2/ubxmessage.py", line 104, in _do_attributes
    pdict = self._get_dict(**kwargs)  # get appropriate payload dict
  File "/usr/local/lib/python3.10/site-packages/pyubx2/ubxmessage.py", line 525, in _get_dict
    pdict = ubg.UBX_PAYLOADS_GET[self.identity]
KeyError: 'CFG-VALSET'

(which makes sense, as ubxtypes_get.py has no knowledge of CFG-VALSET packets)

Describe the solution you'd like

Make the above work! These are completely valid packets, it seems like loading and parsing them like any other packets should be a valid behavior.

(Unless there's already a way to do so, but one wasn't obvious from looking at the docs)

Would you be willing to assist with testing?

More than happy to help test in any way I can (though I doubt my help will be needed, though I'm happy to provide example ubx files)

semuadmin commented 2 years ago

Hi @alinsavix,

OK this is highlighted in the README but often gets overlooked...

By default, pyubx2.UBXReader expects to be parsing output messages from the receiver and the msgmode is implicitlyGET.

If you want to parse input or query messages (like CFG-VALSET or CFG-VALGET for example), you need to set the msgmode to SET or POLL respectively. It was done this way for various compelling reasons.

So, for example, if you have a file containing CFG-VALSET or CFG-VALDEL messages:

>>> from pyubx2 import UBXReader, SET
>>> stream = open('file_containing_SET_messages.bin', 'rb')
>>> ubr = UBXReader(stream, protfilter=2, msgmode=SET)
etc.

Try this and let me know how you get on.

alinsavix commented 2 years ago

Wow, I went over the README several times, looked at the code, and even looked through closed issues, and still managed to ask about something that I should have been able to find by myself! Now don't I feel silly. Sorry about that.

I'll give this a look when I get back to the code later this weekend, and close the ticket if I get good results from it (which I expect I will).

I wonder if there'd be any benefit to catching things that exist but only in a different operating mode, and throwing a more useful error message. Even a "I know what this is, but not in this mode" kind of error would have been enough hint to point me the right direction there.

Anyhow, thanks!

semuadmin commented 2 years ago

No worries!

You're not the first to get caught out by this (despite my best attempts to highlight it in the README!), so there may be some merit in amending the error message to add a caveat to say, in effect, "KeyError: 'CFG-VALSET' - check that you're using the correct msgmode". Let me look into that - anything that makes it friendlier is good.

semuadmin commented 2 years ago

Hi @alinsavix FYI I've created a PR to amend the error message to hopefully make the resolution clearer.

Have a look at testWRONGMSGMODE() in test_exceptions.py in branch "improve-KeyError-message" for an illustration of what should happen when you use the correct and incorrect msgmode keyword.

alinsavix commented 2 years ago

Alright, tested things out, and your proposed 'fix' (i.e. fixing my own stupid) works perfectly. I also gave the new PR a test, and it seems to work just fine.

Also, I was thinking about it, and I think I know why it's so easy for people to miss this information in the README -- or, at least, why it was so easy for me to miss: browsing the readme, I think when I saw the section that covered the available modes, I read it not as a parsing mode, but as a usage mode -- e.g. "I want to poll the GPS" or "I want to set things on the GPS", and I wasn't wanting to do those things. It's obvious that's an incorrect understanding once you know it is, but when I'm looking at an unfamiliar library and experimenting to see if it even meets my needs, I'm just going to look for what does and doesn't look relevant to that evaluation. And at a glance, the mode changes weren't obviously relevant to my use case.

(I don't know how one might fix this. I suspect the new exception will help, though!)

This does raise one question, though: One of the things I'm planning on doing is a protocol-aware proxy (to allow multiple bits of software to get at the same set of GPSs over the network). Half of that is easy -- read the results of things coming off the GPS (in READ mode) and just write them out to the clients. The other way around seems trickier, though -- a client could easily send either a SET or a poll command, but it seems like the library would only parse one or the other, and throw an exception on the other. Any suggestions on how best to deal with that?

semuadmin commented 2 years ago

I think I'd need to understand your intentions a little better to be able to answer your final question. Clearly you don't NEED to parse command (SET) or query (POLL) inputs to a UBX receiver to get them to work - the receiver just needs the binary data. Whatever client is creating the SET or POLL message in the first place will presumably know which type it is. So I'm unclear under what circumstances you would need to parse SET or POLL messages without knowing what type you were parsing?

I'm going to merge https://github.com/semuconsulting/pyubx2/pull/74, which will close this issue, but happy to continue the correspondence in this channel if you have further queries.