Skip to content

Feature/ais6 235 10#195

Open
pierregarreau wants to merge 6 commits intoschwehr:masterfrom
pierregarreau:feature/ais6_235_10
Open

Feature/ais6 235 10#195
pierregarreau wants to merge 6 commits intoschwehr:masterfrom
pierregarreau:feature/ais6_235_10

Conversation

@pierregarreau
Copy link
Copy Markdown

This PR to solve #194

Comment thread src/libais/ais6.cpp Outdated
seq = bits.ToUnsignedInt(38, 2);
mmsi_dest = bits.ToUnsignedInt(40, 30);
retransmit = !bits[70];
retransmit = static_cast<bool>(bits[70]);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ! not is missing. Also, why is a static_cast<bool> better?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From ais type 6 documentation, it should not be negated !:

70 1 Retransmit flag retransmit b 0 = no retransmit (default) 1 = retransmitted

The decoding of the messages type 6 from the tests:

    {
        'nmea': ['!AIVDM,1,1,,A,63m95T8uBK:0044@00P,2*7A'],
        'result': {
            'id': 6,
            'repeat_indicator': 0,
            'mmsi': 257050000,
            'seq': 2,
            'mmsi_dest': 257060000,
            'retransmit': True,
            'spare': 0,
            'dac': 1,
            'fi': 1,
            'ack_dac': 64,
            'msg_seq': 1,
            'spare2': 0}},
    {
        'nmea': ['!AIVDM,1,1,,B,65@<;:1inW@h0480J0,4*60'],
        'result': {
            'id': 6,
            'repeat_indicator': 0,
            'mmsi': 352521000,
            'seq': 0,
            'mmsi_dest': 477535500,
            'retransmit': True,
            'spare': 0,
            'dac': 1,
            'fi': 2,
            'req_dac': 1,
            'req_fi': 40}}

are not correct, as confirmed by decoding the AIS messages for instance here.

Therefore changed tests that are affected + this line.

@schwehr reverted to not using static_cast<bool>.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use original source documentation for AIS message 6. I certainly could have been wrong, but unless you quote an official standard like ITU M.1371, I am not going to change it. gpsd aivdm docs were written by ESR with feedback from me before ITU M.1371 was freely available. I had a purchased copy; ESR did not.

So, I believe that I set this up as a bool called retransmit based on discussions at RTCM SC121 back in the 2007-2009 time frame.

3.4 Message 6: Addressed binary message; table 54:

Retransmit flag 1 Retransmit flag should be set upon retransmission: 0 = no retransmission =
default; 1 = retransmitted

I took the flag as a should retransmit.

No matter the correct answer, changing it in this pr is out of scope. Please don't do it here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schwehr you are right, topic is out of scope for this PR. Will use original source doc in another issue. 👍

Reverted.

Comment thread src/libais/ais_py.cpp Outdated
// TODO(schwehr): AIS_FI_6_1_30_TEXT.
case AIS_FI_6_1_32_TIDAL_WINDOW: // IMO Circ 289
status = ais6_1_32_append_pydict(nmea_payload, dict, pad);
case AIS_DAC_1_INTERNATIONAL: // IMO.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't reindent this all.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. back to original indenting.

Comment thread test/testutils.py Outdated
def Compare(x, y):
if x == y:
return True
if x in [None, 'nan'] and y in [None, 'nan']:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this? And that says that None and 'nan' are ==. Is that really what we want?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified because testTypeExamples fails. Not ideal. Reverted.

@pierregarreau
Copy link
Copy Markdown
Author

@schwehr would love to close this PR, could have a second look at it, and close ?

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

Successfully merging this pull request may close these issues.

2 participants