High level C++ API proposal

Question

High level C++ API proposal

Closed this issue 2 years ago · 10 comments

I'm wondering why we have no High-level API for cppTango (like PyTango has). I think that simplicity is one of the reasons behind PyTango's popularity (more stars, more forks, more contributors). Usually people do not care about class classes, factories, writing boilerplate code or using extra tools (Pogo) to generate such code.

I did some research and checked what could be done to improve the situation. Turns out we can provide a simpler API (on top of the current one) with just one extra header file (which we can even backport to 9.3.3 or let people download manually and #include in their projects).

Below is an example how the new API might look like. I did some quick and dirty PoC implementation to see if it is feasible. Please let me know if you see any value in adding this to Tango.

// For each attribute/command/pipe we need unique struct (like @attribute in pytango)
struct MyScalar : Tango::hl::Attr
{
    using Name = TANGO_FIXED_STRING("MyScalar");
    static constexpr auto Type = Tango::DEV_DOUBLE;
    static constexpr auto Mode = Tango::READ_WRITE;

    // All below fields are optional.
    static constexpr auto PollingPeriod = 1000; // default: no polling
    static constexpr auto DisplayLevel = Tango::OPERATOR; // default: OPERATOR
    static constexpr bool Memorized = true; // default: false
    static constexpr bool MemorizedInit = true; // default: false
    struct Properties // default: empty
    {
        // TBD if we want to use proper types here or just use const char*.
        static constexpr auto Label = "My Scalar";
        static constexpr auto Unit = "U";
        static constexpr auto MaxValue = 10.0;
    };
};

// Alternatively, we may use more terse syntax.
using MyScalar2 = Tango::hl::Attribute<TANGO_FIXED_STRING("MyScalar"), Tango::DEV_DOUBLE, Tango::READ_WRITE>;

// Define device as usual, no need to inherit from special class.
class MyDevice : public TANGO_BASE_CLASS
{
public:
    // Macros that just call init_device and delete_device.
    TANGO_DEFAULT_CONSTRUCTOR(MyDevice, TANGO_BASE_CLASS)
    TANGO_DEFAULT_DESTRUCTOR(MyDevice)

    // Macro that provides declaration of read/write callbacks.
    TANGO_DEVICE_IMPLEMENTATION

    virtual void init_device() override
    {
    }
};

// Simple read callback.
template <>
Tango::DevDouble MyDevice::read_attribute<MyScalar>()
{
    return 3.14;
}

// Complex read callback. Defining both variants for the same attribute
// results in compilation error. Defining no callback for readable attribute
// also results in compilation error.
template <>
void MyDevice::read_attribute<MyScalar>(Tango::Attribute& att)
{
    double value = 3.14;
    att.set_value(&value);
}

// Simple write callback.
template <>
void MyDevice::write_attribute<MyScalar>(Tango::DevDouble value)
{
}

// Complex write_attribute callback is also supported.

// For forwarded attributes, we have a special constructor that takes only
// name and root attribute property. Defining read/write callbacks for such
// attributes results in compilation error.

// Commands and pipes are supported in a way similar to attributes.
using MyCommand = ...;

template <>
Tango::DevLong MyDevice::run_command<MyCommand>(Tango::DevDouble argin)
{
}

// Simple API for spectrum and image - TBD. But I think we need new types like
// Spectrum<T> (kind of std::vector<T> but with the possibility to release the
// ownership of the stored buffer) and SpectrumView<T> (std::span<T>).

using HlTestClass = Tango::hl::HighLevelDeviceClass<TANGO_FIXED_STRING("HlTest"), MyDevice,
    MyScalar,
    MyCommand
    // Optionally more attributes, commands and pipes
    >;

int main(int argc,char *argv[])
{
    Tango::hl::run_server<
        HlTestClass
        // Optionally more classes.
        >(argc, argv);
    return 0;
}

Answer 1 · 2020-07-06T08:58:39.000Z

I'd say that any effort towards facilitating developers in integrating their hardware is a good idea to, at least, explore it!

Answer 2 · 2020-07-06T14:17:56.000Z

Thanks Guifre.

I also talked with Reynald today who was concerned by the use of macros in the proposal above. Some of them can be avoided, some may be written by hand by the users, but to remove others, we'd need to think about a different solution.

Passing character string as template parameter (Attribute<"Name">).
I think this is natively supported in C++17 or C++20 (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0732r2.pdf)
To support all compilers, we need to pass the string in user-defined literal result type (operator "" _), so the macro is:
```
#define TANGO_FIXED_STRING(s) decltype(s ## _tango_fixedstr)
```
To type Attribute<TANGO_FIXED_STRING("Name")> instead of Attribute<decltype("Name"_tango_fixedstr)> (which is a bit harder to read IMO).
Constructor / destructor macros. It would be nice to not force the user to write the constructor that just calls init_device. But base class cannot provide such constructor as virtual calls are not working in this context. So we either provide a macro or ask users to implement constructor by hand.
```
#define TANGO_DEFAULT_CONSTRUCTOR(CLASS, BASE_CLASS) \
 CLASS(::Tango::DeviceClass* device_class, const char* name) \
     : BASE_CLASS(device_class, name) \
 { \
     init_device(); \
 }

#define TANGO_DEFAULT_DESTRUCTOR(CLASS) \
 ~CLASS() \
 { \
     delete_device(); \
 }
```

If we want to wire read/write methods to attributes using template specialization (e.g. by defining methods like read_attribute<Current>() or write_attribute<Voltage>()) we must declare the base templates somewhere. And it cannot be declared in the base class because this would require specialization to be bound to the base class as well. So alternative here is to ask user to write these declarations (which is a no-no IMO because they are quite complex and we may want to change them later) or find another way of wiring methods to attributes.

#define TANGO_DEVICE_IMPLEMENTATION\
 template <typename Tag> \
 typename ::Tango::hl::__detail::TypeMapping<Tag::Type>::Type read_attribute() = delete; \
 \
 template <typename Tag> \
 void read_attribute(Tango::Attribute&) = delete; \
 \
 template <typename Tag> \
 void write_attribute(typename ::Tango::hl::__detail::TypeMapping<Tag::Type>::Type); \
 \
 template <typename Tag> \
 bool is_attribute_alowed(Tango::AttReqType) { return true; } \
 \
 template <typename Tag> \
 Tango::DevLong run_command(Tango::DevDouble) = delete;

For sure we may come with different solutions to the last problem. We may e.g. pass callback pointers when defining the Attribute:

Tango::hl::HighLevelClass<
    TANGO_FIXED_STRING("ClassName"),
    MyDevice,
    Tango::hl::Attribute<
        TANGO_FIXED_STRING("MyAttr"),
        Tango::DEV_DOUBLE,
        Tango::READ_WRITE,
        &MyDevice::read_my_attr,
        &MyDevice::write_my_attr>,
    Tango::hl::Attribute< ... >,
    // ...
>;

Answer 3 · 2020-07-07T09:33:50.000Z

For 2.: Why not get rid of init_device() and delete_device() completely for this API? The user should just implement the constructor and destructor to do their jobs, if needed at all.

Answer 4 · 2020-07-07T09:43:18.000Z

For a totally different take on the idea: We could generate something much nicer from POGO, by letting it generate a pure-virtual interface like MyAbstractDevice with simple methods like virtual double read_my_attr() const = 0 and virtual void write_my_attr(double value). The user would then implement this class and pass a factory method to a high-level function that does everything, like run_server([] {return std::make_unique<MyDevice>();}).
Not saying that's in any way better than @mliszcz idea, just something that I had in the back of my mind for a while. It's basically like the old POGO way, but instead of protected regions, it uses virtual function implementations / language features. Can also be done via CRTP, if people really dislike the virtual, although that should usually get optimized out / devirtualized.

Answer 5 · 2020-07-07T11:39:22.000Z

Hi @ltjax, thanks for your comment.

First, let me clarify that my reason for triggering this discussion is to have a simple API that would allow to create device server without using Pogo (no clicking) and that would fit in one file (so can easily be shared e.g. in github issue to reproduce some problem).

For 2.: Why not get rid of init_device() and delete_device() completely for this API? The user should just implement the constructor and destructor to do their jobs, if needed at all.

You must implement implement init_device because it is pure virtual in DeviceImpl. And the usual expectation is that init_device is called also as part of device creation, not only during Init command (if I recall, this is a requirement we described in some RFC). I just wanted to hide this fact from the user. If one wants to provide a custom constructor, of course it should be possible, but then he should explicitly call init_device.

Regarding your idea with improving Pogo-generated code, I do not yet see what we gain over what we currently have. Protected regions are here to support scenarios like e.g. attribute renaming (so only function name changes, and implementation stays the same). With your proposal this will no longer work. If you just want to get rid of protected regions (and accept lack of some features that require protected regions), maybe we should simply consider to not generate protected section comments.

IMO the proper way of implementing code generator tool for Tango is to re-write Pogo backend so that it can understand C++ (like libclang) and operates on AST instead of on operating on a text file. Such generator could e.g. add or rename methods without any comments or protected regions being stored in the source file.

Answer 6 · 2020-07-07T12:09:15.000Z

First, let me clarify that my reason for triggering this discussion is to have a simple API that would allow to create device server without using Pogo (no clicking) and that would fit in one file (so can easily be shared e.g. in github issue to reproduce some problem).

A noble goal, indeed. There has to be some way to expose the device metadata. The most C++ish way (before we get any better language level reflection) is do build some kind of DSL in C++ for that, like boost.python or luabind.
Using libclang would be cool, yea, but it's not much different from just running pogo generation on some kind of interface definition file. And even with libclang you probably have to add additional metadata that cannot be extracted from the pure C++. A headless mode for "no clicking" would be great for either. Both ways come with with the drawbacks that additional tools always have, like more complicated build-setup etc., while a pure C++ way will naturally have some duplication and be a little more cumbersome.

For 2.: Why not get rid of init_device() and delete_device() completely for this API? The user should just implement the constructor and destructor to do their jobs, if needed at all.

You must implement implement init_device because it is pure virtual in DeviceImpl.

Then do not inherit from that and use an adaptor that redirects init_device/delete_device to c'tor/d'tor respectively. This kind of lifecycle is much easier to grasp, and new language features like c'tor delegation make it unnecessary to have two-phase construction.

Regarding your idea with improving Pogo-generated code, I do not yet see what we gain over what we currently have.

A clear separation of concerns. All the user code would be in one module (header/cpp pair), without any of the generated code. All the metadata would be in the xmi file. It's in 3 files, but it's pretty minimal for what you'd actually write.

You can still use different names for attributes etc. if the code generator knows how to match it to a c++ function pair. I'd just not make that the default. Using different names for the same thing is usually just a cause for unnecessary confusion.

Answer 7 · 2020-07-07T13:06:18.000Z

While I agree with all you've said, I think that this would require breaking ABI (and maybe API). Users will no longer inherit from DeviceImpl, right? I was thinking rather about an API which we can add optionally on top of what we currently have, which would work also with 9.3 and older Tango versions.

Answer 8 · 2020-07-07T13:23:14.000Z

Yea, I was thinking the same thing, with a high-level API on top. In my mind, you'd always compile the high-level stuff with your application. ABI compatbility is handled completely in that layer, not in the user application. So in practice, the adaptor would inherit DeviceImpl.

Answer 9 · 2020-11-05T12:57:52.000Z

Okay, so I'm putting my money where my mouth is. We've started on a new code generator, and it's starting to be useful, though we have yet to test it in a production setting.

You start with a definition file in toml. It is meant to be human-readable and editable and be checked into your repository:

name = "cool_camera"

[[device_properties]]
name = "index"
type = "long"

[[device_properties]]
name = "address"
type = "string"

[[attributes]]
name = "binning"
type = "long"
access = ["read", "write"]

[[attributes]]
name = "exposure_time"
type = "float"

[[commands]]
name = "record"
return_type = "void"
parameter_type = "void"

[[commands]]
name = "square"
return_type = "long"
parameter_type = "long"

[[commands]]
name = "report"
return_type = "float"
parameter_type = "void"

[[commands]]
name = "act"
return_type = "void"
parameter_type = "float"

[[commands]]
name = "talk"
return_type = "string"
parameter_type = "string"

And run this through the hula exe. This is supposed to happen in a build step. This generates a single header/implementation pair. The implementation just needs to be compiled into your binary, the header you include and then you implement the device like this:

#include "hula_cool_camera.hpp"
#include <iostream>

class cool_camera : public cool_camera_base
{
public:
  cool_camera(cool_camera_properties const& p)
  : address_(p.address)
  {
  }

  long read_binning() override
  {
    return binning_;
  }

  void write_binning(long rhs) override
  {
    binning_ = rhs;
  }

  float read_exposure_time() override
  {
    return exposure_time_;
  }

  void record() override
  {
    std::cout << "Recording!" << std::endl;
  }

  long square(long rhs) override
  {
    return rhs*rhs;
  }
  
  float report() override
  {
    return 1234.5f;
  }

  void act(float rhs) override
  {
    exposure_time_ = rhs;
  }

  std::string talk(std::string const& rhs) override
  {
    return "You said " + rhs + " @ " + address_;
  }

private:
  float exposure_time_ = 0.05f;
  long binning_ = 42;
  std::string address_;
};

int main(int argc, char* argv[])
{
  return cool_camera::register_and_run(argc, argv, [] (cool_camera_properties const& p) {
    return std::make_unique<cool_camera>(p); 
  });
}

That's it, a working device server! It's pretty far from being full-featured, but usable. And I'm pretty happy how this is turning out.
If you're interested, you can follow me hacking along on this thing (or try it out): https://github.com/softwareschneiderei/hula

Our goals were:

Standard C++ to implement the device server, which deduplicates a lot of our "common" marshalling code for data and exceptions
Separation of generated code from the user-edited code
Text-editable "definition" file
Insulation from CORBA and Tango in the user code
Easy integration into build steps

What do you think?

Answer 10 · 2020-11-05T13:19:52.000Z

@ltjax, that sounds like a great idea. We have to try it and see whether we could add easily more useful features which are currently present in pogo because there are many features in POGO.
But I like the idea.