Implementation of a RPC helper for Yarp Module.

[francesco-romano]

During the development of a controller we usually come up with the requirement of setting its parameters through the .ini file and later through the RPC port. This task is time consuming and error prone.

One way to automate this process is the paramHelp project

After some discussions with @lornat75 @drdanz @traversaro we think this facility can be improved and integrated in Yarp.

I now write here the summary of the discussion we had today, together with a sketch of the architecture of the library and the issues remained open.

We would like to use thrift to generate automatically getters and setters for the module. In the thrift file we can specify a new statement of this kind (name, and syntax can be changed of course)

property __type__ __name__ (__attribute_list__)

This statement will generate automatically a pair of getter and setter with the following signature:

virtual const __type__& get__Name__();
virtual void set__Name__(const __type__&);
 //using camel case for the method names, with first letter lowercase

The real advantage of the autogeneration is that the two methods are not simply stubs to be implemented, but already implemented methods. I would like to list now the three use cases that guided our design:

• the user does nothing a part from writing the thrift file. All the communication through RPC is handled by the utility. When the user needs to use a parameter, he simply invoke the relative getter and he will use the most recent value for the parameter.
• the user wants to be notified when a value changes so that he can update its behaviour, e.g. some gains in the controller. (This use case is motivated by the fact that we think the communication logic should stay in the module, or at least, separate from the controller logic)
• the user implements the setter and/or the getter to perform some functionality he can't perform with the callback.

Some more details on the implementation.

Requirements on the object type:

• The type specified should have a default constructor, so to easily allocate the storage variable, and be assignable (or copy contructable).

Discussion about the callback process. There is one callback per parameter, and not a global one, which would force the user to implement a long switch in it. @lornat75 suggested the creation of two callbacks: one before the actual set (e.g. willSet__Name__), and one after the set (e.g. didSet__Name__). I also suggested the addition of a validation method. It can be implemented with the following method:

bool validate__Name__(const __type__& newValue);

which by default return true, but can be overloaded by the user. The actual implementation of the callback mechanism can be done with interfaces, or function pointers.

Attribute list: At this point we thought about the following possible attributes:

• atomic/nonatomic: the implementation uses (or does not use) a mutex to access the variable
• readonly: only the getter will be generated
• storage=name: explicitly specify the name for the backing variable (otherwise a default one is given, e.g. a_name, or something else).
• nostorage: no backing variables are used: the setter and getter are pure virtual functions and they must be implemented by the user.
• vector: specify that the type is a vector type. If thrift is able to detect this, we can avoid this property.

Last notes on the vector type.

We must be able to support vector types (as they are very common). This opens another feature of the utility and one possible issue we must resolve. If the object is a vector we would like to be able to set one particular object in the vector. To do this, only if the type is a vector, the utility generates two other methods:

virtual const __type__& get__Name__AtIndex(int index);
virtual void set__Name__AtIndex(const __type__&, index);

To be able to directly access an element in an array we could add another requirement: the type must implement the operator() (or operator[]).

The last issue, which is related to vector type, currently open is: how to specify the size for it, in order to perform a proper allocation and range checking during assignment and element retrieval?

[paulfitz]

@francesco-romano do you think you could sketch out what a simple program using such a property would look like? Specifically how it gets hooked up to the network.

[francesco-romano]

This is just a sketch:

EDIT: see gist https://gist.github.com/lornat75/57f550cf9fc0fb01b9c4

[francesco-romano]

If you have any questions @traversaro and I will be very happy to answer :)

[lornat75]

should we copy this into a "gist"? it could be easier to comment/suggest modify

[lornat75]

https://gist.github.com/lornat75/57f550cf9fc0fb01b9c4

[paulfitz]

Thanks for doing that @francesco-romano. This part:

m_rpcHelper.yarp().attachAsServer(port);
port.open("/myModule/rpc")
m_rpcHelper.addObserver(this);

was what I needed to get clear in my head. I see the general idea.

We could actually get some of this functionality by pushing the existing translation of stuctures a bit. A struct like this:

struct {
  1: i32 integralLimit;
  2: Vector homePosition;
  3: Vector proportionalGain;
  4: Vector integralGain;
  5: double someDoubleVariable;
  6: i32 cycleNumber;
  7: Vector centerOfMass;
};

currently is translated as a big Portable that you can read/write en masse with all that data in it. There would be nothing to stop us also generating separate classes to give an RPC-style interface for sending and receiving fragments of the data, along the lines of what you're proposing. There's nowhere in the thrift syntax to specify read-only or atomic though, or to remap how things are named.

