Automatic Code Generation Support #77

diegoferigo · 2018-02-22T17:38:32Z

Implements #82. Covers the entire Milestone#4.

diegoferigo · 2018-04-11T14:50:33Z

This is a huge PR @traversaro. Understanding the logic behind the automatic code generation is a bit tricky and if you are interested about details we can have a f2f meeting (waiting for some detailed documentation and tutorials on the website).

For the time being, it would be nice receiving feedback mainly on:

Templates and explicit specialization & declaration: ConvertStdVector.h ConvertStdVector.cpp Parameters.h Parameters.cpp
Pimpl idiom in edge cases: Configuration.cpp

traversaro

Clearly, I was not able to review everything, but I have a few comments.

traversaro · 2018-04-11T14:54:51Z

toolbox/CMakeLists.txt

+find_package(Matlab REQUIRED
+    MX_LIBRARY
+    ENG_LIBRARY
+    MAIN_PROGRAM


Do not you need also the SIMULINK component? We recently installed wb-toolbox with @ahmadgazar on a pc without simulink (due to an error on our side) and the compilation failed, but the CMake configuration was not complaining.

Unfortunately SIMULINK is present only in CMake > 3.7. I can add a comment for future reference.

Ahhh, I got it.

traversaro · 2018-04-11T14:57:03Z

toolbox/CMakeLists.txt

@@ -99,8 +118,6 @@ if (WBT_USES_ICUB)
    #
    #     include_directories(SYSTEM ${iKin_INCLUDE_DIRS})
    # endif()
-
-    include_directories(SYSTEM ${ctrlLib_INCLUDE_DIRS})


Why did this disappeared? As far as I know, ICUB still does not export its include directories in the cmake targets.
You will not noticed it if you are using ICUB installed in the same prefix of YARP, but if you try to compile the wb-toolbox with ICUB enabled and installed in a prefix different from YARP or iDynTree, I guess the compilation is failing.

This didn't disapper, it was just moved after the creation of the WBToolboxLibrary target. Here's the code (every reference might disappear due to a force push):

if (WBT_USES_ICUB) list(APPEND LINKED_LIBRARIES ctrlLib) target_compile_definitions(WBToolboxLibrary PUBLIC -DWBT_USES_ICUB) target_link_libraries(WBToolboxLibrary PUBLIC ctrlLib) if (${ICUB_USE_IPOPT}) list(APPEND LINKED_LIBRARIES iKin) target_link_libraries(WBToolboxLibrary PUBLIC iKin) endif() endif()

I switched to target_* when possible, and includes are included transitively.

includes are included transitively.

Unfortunately, this does not hold for ICUB libraries, as they do not declare they include libraries using target_include_libraries, see https://github.com/robotology/icub-main/search?utf8=%E2%9C%93&q=target_include_directories&type= . For this reason, using directly ctrlLib_INCLUDE_DIRS and iKin_INCLUDE_DIRS is necessary.

I see -.- This is unfortunately misleading. I usually install all icub stuff in the same folders as yarp and I would never have noticed it. Fixed, thanks for the tip.

traversaro · 2018-04-11T15:00:59Z

toolbox/CMakeLists.txt

+)
+
+# Import math symbols from standard cmath
+target_compile_definitions(WBToolboxLibrary PUBLIC "-D_USE_MATH_DEFINES")


Why PUBLIC ? I far as I can see, M_PI is used only in .cpp files. I would turn it PRIVATE and only add it only when compiling on MSVC ?

Good catch!

traversaro · 2018-04-11T15:01:52Z

toolbox/CMakeLists.txt

