Align WBT and BlockFactory #168

diegoferigo · 2019-01-30T15:15:09Z

This PR contains the modification required to work with the current master branch of BlockFactory.

traversaro · 2019-01-31T13:54:03Z

toolbox/library/CMakeLists.txt

-        LIST_PREFIX WBT
-        SOURCES src/QpOases.cpp
-        HEADERS include/WBToolbox/Block/QpOases.h)
+if(${qpOASES_FOUND})


With this change, qpOASES blocks are compiled even if WBT_USES_QPOASES is not enabled. I am afraid this may be confusing.

If you want this to be the behaviour (not a big fan of it to be honest), problably it would make sense to just remove those variables then.

Mmh I didn't realized I used the _FOUND here, my mistake. If you see in the bottom of this file, the WBT_USES_ variables are used again. Thanks!

traversaro · 2019-01-31T13:54:35Z

toolbox/library/CMakeLists.txt

+# Other
+list(APPEND WBT_BLOCKS "RealTimeSynchronizer")
+
+if(${ICUB_FOUND})


See https://github.com/robotology/wb-toolbox/pull/168/files#r252674954 .

traversaro · 2019-01-31T13:56:22Z

toolbox/library/src/Jacobian.cpp

-        blockInfo->getOutputPortMatrixSize(OutputIndex::Jacobian).first,
-        blockInfo->getOutputPortMatrixSize(OutputIndex::Jacobian).second);
+        blockInfo->getOutputPortMatrixSize(OutputIndex::Jacobian).rows,
+        blockInfo->getOutputPortMatrixSize(OutputIndex::Jacobian).cols);


This was really necessary! I regretted using pairs and tuples in this case.

traversaro

Comments on the WBT_USES_** options

Due to the change of the installed library name due to the usage of CMake utilities provided by BlockFactory.

After robotology/blockfactory#33 some datatypes and interface methods changed

diegoferigo · 2019-01-31T14:08:24Z

Done.

For what concern the temporary CI fix, I would keep it in this PR, regenerate the docker images after merging, and then revert the commit.

traversaro · 2019-01-31T14:15:10Z

matlab/library/WBToolboxLibrary_repository.mdl