To get exactly what you want will require changing the thrift parser. At that point things get a bit strange, since there's very little left of the original thrift in what we'll be calling thrift. One other option could be to treat parameters as a third IDL (along with the existing yarpidl_thrift and yarpidl_rosmsg) and not trying to force it into thrift. The YARPIDL cmake macros are generalized and don't depend on thrift, they already work with rosmsg for example. We could clean up a bit where code is put so using structures written in different IDLs is clear. A third IDL could perhaps grow to become a better one for us, rather than the two borrowed IDLs already supported? Maybe this is too ambitious. But the use of thrift was originally intended as a prototype, a placeholder to get some experience with. It worked out well enough, but has quirks (like the hack needed to name native YARP types).

[lornat75]

I am fine with both solutions. Extending thrift seems easier.

@paulfitz, @francesco-romano

[arocchi]

A couple of things I would like to point out which are in the current implementation of paramHelp and I would like to see also in the next version 1) document the parameter in the same file where it is defined. This can be used to automatically generate help messages 2) specify basic "validators" which automatically check if the new value is valid (e.g., lower bound, upper bound, lower and upper bounds) 3) as last, support custom user types and custom validators classes (no need to reimplement the validate many times, just implement a validator class once)

[paulfitz]

I also wonder about nested elements. If A contains B, presumably it would be desirable to have modifications to an individual field of B propagate? This is similar to the requirement to be able to change elements of an array individually.

For @arocchi's documentation requirement, it looks like thrift generators already have access to any docstring before an element, like:

/**
 * ... this ...
 */

I hadn't realized that. Should add this to @elen4's auto-generated help for services... EDIT @elen4 already had this implemented. Added it for structures.

[paulfitz]

Along with vectors, thrift also defines sets and maps, will need also a way to specify diffs for those (just adding notes for myself here). Also, appending/removing to/from lists.

[paulfitz]

I'm working on approaching this functionality incrementally. Here's what I'm doing. Suppose we have a structure defined in thrift called Settings. Then, in C++, we get a class called Settings which works as normal, serializing the entire structure, suitable for use on streams. What I'm adding is a class called (provisionally) Settings::Editor. Setters and getters in this class can track changes made to a Settings instance, and apply those changes to another Settings instance (triggering appropriate callbacks). This can also be done also across the network, by hooking up a pair of Settings::Editor instances on the sender/receiver side (rather than Settings instances directly). There are some basic tests of a prototype at https://github.com/paulfitz/yarp/blob/52d8402119f4855d720189f8b83729df46ca06f6/src/idls/thrift/tests/demo/main.cpp#L622 but I'm not ready to ask for feedback yet, it is too early. Here's a rough list of what needs doing:

• [x] setters/getters/callbacks for simple primitive types
• [ ] setters/getters/callbacks for thrift-style lists/sets/maps
• [ ] setters/getters/callbacks for nested types
• [ ] setters/getters/callbacks for yarp-specific classes such as Vector
• [ ] setters/getters/callbacks for fictional fields
• [ ] examples and documentation
• [ ] finalize naming (including capitalization of setters/getters/callbacks)
• [x] extract inline documentation and make it accessible "live"
• [ ] support basic validators for all types
• [ ] support custom validators for all types
• [ ] atomic/nonatomic control of mutexing
• [ ] independent control of name of variables backing a logical field
• [ ] readonly fields

Please do let me know what I've missed from this list.

[paulfitz]

@francesco-romano do you have any thoughts on what the api should look like for variable-length vectors? Or are the set/get AtIndex the only two cases of interest?

[francesco-romano]

What do you mean by “variable length” vector? Our use case is that the size of vector is know at configuration time. We have never thought about vectors which change at run-time. Maybe @traversaro can also comment on this.

[paulfitz]

Sorry for neglecting this thread. I've added a small example of what I've implemented so far. Given a Settings structure https://github.com/robotology/yarp/blob/master/example/idl/thriftEditor/settings.thrift then we can modify the settings locally and see callbacks happening remotely https://github.com/robotology/yarp/blob/master/example/idl/thriftEditor/main.cpp

[traversaro]

Thanks @paulfitz , we will test it soon.

[paulfitz]

Thanks @traversaro. One thing that has been holding me back is that I wasn't sure what to do about manipulations to nested members, lists, and maps. For example, the original proposal had considered one manipulation possible on a top-level list (changing the value an element) but there are others (removing elements, adding at different locations). While we could try to keep our focus narrow, I'd expect to get a lot of abuse from users about this. But the full space of possible manipulations can be enormous and expresses itself awkwardly as a set of set_... calls.

I was thinking an alternative might be to compute differences in state. So the person modifying the structure accesses its fields directly, does what they want with them, then calls e.g. sync(). That compares the current state with a previous copy of the structure, generating a diff. This costs us some storage and cpu, but frees us from having to define a call for every kind of manipulation, and frees the user from having to learn about those calls. On the receiver side, we could provide a callback/validator with the current state, the proposed next state, and a description of the diff.

Not sure, I might be going off the rails here :-)

[barbalberto]