+target_include_directories(WBToolboxLibrary SYSTEM PUBLIC
+    ${Matlab_INCLUDE_DIRS}
+    ${Matlab_ROOT_DIR}/simulink/include
+    ${YARP_INCLUDE_DIRS}


I think this is not necessary, and this should be supported just by linking YARP .

Yes they are not required. Linking YARP is sufficient. By the way, would you recommend to limit the YARP components as I do with iDynTree?

traversaro · 2018-04-11T15:08:00Z

toolbox/include/base/Factory.h


+// iCub-depent blocks


typo: dependent

traversaro · 2018-04-11T15:21:04Z

toolbox/include/base/RobotInterface.h

+    using ControlBoardIndex = unsigned;
+    using JointIndexInControlBoard = unsigned;
+
+    using JointIndexToYarpMap =


I am a bit confused by all this structures (and avoiding this is actually why the remapper and the support for specifying the serialization in iDynTree was explicitly coded to avoid this constructs!).
If this is just used for the "smandruppator", in the long run I suggests to rename this variables to be a generic "subpart" division of the joints, as in the future we could easily connect to the robot using a single controlboard containing all the joints (for performance reason) while keeping the nice division of the robot in parts for visualization/debugging.

All these maps and methods for gathering them are for the "smandruppator" (ModelPartitioner). I plan sooner or later to polish this RobotInterface class further and use it as standalone wrapper for the RemoteControlBoardRemapper and KinDynComputations. Since these maps might be helpful and coding them was a real pain, I decided to store them here. They are lazily accessed, if you never call the get methods the code that spawns multiple single control boards is never executed.

traversaro

Reviewed requested parts.

traversaro · 2018-04-12T08:08:51Z

toolbox/src/base/ConvertStdVector.cpp

+    output.clear();
+    output.resize(input.size());
+    std::transform(input.begin(), input.end(), output.begin(), [](const int& num) {
+        return std::to_string(num);


How are you using this methods? If you are using this methods to convert parameters in configuration files, typically you do not want this conversion to be influenced by the locale setting, and so to_string is not the appropriate functions. I suspect also std::stod is affected.

I originally became aware of this edge case in the context of URDF parsers, see robotology/idyntree#288 . Most of the time this kind of problems it not recognized because:

The default locale setting for C/C++ program is "C".

The user-preferred language for most developers is en-US, or in general en-SOMETHING.

However, if the libraries is used in a process that uses the Qt libraries on a computer in which the user-preferred locale is something like it-IT or es-ES, the Qt libraries set the C/C++ locale to be the user-preferred locale, and the libraries stop working properly. As you may imagine, this is not a behavior that is easy to debug.

As you may imagine, this is not a behavior that is easy to debug.

I remember "piovere madonne" from that issue 😅 In this case no file is involved, at least, not at the current stage.

This template is used inside wbt::Parameters for converting vector parameters between different data types. Values are usually read from the block's mask in Simulink.

I don't see how this can affect std::to_string, but I see the point of the other way around, and I mean std::stod and std::stoi which I use in many points. The origin of this feature is to give the possibility of calling:

// Store a parameter as a double Parameter p(42, {ParameterType::DOUBLE, 0, 1, 1, "ParamName"}); // Insert it in a container with other parameters Parameters params; params.storeParameter(p); ... // Get the parameter that is cast internally int value; getParameter("ParamName", value);

This should hold also for std::string.

In future versions I plan to give the possibility of override those variables for give flexibility to the automatic code generation. In this case, probably the parameters to override will be stored in a configuration file.

@traversaro Do you think this problem should be addressed by whom will parse the configuration? It will not be responsibility of the toolbox. And, if this is the case, all RFModule should be careful of this behavior (how does yarp handle it? I guess it does not).

And, if this is the case, all RFModule should be careful of this behavior (how does yarp handle it? I guess it does not).

I do not think that yarp::os::Property use any kind of double to string facilities provided by C++.

I don't see how this can affect std::to_string, but I see the point of the other way around

Basically, you risk to store doubles as 10,000. Depending on the usage, it may or not may be a problem.

Values are usually read from the block's mask in Simulink.

They are read as strings or directly as doubles? In the generated code, how they are stored? If they are read or stored as strings, I think we already have the problem, as a user would but 0.10 in its Simulink mask, and in could be read if the wrong way depending on the locale.

They are read as strings or directly as doubles?

Numeric types are read as double and strings as strings. Simulink functions are used, and we do not have control on them. Hopefully they are robust to locale.

In the generated code, how they are stored?

About serialization / deserialization to the RTW file (which is kind of a database read by the automatic generation pipeline) the following logic is used:

Strings are serialized as "stringContent"

Numeric types are all converted to double and serialized as 42.0

Again, I use Simulink functions for doing this.

If what I wrote so far is right, the parameters are correctly read by Simulink, correctly serialized to the RTW file, and correctly read from the RTW file from the autogenerated code pipeline.

Possible problems I see

These helpers are used only when the block developer wants for some reason to get a parameter with a type different than the original type (specified in the parameter metadata). This step might be locale-dependent.

Numeric -> string

If you want to convert a double to a string you will get "10,000" as you mentioned. But, since this is already kind of downstream, after the cast unlikely you will compare numeric types as strings. Rather than converting the numeric parameter to string you would rather convert the other string to numeric and then compare the numeric values.

String -> numeric

Problems would arise if the block developer chooses to store a numeric type as a string (non-default behavior). In this case I have no idea what would be the result of calling std::stod("42.0") on locales that assume the "42,0" as a separator. Anyway, this is a very edge case.

Final thoughts

In the light of these considerations, the current situation doesn't look problematic. Does it sound good even for to @traversaro?

Sorry, I think I finally got it.

The Parameters class is a generic key-value storage in which values are typed (either std::string, bool, int or double) used to set parameters for code-generated blocks.
However, even if the parameters are typed, the class offers a way of converting internally parameters from one type to another. However, this feature is not used currently in any part of the code, and at the moment we are not currently plan to use it, right?

At this point, the only thing I am afraid is the current situation:

a given block takes in input a double parameter, but for some reason the external C++ code save it as a string parameter, with some strange locale setting

if I understand correctly the code in https://github.com/robotology/wb-toolbox/pull/77/files/05bd751d6d557cb110d1437bb845ba4a7812fcd0#diff-169c37a45eec0ad411c3e3a6f9052cf3R208, the parameter parsing is successful, but the parameter is actually converted implicitly from string to double.

I must say that if I got the big picture, I do not understand the need for the implicit conversion functionality in Parameters, but now that it is there I do not think it make sense to try to remove it.

I have nevertheless two suggestions, but I am not sure if they make sense:

Add a line in Parameters's documentation to say that for implicit conversions between std::string and the other types the standard functions std::to_string, std::stoi, etc etc are used, and the behavior of this functions are influenced by the process-global behavior.

If it is not too complicated, consider also the implicit type when performing the check on the block parameters.

However, this feature is not used currently in any part of the code, and at the moment we are not currently plan to use it, right?

We do not currently use it, but on the long run I thought it was a nice addition especially if we split the core of the toolbox. Consider the following snippet:

Parameters parameters; parameters.storeParameter(42, {ParameterType::DOUBLE, /*index=*/0, /*rows=*/1, /*cols=*/1, "paramName"});

The type specified in the metadata always wins. The usage without conversion might have led to a template instantiation difficult to figure out. In this way we don't mind.

traversaro · 2018-04-12T08:18:48Z

toolbox/include/base/Parameters.h

+
+private:
+    class impl;
+    std::unique_ptr<impl, void (*)(impl*)> pImpl;


I am confused here. You are passing void (*)(impl*) as the deleter, and I am not sure that I understand what this mean at the C++ level to be honest. Who is actually deleting the class? Are you sure that this is not creating a memory leak?

Sorry, I got it during the review. void (*)(impl*) is the type of the deleter, and the actual deleter is passed in the unique_ptr constructor. I think you mentioned me f2f that the need for doing this is due to the necessity of defining a copy constructor, it would be nice to have a small commenter here hinting that.

Yes something like that. If I understood correctly, the issue here is use usage of:

~WBBlock() override = default;

I used as an example WBBlock which is not copiable.

The combination of the default destructor plus the forward declaration of the object handled by the unique_pointer makes sizeof complain. sizeof is used in the default deleter, for this reason passing a custom deleter solves this issue.

The combination of the default destructor plus the forward declaration of the object handled by the unique_pointer makes sizeof complain. sizeof is used in the default deleter, for this reason passing a custom deleter solves this issue.

The combination of the default destructor plus the forward declaration of the object handled by the unique_pointer is a standard feature of the pimpl pattern, see for example all the usage in the blocks such as https://github.com/robotology/wb-toolbox/pull/77/files#diff-d7d137bec79209b2e9a3a4548fd95e51R25, and there seems not to be problem with sizeof .

If I understood correctly, the problem is combining this with a non-copyable class? In that case it is not clear to me how it is related to sizeof, but I do not strongly feel about this issue, so if it is working for you it is fine for me.

From unique_ptr:

std::unique_ptr may be constructed for an incomplete type T, such as to facilitate the use as a handle in the pImpl idiom. If the default deleter is used, T must be complete at the point in code where the deleter is invoked, which happens in the destructor, move assignment operator, and reset member function of std::unique_ptr.

As discusses f2f, when possible removing the custom deleter and moving the default destructor to the cpp is preferred for readability.

traversaro · 2018-04-12T08:48:25Z

toolbox/include/base/Parameters.h

+     * @return True for success, false otherwise.
+     */
+    template <typename T>
+    bool storeParameter(const T& param, const wbt::ParameterMetadata& paramMetadata);


Have you tried compiling and linking this on Windows? I think the WINDOWS_EXPORT_ALL_SYMBOLS option will be quite stressed by this edge case.

Also giving it a shot on macOS may be worth, given all the intensive use of templates.
You can use my former iMac (the idea is using it for that). cc @DanielePucci

Have you tried compiling and linking this on Windows?

Of course no 😁 We actually claim multiplatform support but at least for versions >= 3 we never tested in depth all functionalities on non-linux platforms. For this reason some while ago I decided to put a big disclaimer on the website.

No, apparently we cannot use the iMac because it does not have a Matlab/Simulink license.

Other minor changes: - Only the WBToolboxMex depends on Matlab and Simulink headers - Link only required YARP components

std::stringstream is not copiable and new compilers don't complain thanks to the supported move semantics. Using pointers to stringstream in the internal container allows old compilers to work properly.

- Use a smart pointer to handle the temporary Block object in the mdlInitializeSizes callback - Raise an error if a block additional option is not recognized

This method is part of the Block interface. If in child Block classes the method is called but not directly implemented, the parent method is called. In this case this behaviour can be error prone, especially when refactoring the code.

Reverts wrong 47e2dc6 commit

- Wrong GetLimit parameter type. - Typo in a Signal validity check. - Error when setting port sizes. If the port number is greater than the configured number of ports, Simulink crashes. - Related edits to the Simulink Library

This is due to the default value of Yarp's YARP_WRAP_STL_STRING option which is ON in Windows.

The RTW file should have all the required information about signals and parameters.

- The Coder implementation should get the real size from the RTW - The Simulink implementation should get the real size from the parameter parsed from the mask (this value actually is what is written to the RTW file later in the pipeline)

robotology#77

diegoferigo added complexity::hard priority::high issue::status::in-progress type::enhancement labels Feb 22, 2018

diegoferigo self-assigned this Feb 22, 2018

diegoferigo force-pushed the WB3.1 branch from d8e4a56 to 81e8d43 Compare February 23, 2018 10:12

diegoferigo added the other::documentation-missing label Feb 24, 2018

diegoferigo force-pushed the devel branch from 6e71de4 to fe4be7d Compare February 25, 2018 11:32

diegoferigo force-pushed the WB3.1 branch from 81e8d43 to 5763b17 Compare February 26, 2018 07:26

diegoferigo added component::blocks component:library-mex component:simulink labels Feb 27, 2018

diegoferigo force-pushed the WB3.1 branch 4 times, most recently from 82c5c42 to e2b2829 Compare April 6, 2018 12:24

diegoferigo force-pushed the WB3.1 branch from eb64e5f to c19997c Compare April 11, 2018 12:27

diegoferigo changed the title ~~[WIP] Automatic Code Generation Support~~ Automatic Code Generation Support Apr 11, 2018

diegoferigo requested a review from traversaro April 11, 2018 14:43

traversaro requested changes Apr 11, 2018

View reviewed changes

traversaro reviewed Apr 11, 2018

View reviewed changes

diegoferigo force-pushed the WB3.1 branch 3 times, most recently from 9407ef0 to 05bd751 Compare April 12, 2018 06:33

traversaro requested changes Apr 12, 2018

View reviewed changes

diegoferigo force-pushed the WB3.1 branch 3 times, most recently from 01ca540 to 01119d9 Compare April 12, 2018 15:31

diegoferigo added 6 commits May 7, 2018 11:17

YARP and iCub are now linked privately

cbf50e0

Other minor changes: - Only the WBToolboxMex depends on Matlab and Simulink headers - Link only required YARP components

Added new tests in Travis exploiting build stages

1f82185

Compile S-Function only when Matlab is found

1e203d8

Fixed wbt::Log for old versions of gcc and clang

f623a55

std::stringstream is not copiable and new compilers don't complain thanks to the supported move semantics. Using pointers to stringstream in the internal container allows old compilers to work properly.

Missing header let osx fail compiling

eae3335

Added new Appveyor test

99bd06a

diegoferigo force-pushed the WB3.1 branch from 1e8d565 to bac5648 Compare May 7, 2018 09:18

diegoferigo added 15 commits May 7, 2018 09:32

Added badges in README

6c5be76

Fix Windows compilation

f440b3c

Improved log messages of Parameters class

9011b26

Small S-Function improvements

b4e532b

- Use a smart pointer to handle the temporary Block object in the mdlInitializeSizes callback - Raise an error if a block additional option is not recognized

Always check the return value of Signal::setBuffer

6e6bca4

Specify explicitly the parseParameter class

43bca59

This method is part of the Block interface. If in child Block classes the method is called but not directly implemented, the parent method is called. In this case this behaviour can be error prone, especially when refactoring the code.

Fixed Signal::setBuffer method

f6a53ff

Reverts wrong 47e2dc6 commit

Fixed small issues that prevented test controllers to run

45e8488

- Wrong GetLimit parameter type. - Typo in a Signal validity check. - Error when setting port sizes. If the port number is greater than the configured number of ports, Simulink crashes. - Related edits to the Simulink Library

Improved logic for cleaning S-Function resources at termination phase

7aeb701

Fixed compilation in Windows

ac0450d

This is due to the default value of Yarp's YARP_WRAP_STL_STRING option which is ON in Windows.

Improved doxygen layout

93eb55c

Small fixes

40c9cd1

The autogenerated code should not call configureSizeAndPorts

825c2b5

The RTW file should have all the required information about signals and parameters.

ParameterMetadata::operator== should be const

44a5cfc

diegoferigo force-pushed the WB3.1 branch from bac5648 to 3960ed4 Compare May 7, 2018 09:32

diegoferigo merged commit da74420 into robotology:devel May 7, 2018

diegoferigo deleted the WB3.1 branch May 7, 2018 13:23

diegoferigo added pr::resolution::merged and removed pr::status::feedback labels May 8, 2018

diegoferigo mentioned this pull request May 8, 2018

Automatic Code Generation support exploiting Simulink Coder #82

Closed

diegoferigo added a commit to diegoferigo/WB-Toolbox that referenced this pull request May 18, 2018

Added remaining release notes of PR robotology#77

1fa47fb

robotology#77

diegoferigo mentioned this pull request Jun 22, 2018

New CMake structure and plugin-based blocks architecture #137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatic Code Generation Support #77

Automatic Code Generation Support #77

diegoferigo commented Feb 22, 2018 •

edited

Loading

diegoferigo commented Apr 11, 2018 •

edited

Loading

traversaro left a comment

traversaro Apr 11, 2018

diegoferigo Apr 11, 2018 •

edited

Loading

traversaro Apr 11, 2018

traversaro Apr 11, 2018

diegoferigo Apr 11, 2018

traversaro Apr 11, 2018

diegoferigo Apr 12, 2018 •

edited

Loading

traversaro Apr 11, 2018

diegoferigo Apr 11, 2018

traversaro Apr 11, 2018

diegoferigo Apr 11, 2018

traversaro Apr 11, 2018

diegoferigo Apr 11, 2018

traversaro Apr 11, 2018

diegoferigo Apr 11, 2018 •

edited

Loading

traversaro left a comment

traversaro Apr 12, 2018

diegoferigo Apr 12, 2018

traversaro Apr 12, 2018

diegoferigo Apr 12, 2018 •

edited

Loading

traversaro Apr 15, 2018 •

edited

Loading

diegoferigo May 3, 2018

traversaro Apr 12, 2018

traversaro Apr 12, 2018

diegoferigo Apr 12, 2018

traversaro Apr 12, 2018

diegoferigo Apr 12, 2018

traversaro Apr 12, 2018

traversaro Apr 12, 2018

diegoferigo Apr 12, 2018 •

edited

Loading

traversaro Apr 12, 2018

Automatic Code Generation Support #77

Automatic Code Generation Support #77

Conversation

diegoferigo commented Feb 22, 2018 • edited Loading

diegoferigo commented Apr 11, 2018 • edited Loading

traversaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoferigo Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoferigo Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoferigo Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

traversaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoferigo Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Possible problems I see

Numeric -> string

String -> numeric

Final thoughts

traversaro Apr 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoferigo Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoferigo commented Feb 22, 2018 •

edited

Loading

diegoferigo commented Apr 11, 2018 •

edited

Loading

diegoferigo Apr 11, 2018 •

edited

Loading

diegoferigo Apr 12, 2018 •

edited

Loading

diegoferigo Apr 11, 2018 •

edited

Loading

diegoferigo Apr 12, 2018 •

edited

Loading

traversaro Apr 15, 2018 •

edited

Loading

diegoferigo Apr 12, 2018 •

edited

Loading