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

Add parsing support for 8:367:23, 8:367:24, 8:367:25, 8:367:33. #219

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

andyvan-trabus
Copy link
Contributor

This adds support for parsing the USCG 23, 24, 25, and 33 messages. I've tested this with live messages, and have added one each of those live messages as a test case.

Copy link
Owner

@schwehr schwehr left a comment

Choose a reason for hiding this comment

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

Please pick the simplest of the messages and start with just that one message.

Also, there needs to be at least one test in https://github.com/schwehr/libais/blob/master/src/test/ais8_367_test.cc

@@ -626,8 +626,104 @@ Ais8_1_31::Ais8_1_31(const char *nmea_payload, const size_t pad)
spare2 = bits.ToUnsignedInt(350, 10);

assert(bits.GetRemaining() == 0);
}

// SSW FI23 Satellite Ship Weather 1-Slot Version
Copy link
Owner

Choose a reason for hiding this comment

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

8_367_* messages should go in ais8_367.cpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: the code for 8:367:33 is quite large, is it okay to have that split out into its own separate file?

src/libais/ais.h Outdated
@@ -2433,6 +2433,335 @@ class Ais27 : public AisMsg {
};
ostream& operator<< (ostream &o, const Ais27 &msg);


Copy link
Owner

Choose a reason for hiding this comment

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

These messages are out of order. They should go above right after Ais8_367_22.

src/libais/ais.h Outdated

//
// SSW Satellite Ship Weather 1-Slot Version
class Ais8_367_24 : public Ais8 {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's start off with a PR that is just Ais8_367_23 and do each message in tern so that we can carefully go through each message on its own.

@@ -139,6 +139,8 @@ unique_ptr<AisMsg> CreateAisMsg8(const string &body, const int fill_bits) {
switch (msg.fi) {
case 22:
return MakeUnique<libais::Ais8_367_22>(body.c_str(), fill_bits);
case 33:
Copy link
Owner

Choose a reason for hiding this comment

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

This PR has 8_367_23, 8_367_24, 25, and 33 all in one. The others are missing here. But let's back up and just do one message to start. Maybe pick the simplest message.

@schwehr
Copy link
Owner

schwehr commented Sep 8, 2021

Please also make sure to link to the message specification in both the PR text and the code.

@andyvan-trabus
Copy link
Contributor Author

Kurt,
Since you only commented on this PR and not the cut-down version, I've removed the other messages from THIS PR, and I believe I have addressed your other comments.

Set pressure to Py_None if not a real pressure value.
@schwehr
Copy link
Owner

schwehr commented Sep 20, 2021

Using this PR is fine. I've merged a couple cleanup PRs. These are fixes to use unique_ptr and moving to always using std::

Also, I'd like to try to get away from putting the initializing in constructor switching to the default member initializer style (see data_members). I will be trying to do that for the whole of the library. So if you are up for doing that on this CL that would be very helpful. e.g.

class AisMsg {
 public:
  int message_id = 0;  // <-- default member initializer`
  int repeat_indicator = 0;
  int mmsi = 0;

  // TODO(schwehr): make status private and have accessors.
  bool had_error() const {  return status != AIS_OK;  }
  AIS_STATUS get_error() const { return status; }

  virtual ~AisMsg() {}

 protected:
  AIS_STATUS status = AIS_UNINITIALIZED;  // AIS_OK or error code
  int num_chars = 0;  // Number of characters in the nmea_payload.
  size_t num_bits = 0;  // Number of bits in the nmea_payload.
  AisBitset bits = {};  // Bitset that was constructed out of the nmea_payload.

  AisMsg() : status(AIS_UNINITIALIZED), num_chars(0), num_bits(0), bits() {}
  AisMsg(const char *nmea_payload, const size_t pad);

  // Returns true if the msg is in a good state "so far", i.e. either AIS_OK or
  // AIS_UNINITIALIZED.
  bool CheckStatus() const;
};

and

AisMsg::AisMsg(const char *nmea_payload, const size_t pad) {  // <-- Drop the initializer list
  assert(nmea_payload);
  assert(pad < 6);

  const AIS_STATUS r = bits.ParseNmeaPayload(nmea_payload, pad);
  if (r != AIS_OK) {
    status = r;
    return;
  }
  num_bits = bits.num_bits();
  num_chars = bits.num_chars();

  if (num_bits < 38) {
    status = AIS_ERR_BAD_BIT_COUNT;
    return;
  }

  message_id = bits.ToUnsignedInt(0, 6);
  repeat_indicator = bits.ToUnsignedInt(6, 2);
  mmsi = bits.ToUnsignedInt(8, 30);
}

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