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

Improve the C++ API #12

Open
paravoid opened this issue Oct 23, 2024 · 0 comments
Open

Improve the C++ API #12

paravoid opened this issue Oct 23, 2024 · 0 comments

Comments

@paravoid
Copy link

I'm building my own hardware abstraction subclasses (like RTSPulseOutput etc.), and trying to use the C++ API.

IMHO the API is kinda awkward at the moment. Looking at e.g. arduino/RTSPulseOutput_GPIO.cpp:

  • Downstream C++ users are exposed to the underlying C-style struct.
  • Overrides to the standard functions need to happen through static functions (or lambdas), rather than a subclass overloading the methods.
  • You need to create a constructor yourself, to set all the function pointers. If these ever change in the OpenRTS code, you'd need to change your code.
  • You cannot easily hold your own state, and use "this" (like e.g. "this->data_pin") from your own methods. arduino/RTSPulseSource_GPIO.cpp addresses this by storing the instance reference to user_data_ptr, and casting back and forth but it can get a little bit repetitive.
  • There are no compile-time checks that you've set all callbacks correctly, like a pure virtual function would ensure.

What I've been experimenting with is a more C++-like interface, like so:

class RTSPulseOutputAdapter : public RTSPulseOutput {
 public:
  RTSPulseOutputAdapter() {
    rts_pulse_output::enable = &RTSPulseOutputAdapter::enable_impl;
    rts_pulse_output::disable = &RTSPulseOutputAdapter::disable_impl;
    rts_pulse_output::send_pulse = &RTSPulseOutputAdapter::send_pulse_impl;
  };

  virtual void enable() = 0;
  virtual void disable() = 0;
  virtual void sendPulse(bool state, uint32_t micros) = 0;

 protected:
  static void enable_impl(struct rts_pulse_output *pulse_output) {
    static_cast<RTSPulseOutputAdapter*>(pulse_output)->enable();
  }
  static void disable_impl(struct rts_pulse_output *pulse_output) {
    static_cast<RTSPulseOutputAdapter*>(pulse_output)->disable();
  }
  static void send_pulse_impl(struct rts_pulse_output *pulse_output, bool state, uint32_t micros) {
    static_cast<RTSPulseOutputAdapter*>(pulse_output)->sendPulse(state, micros);
  }
};

This can then be used, as an example, like so:

class MyRTSPulseOutput : public RTSPulseOutputAdapter {
 public:
  void enable() override;
  void disable() override;
  void sendPulse(bool state, uint32_t micros) override;

 protected:
   MyPin *pin_{nullptr};
   uint8_t some_other_state;
};

void MyRTSPulseOutput::enable {
  this->pin_->do_stuff_to_enable();
}

void MyRTSPulseOutput::disable {
  this->pin_->do_stuff_to_disable();
}

void MyRTSPulseOutput::sendPulse(bool state, uint32_t micros)  {
  this->pin_->write(state);
}

...which is quite clean and abstracted!

I'm not a proficient C++ coder by any stretch, so there may be smarter/less repetitive ways to do the same thing. But this works and doesn't look too bad! However, I'm effectively rebuilding the C++ API in my own code, which is a lot of extra scaffolding.

My proposal would for RTSPulseOutputAdapter to be folded into RTSPulseOutput and have this be the canonical interface. Same for the other interfaces as well -- I just picked on Output as it's the simplest one.

Thoughts?

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

1 participant