Skip to content

Conversation

@jbangelo
Copy link
Contributor

This PR adds a few lightweight C++ classes that wrap around the existing libsbp C interface. The main item of interest here is the MessageHandler. This class automates the infrastructure for registering a callback to have a member function called upon receipt of a sbp message.

Here is an example of how it would be used

class ECEFHandler : private sbp::MessageHandler<msg_gps_time_t, msg_pos_ecef_t, msg_pos_ecef_cov_t> {
 public:
  ECEFHandler(sbp::State *state) : private sbp::MessageHandler<msg_gps_time_t, msg_pos_ecef_t, msg_pos_ecef_cov_t>(state)
  {
    /* Do other constructor stuff */
  }

 private:
  void handle_sbp_msg(uint16_t sender_id, uint8_t message_length, const msg_gps_time_t& msg)
  {
    /* handle GPS time message */
  }

  void handle_sbp_msg(uint16_t sender_id, uint8_t message_length, const msg_pos_ecef_t& msg)
  {
    /* handle pos ECEF message */
  }

  void handle_sbp_msg(uint16_t sender_id, uint8_t message_length, const msg_pos_ecef_cov_t& msg)
  {
    /* handle pos ecef cov message */
  }
};

The instantiation and registration of the sbp_msg_callbacks_node_t objects are all taken care of automatically by the sbp::MessageHandler constructor, and are unregistered in the destructor. The derived class is able to publicly or privately inherit from sbp::MessageHandler, and the handle_sbp_msg member functions can be public or private. Each handle_sbp_msg is a virtual member function so additional levels of inheritance can be employed, though we do incur the overhead of the vtable lookup at run time. The mapping of message struct to message ID is achieved via a set of type traits that are automatically generated from the specification YAML. Currently only the message ID is included in the message type traits, but additional information could be added in the future if deemed useful.

The other classes are thin wrappers around the existing C structs, and probably don't need much explanation.

@jbangelo jbangelo force-pushed the jbangelo/STAR-869-add-c++-support branch from c767d02 to 9c79006 Compare October 21, 2019 20:51
Copy link
Contributor

@kmdade kmdade left a comment

Choose a reason for hiding this comment

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

Amazing.

So do we draw lots for who goes back and replaces all the existing SBP one-offs? ;)


public:

explicit MessageHandler(State *state) : details::CallbackInterface<MsgTypes...>(), state_(*state), callback_nodes_() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason the ctor argument isn't State&?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The State needs to be mutable, and our C++ guidelines say any non-const parameter should be passed as a pointer.

Copy link
Contributor

@RReichert RReichert left a comment

Choose a reason for hiding this comment

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

Brilliant work, I see no major fault stopping me from approving.

virtual s32 write(const u8 *buffer, u32 buffer_length) = 0;
};

class State {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering it the class name could be better. The class acts as a SBP processor, where you have a state internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name came from the sbp_state_t which it wraps into a C++ object. I'm open to alternate suggestions though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually call it SBP since this class is how I think of SBP but it might be inappropriate. I'll leave it up to you, my comment was merely a suggestion

@@ -0,0 +1,1026 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a CI step that validates that the committed message file is identical to that generated from the generator tool?

I'm assuming that this file was generated by the python/template file that was added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's a double check in the CI. I think it's assumed as part of the manual review that only the relevant changes are made to the auto generated files.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably say it would be worth adding that in so there is no misalignment between one source and its C++ representation

* the `context` parameter.
*
* @tparam MsgT The message type to decode the payload as
* @tparam ClassT The class type to call the function on
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to call the same function on multiple class types? I find this is a common pattern for me (i.e do the same thing for any ephemeris message received)

Copy link
Contributor Author

@jbangelo jbangelo Oct 22, 2019

Choose a reason for hiding this comment

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

Not with how this PR currently stands. The problem with that is that all of the message types are distinct and there's little to no information relating the content of similar messages. The closest thing we could get is defining a base class that handles each of the distinct message types and converts them into a common type and then calls another member function that works on the common type.

Something like this:

class CommonEphemerisHandler : public sbp::MessageHandler<EphemerisMessage1, EphemerisMessage2> {
  public:
    struct CommonEphemerisType { ... };

    virtual void handle_common_ephemeris(const CommonEphemerisType& eph) = 0;

    void handle_sbp_msg(uint16_t sender_id, uint8_t message_length, const EphemerisMessage1& msg) {
      CommonEphemerisType eph = convertEph1(msg);
      handle_common_ephemeris(eph);
    }

    void handle_sbp_msg(uint16_t sender_id, uint8_t message_length, const EphemerisMessage2& msg) {
      CommonEphemerisType eph = convertEph2(msg);
      handle_common_ephemeris(eph);
    }
};

class CustomEphemerisHandler : public CommonEphemerisHandler {
  public:
    void handle_common_ephemeris(const CommonEphemerisType& eph) { ... }
};

class OtherEphemerisHandler : public CommonEphemerisHandler {
  public:
    void handle_common_ephemeris(const CommonEphemerisType& eph) { ... }
};

@jbangelo jbangelo force-pushed the jbangelo/STAR-869-add-c++-support branch 4 times, most recently from 3473007 to c175d1c Compare October 24, 2019 19:19
Added steps to autogenerate SBP message traits for the C++ headers

Regenerated the C files
@jbangelo jbangelo force-pushed the jbangelo/STAR-869-add-c++-support branch from d2bc31c to 73e7159 Compare October 24, 2019 20:17
@jbangelo jbangelo merged commit cb83de6 into master Oct 24, 2019
@jbangelo jbangelo deleted the jbangelo/STAR-869-add-c++-support branch October 24, 2019 21:14
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.

5 participants