semuconsulting / pyspartn

Python library for parsing SPARTN protocol messages.
BSD 3-Clause "New" or "Revised" License
7 stars 3 forks source link

Fix different constellation timebase resolution by trying 12h prior, during, and after given basedate #21

Closed jonathanmuller closed 7 months ago

jonathanmuller commented 7 months ago

pyspartn Pull Request Template

Description

Different constellations are using different time reference : GPS = GAL = QZSS = UTC -18s BEI = UTC -14 GLO = UTC + 3h (exactly 3h, 0 seconds)

Because of that, passing a basedate to convert a 16 bits timetag to 32 bits unambiguous can be either

In order to account for those time difference not being in sync, 3 unambitious solutions are calculated :

By doing so, basedate can be up to 3h inaccurate (as GLONASS is +3h ahead) and still allow for a correct parsing

Fixes #17

Testing

Please test all changes, however trivial, against the supplied pytest suite tests/test_*.py. Please describe any test cases you have amended or added to this suite to maintain >= 85% code coverage.

Checklist:

semuadmin commented 7 months ago

Thanks for this. I'm probably being a bit slow here but I'm not entirely clear what specific scenario this fixes (over and above the UTC/leap second adjustments already made in v0.3.2-beta via TIMETAGSHIFT)?

Can you give an explicit example of a datastream decryption that would fail with the current code but succeed with this PR? Ideally I'd like to see a further test in test_stream.py which exercises this specific scenario.

FYI the next release (RC 0.3.3-beta) will use datetime.now(UTC) rather than datetime.now() as the default, but that obviously only makes an hours difference.

semuadmin commented 7 months ago

@jonathanmuller @JonathanMullerGeosatis

I've added a couple of test cases to confirm that these changes do cater for "half-day rollover" scenarios. Your changes will go into https://github.com/semuconsulting/pyspartn/pull/22, hopefully in the next week or so. Thanks again for the contribution.

jonathanmuller commented 7 months ago

Sure, let me give you more details :

First, the intend of this PR has less to do with stream decryption than with single message decryption, which is the main feature I'm using, that's probably why it makes less (bit not none) sense in a stream context.

Currently, the following message fail to be parsed using the parse function :

>>> msg =bytes.fromhex("73000d6d1126705c17483be529d9707b585195e21e5e4bfc3aeeda1e56c11f3d196dcfeed54110")
>>> SPARTNReader.parse(msg, decode=True, key="930d847b779b126863c8b3b2766ae7cc", basedate=datetime.datetime(2024, 4, 25, 11, 37, 0))
...
pyspartn.exceptions.SPARTNMessageError: Attribute size 1 exceeds remaining payload length 0

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/jonathan/Desktop/cloud_locate/custom/pyspartn/src/pyspartn/spartnreader.py", line 293, in parse
    return SPARTNMessage(
           ^^^^^^^^^^^^^^
  File "/home/jonathan/Desktop/cloud_locate/custom/pyspartn/src/pyspartn/spartnmessage.py", line 102, in __init__
    self._do_attributes()
  File "/home/jonathan/Desktop/cloud_locate/custom/pyspartn/src/pyspartn/spartnmessage.py", line 197, in _do_attributes
    raise SPARTNTypeError(
pyspartn.exceptions.SPARTNTypeError: Error processing attribute 'groupSat' in message type SPARTN-1X-OCB-GLO

However, the basedate it correct (and provided as UTC+0 timezone)

Nevertheless, in a stream context, here are a scenario that would fail without this PR :

I could not record such a stream because it needs such a precise timming, but I'll see what I can do to have a tets case that covers it

EDIT : @semuadmin I was so slow to type this comment that you had time to comment in between, so I was obviously responding to your earlier comment