I like the idea of letting the user directly access the srtuct, it'll be more user friendly and intuitive to use, instead of having functions with 'complex' names.

I'll push it a bit further for the sake of sharing ideas.

We could create a small wrapper around types and override the '=' operator in such a way that il will automatically send the yarp message, like:

class YInt{
private:
    int value;
    Settings::Editor *set;

public:
    YInt();
    YInt(Settings::Editor *s);
    // cast to 'normal' int
   operator int() {   return value; } 
    YInt & operator=(int v)
    { 
         // store the value
          value = v;
          // send a yarp message to notify the value has been changed
          set->write();
    }
    YInt & operator=(YInt v);
};

In the main, instead of

    settings.set_id(5);

will use

   settings.id = 5;

Basically it is the same as calling the function set_int, just more friendly and readable.

But I don't know how it scales for vector, list or other types.

[paulfitz]

@traversaro or others, did you get a chance to test? Did I miss something basic?

[traversaro]

@paulfitz sorry, still didn't have the change to the test this new features. I hope that when @francesco-romano comes back from vacation we can try them in a new module, or we can try to migrate a paramHelp-based module.

[francesco-romano]

@paulfitz I managed to take a look at the issue. First of all, thanks you for the work. I think it is going in the right direction.

I have started migrating a module to the thrift, and I will keep updating it as a "test" application.

I have some comments so far (maybe something is still a todo, but I list them here anyway). Some are "important", others are just notes / opinions

1. the most "often" use case for us is the use of RPC via command-line. I think this is still missing (or I wasn't able to understand how setting variables via yarp rpc)
2. I noticed in the auto generated files that the thrift-include files are included with angular brackets and full path. I'd prefer the use of quotes with just the file name, i.e. #include <include/thrift/Gains.h> => #include "Gains.h". This is because I usually use quotes for intra-project includes and angular brackets for libraries.
3. If I understood correctly the begin/end functions are needed to send the data over the network only once. I tried them but I don't think they work as expected (but anyway, it is a great feature!)
4. I noticed the data members are public instead of private + accessors. The accessors are only in the Editor. Is this intended? If so, how can I access nested data with accessors?
5. What's the use of the state() method?

If you need any other feedback (in some area in particular) I can focus my attention there.

[paulfitz]

thanks for the feedback @francesco-romano! I made some fixes based on your comments, and updated the example/idl/thriftEditor example. If trying stuff out, please be sure to rebuilt yarp and then do a make clean on your project so that the code is regenerated.

1. Manual messages would look like patch (set var1 val1) (set var2 val2). I haven't optimized this for human comfort, happy to do it any way you like.
2. I take your point about header files but would like to postpone changing this.
3. You're right, the begin/end functions were ineffective, should be fixed now.
4. The data members are public since this is exactly how we've been translating thrift structures, unmodified. This is nothing special for this RPC helper. For other uses of structures direct access to its members is exactly what you want. Access to nested data is a great question, see e.g. https://github.com/robotology/yarp/issues/184#issuecomment-58535608. Once we have a clear agreed idea on how the api for manipulating nested data should look, then I'm happy to reorganize the generated code to match.
5. The state() method gives you the bare structure that is being edited. It can be external to the Editor, or owned by it.

I'd say the question about nested data is where I most want ideas. What should code setting nested members (and manipulating lists) really look like? Feel free to ignore the structure I've made so far completely.

[francesco-romano]

Thanks @paulfitz for the answers. 1) Ok, I'll take a look at it. Let's say that (for us) the most common way to interact with the module is via the terminal. We'll try to find an "easy" way. Through the patch command it seems to me too cumbersome. 2) No problem. It is a minor detail of course. And I noticed it only because I add to modify the inclusion otherwise it did not compile. 5) Ok.

For nested structure. Until now the most complex data we modified has been a vector or matrix. We've never thought about real structures. Nevertheless we will think about it. I've also introduced the argument to @alecive. Some advices from his experience on iCub's modules will be useful!

[paulfitz]

For (1), I've added a shorthand:

set <fieldname> <value> <fieldname2> <value2>

now works. Example updated.

[paulfitz]

hmm if the most common way of setting values is via command line, then finding a good code API for nested values matters less. From command line, preferred syntax would I guess be:

• <field>.<field>.<field> for nesting I guess, and
• <field>[index] for access to individual slots in an array

[francesco-romano]

@paulfitz I haven't tested with the new updates yet, but I like the command line syntax. Also for nested and vector.

[paulfitz]

Started a tutorial for this http://wiki.icub.org/yarpdoc/thrift_editor.html

[lornat75]

Thank @paulfitz and @francesco-romano this looks like a very useful tool.

@andreadelprete @arocchi @EnricoMingo if you are not yet aware of this, it should work as a replacement for the param helper using the thrift IDL in yarp

[pattacini]

Seems thrift implementation addressed this issue. Closing.