@@ -5837,14 +5837,14 @@ Library {
 	}
 	Block {
 	  BlockType		  S-Function
-	  Name			  "Yarp Read"
+	  Name			  "YarpRead"


Even if WB-Toolbox will always ship the manually written .slx/.mdl, I think this space removal is good for eventual consistency with blockfactory libraries generated by robotology/blockfactory#10 .

For sure less error prone, it was a trivial change, just some sed magic. Lucky us that we use mdl and not slx.

OT: To be honest I am still confused on the role of mdl and slx in WBToolbox. The .slx model include .mdl one, but has support for the images? We could use the mdl also in the example of BlockFactory?

I should try again substituting the slx with the mdl also in the installed tree.

Ok I just tried. The models work fine, but the library does not show up in the Simulink Library.

So let me recap:

The file that you mantain is the <something>.mdl

Then you save it to .slx, using the export_library.m script, and that is the file that you are installing so that it can be found by Simulink

is this correct?

I do not think the .slx file is a big problem for WB-Toolbox, but just as thought experiment: why we do not run the export_library.m script as part of the build project to create the .slx in the build directory, and then we install as any build artefact?

Just to finish this discussion, installing the mdl is not optimal and can create problems after an update. If the user resaves the library in slx, and afterwards the mdl is updated after an upstream change, the slx does not get updated automatically and there is no warning to the user. This means that the user thinks to have the latest update from the repository but instead he / she is running an old version. This marks installing mdl not an option.

Some while ago I stored some information on the export process here (a bit unusual but there was no website at the moment):

wb-toolbox/matlab/CMakeLists.txt

Lines 9 to 14 in 368119e

# The logic about library development is the following:

#

# * Developers should edit the library (e.g. changing block's masks) stored in the

# matlab/library/WBToolboxLibrary_repository.mdl file.

# * Developers should export the changes using the export_library custom target.

# * Users will use the files installed from the exported/ folder.

why we do not run the export_library.m script as part of the build project to create the .slx in the build directory, and then we install as any build artefact?

Usually the mdl is exported with the matlab version of the maintainer of the library (:hand:). If users have an old version of Matlab they cannot convert a model / library created with a newer version.

Cool. I finally got it, I think. So if we did not wanted to commit a .slx we could first save the mantainer's .mdl to a mdl compatible with 2014b:

save_system(libraryName, 'WBToolboxLibrary', 'ExportToVersion', 'R2014B_MDL');

and then convert it to R2014B_SLX just during the build on the user's machine, before its install, to support correctly the EnableLBRepository option.

traversaro

The .slx/.mdl discussion should not block the merge.

diegoferigo · 2019-01-31T15:30:44Z

Merged, thanks.

diegoferigo added complexity::low component::blocks component::cmake pr:status:in-progress labels Jan 30, 2019

diegoferigo self-assigned this Jan 30, 2019

diegoferigo force-pushed the feature/AlignWithBlockFactory branch 4 times, most recently from c79004b to 1585307 Compare January 31, 2019 13:38

diegoferigo changed the title ~~[WIP] Align WBT and BlockFactory~~ Align WBT and BlockFactory Jan 31, 2019

diegoferigo added pr::status::feedback and removed pr:status:in-progress labels Jan 31, 2019

diegoferigo requested a review from traversaro January 31, 2019 13:47

traversaro reviewed Jan 31, 2019

View reviewed changes

traversaro requested changes Jan 31, 2019

View reviewed changes

diegoferigo added 5 commits January 31, 2019 15:06

Use BlockFactory CMake function to register blocks and create the plugin

828b4c9

Updated Simulink Library

7d25e89

Due to the change of the installed library name due to the usage of CMake utilities provided by BlockFactory.

Modified blocks to use updated BlockFactory types and methods

2628ce8

After robotology/blockfactory#33 some datatypes and interface methods changed

Removed spaces from Simulink Libraries block names

0764c03

Temporary CI fix

99fc2c3

diegoferigo force-pushed the feature/AlignWithBlockFactory branch from 1585307 to 99fc2c3 Compare January 31, 2019 14:06

traversaro reviewed Jan 31, 2019

View reviewed changes

traversaro approved these changes Jan 31, 2019

View reviewed changes

traversaro mentioned this pull request Jan 31, 2019

Automatically generate required Simulink library files robotology/blockfactory#10

Open

diegoferigo merged commit ccb1447 into robotology:devel Jan 31, 2019

diegoferigo deleted the feature/AlignWithBlockFactory branch January 31, 2019 15:30

diegoferigo added pr::resolution::merged and removed pr::status::feedback labels Jan 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align WBT and BlockFactory #168

Align WBT and BlockFactory #168

diegoferigo commented Jan 30, 2019

traversaro Jan 31, 2019

traversaro Jan 31, 2019

diegoferigo Jan 31, 2019

traversaro Jan 31, 2019

traversaro Jan 31, 2019

diegoferigo Jan 31, 2019

traversaro left a comment

diegoferigo commented Jan 31, 2019

traversaro Jan 31, 2019

diegoferigo Jan 31, 2019

traversaro Jan 31, 2019

diegoferigo Jan 31, 2019

diegoferigo Jan 31, 2019

traversaro Jan 31, 2019 •

edited

Loading

diegoferigo Jan 31, 2019

diegoferigo Jan 31, 2019

traversaro Jan 31, 2019

diegoferigo Jan 31, 2019

traversaro left a comment

diegoferigo commented Jan 31, 2019

	# The logic about library development is the following:
	#
	# * Developers should edit the library (e.g. changing block's masks) stored in the
	# matlab/library/WBToolboxLibrary_repository.mdl file.
	# * Developers should export the changes using the export_library custom target.
	# * Users will use the files installed from the exported/ folder.

Align WBT and BlockFactory #168

Align WBT and BlockFactory #168

Conversation

diegoferigo commented Jan 30, 2019

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

traversaro left a comment

Choose a reason for hiding this comment

diegoferigo commented Jan 31, 2019

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

traversaro Jan 31, 2019 • 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

traversaro left a comment

Choose a reason for hiding this comment

diegoferigo commented Jan 31, 2019

traversaro Jan 31, 2019 •

edited

Loading