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

Different payload after decode/encode roundtrip #86

Closed
pgorczak opened this issue Oct 11, 2022 · 8 comments
Closed

Different payload after decode/encode roundtrip #86

pgorczak opened this issue Oct 11, 2022 · 8 comments

Comments

@pgorczak
Copy link

pgorczak commented Oct 11, 2022

First of all thanks for maintaining this fantastic project!

I am currently trying to generate some mock NMEA data by modifying fields in an existing data stream. That requires a decode/encode roundtrip (followed by another decode later on).

I noticed that the NMEA sentences change, even when not modifying the AIS data. After decoding another time, the AIS data is also different.

Example

example.py

import sys

from pyais.decode import _assemble_messages
import pyais.encode
from pyais.stream import BinaryIOStream

# Read NMEA sentences from stdin
for nmea in BinaryIOStream(sys.stdin.buffer):
    print(nmea)
    assert nmea.is_valid
    # decode AIS message
    m = nmea.decode()
    print(m)
    # encode and decode AIS -> NMEA -> AIS three more times
    for _ in range(3):
        sentences = pyais.encode.encode_msg(
            m, talker_id='AI'+nmea.type, radio_channel=nmea.channel)
        sentences = [s.encode() for s in sentences]
        nmea = _assemble_messages(*sentences)
        print(nmea)
        assert nmea.is_valid
        m = nmea.decode()
        print(m)
    print('---')

After feeding this script with the file from this repo's test folder, many sentences and messages don't match up e.g. the first set of output:

python example.py < ais_test_messages

b'!AIVDM,1,1,,A,13HOI:0P0000VOHLCnHQKwvL05Ip,0*23\n'
MessageType1(msg_type=1, repeat=0, mmsi=227006760, status=<NavigationStatus.UnderWayUsingEngine: 0>, turn=None, speed=0.0, accuracy=False, lon=0.13138, lat=49.475577, course=36.7, heading=511, second=14, maneuver=0, spare_1=b'\x00', raim=False, radio=22136)
b'!AIVDM,1,1,,A,13HOI:00002IuQi?IR5gwqh0EWP,2*0E'
MessageType1(msg_type=1, repeat=0, mmsi=227006760, status=<NavigationStatus.UnderWayUsingEngine: 0>, turn=0.0, speed=0.0, accuracy=False, lon=33.633373, lat=-84.936497, course=409.5, heading=312, second=0, maneuver=0, spare_1=b'\xa0', raim=False, radio=1656)
b'!AIVDM,1,1,,A,13HOI:00002IuQg?IR5gwqh0D0Ip,0*0D'
MessageType1(msg_type=1, repeat=0, mmsi=227006760, status=<NavigationStatus.UnderWayUsingEngine: 0>, turn=0.0, speed=0.0, accuracy=False, lon=33.633372, lat=-84.936497, course=409.5, heading=312, second=0, maneuver=0, spare_1=b'\xa0', raim=False, radio=1656)
b'!AIVDM,1,1,,A,13HOI:00002IuQg?IR5gwqh0D0Ip,0*0D'
MessageType1(msg_type=1, repeat=0, mmsi=227006760, status=<NavigationStatus.UnderWayUsingEngine: 0>, turn=0.0, speed=0.0, accuracy=False, lon=33.633372, lat=-84.936497, course=409.5, heading=312, second=0, maneuver=0, spare_1=b'\xa0', raim=False, radio=1656)

A second roundtrip changes the payload again, although fewer bits seem to change (sometimes all AIS fields stay the same). After the second roundtrip, the data stays the same and further encodes and decodes result in the same NMEA sentence and AIS information.

@pgorczak
Copy link
Author

pgorczak commented Oct 11, 2022

I got a bit closer to the issue: the logic in messages.Payload.to_bitarray skips None fields so they don't show up in the serialized payload. Here, None is interpreted as "skip this data while serializing".

It is used in another way in the messages.from_turn function, which is responsible for converting the AIS representation for rate of turn to degrees/s. The AIS representation allows for special values meaning "no information available", which that function converts to None.

This means that any AIS position report with a turn field containing the "no info" special values gets serialized to a payload where the 8 Bits encoding turn are missing. Looking at the payload length before and after the round trip, it shrinks from 168 Bit to 160 Bit.

@pgorczak
Copy link
Author

The messages.Payload.from_bitarray function did not indicate a problem, but this might be intended. As far as I can tell, it

  • assigns None to fields that start past the length of the input payload,
  • uses as many bits as there are left for a field that starts within the input payload but goes past its end.

In my example above, the final radio field expects 19 Bits but gets only 11, which are cast to an integer without raising an error.

@pgorczak
Copy link
Author

pgorczak commented Oct 11, 2022

Here's another snippet highlighting what's happening

from pyais.messages import NMEAMessage

nmea = NMEAMessage(b'!AIVDM,1,1,,A,13HOI:0P0000VOHLCnHQKwvL05Ip,0*23')
ais = nmea.decode()
print(f'AIS payload 1: {nmea.bit_array.to01()}')
bits = ais.to_bitarray().to01()
print(f'AIS payload 2: {bits[:42]}{" "*8}{bits[42:]}')

Output

AIS payload 1: 000001000011011000011111011001001010000000100000000000000000000000000000100110011111011000011100010011110110011000100001011011111111111110011100000000000101011001111000
AIS payload 2: 000001000011011000011111011001001010000000        0000000000000000000000100110011111011000011100010011110110011000100001011011111111111110011100000000000101011001111000

The input message contains a 128 decimal in the turn field (no info) and those 8 bits get snipped out of the re-serialized data.

@pgorczak
Copy link
Author

pgorczak commented Oct 11, 2022

There's another implication for Message 21's final name_ext field. Looks like it can be between 0 and 88 Bits long. The current implicit conversion of missing Bits -> None and None -> no Bits works but we might want to keep it in mind when fixing this issue.

@M0r13n
Copy link
Owner

M0r13n commented Oct 12, 2022

@pgorczak

Thank you for your investigation. I can reproduce your examples. And I agree that the encoding and decoding of turn values should be consistent. Actually, there was a bug in there. If the value for turn was any of 127,-127,-128 the value was set to None. I changed this behavior slightly, by adding an IntEnum TurnRate that is returned instead:

class TurnRate(IntEnum):
    # Source: https://gpsd.gitlab.io/gpsd/AIVDM.html#_types_1_2_and_3_position_report_class_a
    # turning right at more than 5deg/30s (No TI available)
    NO_TI_RIGHT = 127
    # turning left at more than 5deg/30s (No TI available)
    NO_TI_LEFT = -127
    # 80 hex) indicates no turn information available (default)
    NO_TI_DEFAULT = -128

This has two benefits:

  • the decoding/encoding functions behave consistently
  • the original integer value remains accessible. Therefore, as a user, I can differentiate between:
    • turning right at more than 5deg/30s (No TI available)
    • turning left at more than 5deg/30s (No TI available)
    • indicates no turn information available (default)

By doing so, your example no longer produces different payloads. In your three way roundtrip example the last three parts are now always identical. But the first message might still differ from the last three messages. In my observation, however, this was due to rounding errors for the lat, lon fields. E.g. 18.321188 vs 18.321187.

I encourage you to try my changes on this PR. Does it resolve your issue?

@pgorczak
Copy link
Author

Thanks, that definitely fixes it for me! I like the idea of encoding the special values as enums.

Just one addition, unrelated to this issue: when I inspect MessageX instances, the special turn values have been converted to floats. I thinks that might be due to the attrs library, since the bit_field the to_turn function acts as a converter on is declared as a float attribute.

@pgorczak pgorczak reopened this Oct 13, 2022
@M0r13n
Copy link
Owner

M0r13n commented Oct 13, 2022

I inspect MessageX instances, the special turn values have been converted to floats

This isn't really an issue. As a user, I don't know what type of value I might get for turn in advance. Therefore, I always need to check for special cases like that:

assert decoded.turn == TurnRate.NO_TI_DEFAULT

and this is True if the value is both -128 or -128.0.

@M0r13n
Copy link
Owner

M0r13n commented Oct 13, 2022

This fix is released in version 2.2.2

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