ControlBoardWrapper2 ROS topic

barbalberto commented 10 years ago

Since some people uses both YARP and ROS, we tought to have the controlBoardWrapper2 optionally create a ROS topic along the usual yarp port.

I've done a first implementation of this and now it is able to create a topic publiching a sensor_msgs/JointState message which is one of the 'native' ROS messages documented here http://docs.ros.org/api/sensor_msgs/html/msg/JointState.html

I used a .msg file to generate the class that will be sent through the topic The data sent are

list of joint name (useless on yarp, but needed on ROS)
vector of encoders
vector os velocities
vector of torques

For now the data is filled with dummy names and numbers just to see if it works as expected.

I pushed the code on a branch called ControlBoardWrapper2_NewOutputPort. The CMake flag CBW2_USE_ROS_MSG has to be enabled in order to compile the needed code and generate this topic.

It has been tested with a simple subscriber create by following the tutorial on ROS webpage (http://wiki.ros.org/ROS/Tutorials/ExaminingPublisherSubscriber) so not using yarp code, btw yarp read is able to read the data back if the yarpidl_rosmsg service is running like explained in the page http://wiki.icub.org/yarpdoc/yarp_with_ros.html

TODO:

define exactly with type of ros message could be more useful to implement
fix segfault while closing (it is still WIP ... )

traversaro commented 10 years ago

@miaragao worked a lot on ROS/YARP integration on the iCub, I guess he could be interested in this enhancement, and can provide feedback on useful features for the users.

@EnricoMingo @arocchi With a similar option on the ForceTorque/IMU wrappers you could get rid of your bridges.

barbalberto commented 10 years ago

yes, potentially this can be used for all wrappers. The only thing is that robotInterface creates ports, and therefore also topics, for each part of the body while it seems that ROS have one topic for all the robot. This is something we have to discuss about maybe?

paulfitz commented 10 years ago

Ports can publish to the same topic, if you want them to.

mikearagao commented 10 years ago

Hi all! First I'm sorry for the delay, I have been away on vacation.

This could actually be really useful for my work. Right now, on ROS side I have the joint names hard coded, and this would solve that problem for example (although the joints names on the URDF model doesn't match the names on the iCub robot right now - this will be fixed). This is just one of the examples. I will continue working with/on the integration of YARP/ROS so I am indeed interested on this work.

Actually I would like to try this really soon in order to solve the problem with the joint names.

@barbalberto about your TODO list:

"define exactly with type of ros message could be more useful to implement"

I can't tell you exactly what I might need here (as a user of the YARP/ROS integration), but the joint names list is definitely useful. I hope I can help you on this as my work progresses.

Cheers!

barbalberto commented 10 years ago

@paulfitz: can you help me with the idl_ros generation? I was updating the branch to new yarp head and the ros generation does not work anymore. The result I get is shown in the picture, on the right there is the build folder containing only the headers file but no .cpp; in the left side is the src folder, the one given to yarp_idl_to_dir function, and this one contains only one include file. rosmsg Also the variable I get back from the function are not correctly set. I'm following this tutorial http://wiki.icub.org/yarpdoc/rostypes_tutorial_portable.html

What am I doing wrong? The code is in the branch ControlBoardWrapper2_NewOutputPort, the device involved is the ControlBoardWrapper2. In this branch, the cmake flag CBW2_USE_ROS_MSG will enable the compilation of the rosmsg; the thrift version works correctly.

The funny things is that if I activate the TEST_yarpidl_rosmsg and TEST_yarpidl_thrift, it says that the thrift fails and rosmsg succed...

paulfitz commented 10 years ago

Hi @barbalberto, looks like on that branch there is a modification to conf/template/placeGeneratedYarpIdlFiles.cmake.in in 2edbb6f831a0d24ab0c1998eba34761401ac4691 that is being disruptive. It may be a fix to something that got fixed independently in master, and now the two are conflicting.

barbalberto commented 10 years ago

you mean the add of @output_dir@ ? I did it because it was helpful when I firstly implemented, however if I reset that file as it is on the master, I don't see any changes.

paulfitz commented 10 years ago

Yes that's what I mean @barbalberto. Is TEST_yarpidl_thrift still failing for you?

barbalberto commented 10 years ago

yes, the one failing is the idl_thrift_demo_test. To be sure I just cloned a new clean repo and compiled on the master branch and still fails. 1/59 Test #1: idl_thrift_demo_base ............... Passed 0.01 sec Start 2: idl_thrift_demo_config 2/59 Test #2: idl_thrift_demo_config ............. Passed 0.76 sec Start 3: idl_thrift_demo_make 3/59 Test #3: idl_thrift_demo_make ............... Passed 9.27 sec Start 4: idl_thrift_demo_test 4/59 Test #4: idl_thrift_demo_test ...............***Failed 0.03 sec Start 5: idl_rosmsg_demo_base 5/59 Test #5: idl_rosmsg_demo_base ............... Passed 0.01 sec Start 6: idl_rosmsg_demo_config 6/59 Test #6: idl_rosmsg_demo_config ............. Passed 0.73 sec Start 7: idl_rosmsg_demo_make 7/59 Test #7: idl_rosmsg_demo_make ............... Passed 0.76 sec Start 8: idl_rosmsg_demo_test 8/59 Test #8: idl_rosmsg_demo_test ............... Passed 0.02 sec

For the ros part the behaviour does not change with or without the @output_dir@, the .cpp files are not generated.

paulfitz commented 10 years ago

@barbalberto can you do me a favor and do ctest -V idl_thrift (edit: ctest -V -R idl_thrift) and pastebin the results?

barbalberto commented 10 years ago

Here it is:

UpdateCTestConfiguration  from :/usr/local/src/robot/test/yarp/build/DartConfiguration.tcl
UpdateCTestConfiguration  from :/usr/local/src/robot/test/yarp/build/DartConfiguration.tcl
Test project /usr/local/src/robot/test/yarp/build
Constructing a list of tests
Done constructing a list of tests
Checking test dependency graph...
Checking test dependency graph end
test 1
      Start  1: idl_thrift_demo_base

1: Test command: /usr/local/src/robot/test/yarp/build/bin/yarpidl_thrift "-out" "/usr/local/src/robot/test/yarp/build/src/idls/thrift/tests" "--gen" "yarp" "/usr/local/src/robot/test/yarp/src/idls/thrift/tests/demo/demo.thrift"
1: Test timeout computed to be: 9.99988e+06
 1/59 Test  #1: idl_thrift_demo_base ...............   Passed    0.01 sec
test 2
      Start  2: idl_thrift_demo_config

2: Test command: /usr/bin/cmake "-DALLOW_IDL_GENERATION=TRUE" "-DYARP_DIR=/usr/local/src/robot/test/yarp/build" "/usr/local/src/robot/test/yarp/src/idls/thrift/tests/demo"
2: Test timeout computed to be: 9.99988e+06
2: -- thrift code for demo.thrift => /usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo
2: -- thrift code for namespaced.thrift => /usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo
2: -- thrift code for objects3D.thrift => /usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo
2: -- thrift code for wrapping.thrift => /usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo
2: -- Configuring done
2: -- Generating done
2: -- Build files have been written to: /usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo
 2/59 Test  #2: idl_thrift_demo_config .............   Passed    0.15 sec
test 3
      Start  3: idl_thrift_demo_make

3: Test command: /usr/bin/cmake "--build" "/usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo"
3: Test timeout computed to be: 9.99988e+06
3: Scanning dependencies of target demo_test
3: [  5%] Building CXX object CMakeFiles/demo_test.dir/main.cpp.o
3: /usr/local/src/robot/test/yarp/src/idls/thrift/tests/demo/main.cpp: In function ‘bool test_enums()’:
3: /usr/local/src/robot/test/yarp/src/idls/thrift/tests/demo/main.cpp:292:57: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘std::vector<DemoEnum>::size_type {aka long unsigned int}’ [-Wformat=]
3:      printf("lst1 %d lst2 %d\n", lst1.size(), lst2.size());
3:                                                          ^
3: /usr/local/src/robot/test/yarp/src/idls/thrift/tests/demo/main.cpp:292:57: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘std::vector<DemoEnum>::size_type {aka long unsigned int}’ [-Wformat=]
3: [ 10%] Generating demo_thrift.cmake, usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/include/DemoEnum.h, usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/include/DemoStruct.h, usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/include/DemoStructList.h, usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/include/DemoStructExt.h, usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/include/Demo.h, usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/src/DemoEnum.cpp, usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/src/DemoStruct.cpp, usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/src/DemoStructList.cpp, usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/src/DemoStructExt.cpp, usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/src/Demo.cpp
3: [ 15%] Building CXX object CMakeFiles/demo_test.dir/usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/src/DemoEnum.cpp.o
3: c++: error: /usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/src/DemoEnum.cpp: No such file or directory
3: c++: fatal error: no input files
3: compilation terminated.
3: make[2]: *** [CMakeFiles/demo_test.dir/usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/src/DemoEnum.cpp.o] Error 4
3: make[1]: *** [CMakeFiles/demo_test.dir/all] Error 2
3: make: *** [all] Error 2
 3/59 Test  #3: idl_thrift_demo_make ...............***Failed    0.93 sec
test 4
      Start  4: idl_thrift_demo_test

4: Test command: /usr/bin/ctest
4: Test timeout computed to be: 9.99988e+06
4: Test project /usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo
4:     Start 1: demo_basic
4: 1/1 Test #1: demo_basic .......................   Passed    0.08 sec
4: 
4: 100% tests passed, 0 tests failed out of 1
4: 
4: Total Test time (real) =   0.09 sec
 4/59 Test  #4: idl_thrift_demo_test ...............   Passed    0.10 sec
test 5
      Start  5: idl_rosmsg_demo_base

5: Test command: /usr/local/src/robot/test/yarp/build/bin/yarpidl_rosmsg "/usr/local/src/robot/test/yarp/src/idls/rosmsg/tests/demo/Demo.msg" "--out" "/usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests" "--gen" "yarp"
5: Test timeout computed to be: 9.99988e+06
 5/59 Test  #5: idl_rosmsg_demo_base ...............   Passed    0.01 sec
test 6
      Start  6: idl_rosmsg_demo_config

6: Test command: /usr/bin/cmake "-DALLOW_IDL_GENERATION=TRUE" "-DYARP_DIR=/usr/local/src/robot/test/yarp/build" "/usr/local/src/robot/test/yarp/src/idls/rosmsg/tests/demo"
6: Test timeout computed to be: 9.99988e+06
6: -- rosmsg code for Demo.msg => /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo
6: [type] BEGIN Demo.msg
6: [type]   int32 x []
6: [type]   int8 a_signed_byte []
6: [type]   uint8 an_unsigned_byte []
6: [type]   int16 a_signed_byte2 []
6: [type]   uint16 an_unsigned_byte2 []
6: [type]   int32 a_signed_byte4 []
6: [type]   uint32 an_unsigned_byte4 []
6: [type]   int64 a_signed_byte8 []
6: [type]   uint64 an_unsigned_byte8 []
6: [type]   string str []
6: [type] END Demo.msg
6: Generating /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo/_yarp_idl_//demo/Demo.h
6: -- rosmsg code for Tennis.srv => /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo
6: [type] BEGIN Tennis.srv
6: [type]   int32 x []
6: [type]   int32 y []
6: [type]   int32 response []
6: [type] END Tennis.srv
6: --- reply ---
6: Generating /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo/_yarp_idl_//tennis/TennisReply.h
6: Generating /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo/_yarp_idl_//tennis/Tennis.h
6: -- rosmsg code for Rpc.srv => /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo
6: [type] BEGIN Rpc.srv
6: [type]   int8 PIPPO [1]
6: [type]   int8 PLUTO [2]
6: [type]   int8 pluppo []
6: [type]   string msg []
6: [type]   uint32 CODE [54321]
6: [type]   string val []
6: [type]   uint32 an_int []
6: [type] END Rpc.srv
6: --- reply ---
6: Generating /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo/_yarp_idl_//rpc/RpcReply.h
6: Generating /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo/_yarp_idl_//rpc/Rpc.h
6: -- rosmsg code for SharedData.msg => /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo
6: [type] BEGIN SharedData.msg
6: [type]   string text []
6: [type]   float64[] content []
6: [type] END SharedData.msg
6: Generating /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo/_yarp_idl_//shareddata/SharedData.h
6: -- Configuring done
6: -- Generating done
6: -- Build files have been written to: /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo
 6/59 Test  #6: idl_rosmsg_demo_config .............   Passed    0.14 sec
test 7
      Start  7: idl_rosmsg_demo_make

7: Test command: /usr/bin/cmake "--build" "/usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo"
7: Test timeout computed to be: 9.99988e+06
7: Scanning dependencies of target demo_test
7: [ 25%] Generating Rpc_srv.cmake, usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo/include/Rpc.h, include/RpcReply.h
7: [type] BEGIN Rpc.srv
7: [type]   int8 PIPPO [1]
7: [type]   int8 PLUTO [2]
7: [type]   int8 pluppo []
7: [type]   string msg []
7: [type]   uint32 CODE [54321]
7: [type]   string val []
7: [type]   uint32 an_int []
7: [type] END Rpc.srv
7: --- reply ---
7: Generating /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo/_yarp_idl_//rpc/RpcReply.h
7: Generating /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo/_yarp_idl_//rpc/Rpc.h
7: [ 50%] Building CXX object CMakeFiles/demo_test.dir/main.cpp.o
7: Linking CXX executable demo_test
7: [100%] Built target demo_test
 7/59 Test  #7: idl_rosmsg_demo_make ...............   Passed    0.53 sec
test 8
      Start  8: idl_rosmsg_demo_test

8: Test command: /usr/bin/ctest
8: Test timeout computed to be: 9.99988e+06
8: Test project /usr/local/src/robot/test/yarp/build/src/idls/rosmsg/tests/make/demo
8:     Start 1: demo_basic
8: 1/1 Test #1: demo_basic .......................   Passed    0.00 sec
8: 
8: 100% tests passed, 0 tests failed out of 1
8: 
8: Total Test time (real) =   0.01 sec
 8/59 Test  #8: idl_rosmsg_demo_test ...............   Passed    0.02 sec
test 9
      Start  9: OS::PortTest
[snip]

paulfitz commented 10 years ago

thanks @barbalberto, that's really odd... Those usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/usr/local/src/robot/test/yarp/build/src/idls/thrift/tests/make/demo/src/DemoEnum.cpp paths look super funky, like the output path is getting included twice.

paulfitz commented 10 years ago

@barbalberto I can replicate the failure in https://github.com/robotology/yarp/issues/209#issuecomment-59387078 by including the change you made to conf/template/placeGeneratedYarpIdlFiles.cmake.in in 2edbb6f831a0d24ab0c1998eba34761401ac4691 - are you sure you got rid of that?

barbalberto commented 10 years ago

Oops, you are right. I thought I've removed it but apparently was still there, sorry about that :( Removing it made the test work fine, but still I'm not able to make my ros msg to work, which is my main concern.

if I print the variable full_headers in the YARP_IDL_TO_DIR_CORE function, I see just one header file (in my case joint.h) but Header.h and TickTime.h are missing and therefore not copied to src folder. The compilation fails jointState.h tries to include TickTime.h

The joint.h is correctly copied in the right place so that's fine.

barbalberto commented 10 years ago

I tried the example here http://wiki.icub.org/yarpdoc/rostypes_tutorial_portable.html and it works

Could the problem come from the fact that I'm using it inside yarp itself? maybe too early? In order to have the yarp_idl functions to work I had to add a find_package yarp, which it sound kind of weird to me.

find_package(YARP REQUIRED)
list(APPEND CMAKE_MODULE_PATH ${YARP_MODULE_PATH}) include(YarpIDL)

maybe cmake falls in some wrong loop or has some variable not correctly initialized?

paulfitz commented 10 years ago

@barbalberto looks like the code for the special built-in ROS Header type (which includes in turn a reference to a built-in time type) is not getting copied into place. Working on a test case, then a fix.

paulfitz commented 10 years ago

@barbalberto the Header type should be generating ok now, and I made a tweak so that yarp_idl_to_dir is easier to get at while building yarp (you weren't seeing it because it is known as yarp_idl_to_dir_core internally). You shouldn't need that find_package(YARP) now. Btw, could you move the generated file directory to somewhere in CMAKE_CURRENT_BINARY_DIR rather than in the source tree? There would be no reason to commit the generated code in this case.

barbalberto commented 10 years ago

Ok, now the files are correctly installed in the fodler specified in the yarp_idl_to_dir! Thank you Paul.

I used CMAKE_CURRENT_SOUCE_DIR just because all the tutorials are done like this so I wanted to stick with them at first, but yes, my idea was also to have the generated files in the build folder in the final version. As a side note, if I put CMAKE_CURRENT_BINARY_DIR in the yarp_idl_to_dir function, then the build folder will have the generated files twice, one in the CMAKE_CURRENT_BINARY_DIR/_yarpidl folder (as a temporary location) and one in the CMAKE_CURRENT_BINARY_DIR/include, after the 'installation', but it does not hurt I guess.

Now I'm facing a last problem: if I try to compile using the thrift from a clean repository, the configure step of cmakes calls the thrift generator, which is not built yet. This is the error I get: CMake Error at conf/YarpIDL.cmake:79 (message): yarpidl_rosmsg (YARPIDL_rosmsg_LOCATION-NOTFOUND) failed, aborting. Call Stack (most recent call first): conf/YarpIDL.cmake:124 (YARP_IDL_TO_DIR_CORE) src/libYARP_dev/src/modules/ControlBoardWrapper/CMakeLists.txt:53 (yarp_idl_to_dir)

So right now I should first compile yarp repository telling my device to not use the ros/yarp thrift msg and then, as a second step, re-run cmake enabling the rosmsg generator and then it works.

just adding "add_dependencies(YARP_dev yarpidl_rosmsg yarpidl_thrift )" to my device, doesn't help.

I see in the ros/test you create several targets for generating / compiling / testing ... shall I do something similar in this case?

paulfitz commented 10 years ago

No, please don't copy the test targets for generating/compiling/running, they are just there to test generating/compiling/running, not because that's a good way to structure your build. It is in fact very delicate, with is why those tests don't run by default.

At some point @elen4 took a pass at making the IDL machinery easier to use, and one of the outcomes of that was to run it at configuration time, not just build time. That means that using it while compiling yarp itself is a little awkward. Fixable, but awkward. I'll poke at it.

lornat75 commented 10 years ago

@paulfitz this is an interesting chicken-egg problem :) ideas on how to solve it?

elen4 commented 10 years ago

Hi @barbalberto I pushed a fix for the macro to the ControlBoardWrapper2_NewOutputPort branch on my fork, could you try it out? As for generating code inside the source tree, the original idea to transition to IDLs was to commit generated code as well while the IDL generators were optional and not stable at all. Of course this is not the case within YARP, since generators are now on by default, and you can assume that you will always be able to generate the code during your configuration time. The tricky point I see here is the use of the ALLOW_IDL_GENERATION option, that @barbalberto for now enables in his CMakeLists, but I suppose at some point it should be just enabled "globally" by default for the YARP repo.

drdanz commented 10 years ago

The problem is that add_custom_command uses the executable is "found" using

 find_program(YARPIDL_${family}_LOCATION yarpidl_${family} HINTS ${YARP_IDL_BINARY_HINT})

It should instead use the target that creates the executable. This should handle all the dependencies automatically:

According to CMake 3.0.2 documentation

If COMMAND specifies an executable target (created by ADD_EXECUTABLE) it will automatically be replaced by the location of the executable created at build time. Additionally a target-level dependency will be added so that the executable target will be built before any target using this custom command. However this does NOT add a file-level dependency that would cause the custom command to re-run whenever the executable is recompiled.

See: http://www.cmake.org/cmake/help/v3.0/command/add_custom_command.html

For external programs using this macro, I think the target executable should be "exported" together with the libraries, so that CMake knows the target after find_package(YARP), since the YARPConfig.cmake file will have statements like:

 add_executable(<name> IMPORTED [GLOBAL])

See:

I'm not sure if CMake 2.8.9 supports all of this, though, but I think it does since add_executable supports the IMPORTED flag, see http://www.cmake.org/cmake/help/v2.8.9/cmake.html#command:add_executable

barbalberto commented 10 years ago

@elen4: I'm not sure to understand what the patch in the YarpIDL.cmake file should do. Is it a better error message in case yarp thrift is not found? ( I already removed the need of find_package(YARP) after the last commmit of Paul.

I used the ALLOW_IDL_GENERATION just for convenience and I meant to remove it. Will this make things easier?

So as far as I understand, there are 2 ways to fix this issue.

generate the file once, commit them in the source tree and ignore the generation from then on. If the .thrift file will change in the future, the new version of the generated files should be pushed as well.
set the generator as a dependency of the module using the .thrift file forcing to compile the generator before the module (and issue the generation command in between and not at cmake configure time). Am I right? Seems both cool and complicated to me. Could we could get into trouble if the generator in turn depends on yarp_dev? :$ (althought it doesn't seems the case)

I know how to do the first solution, not the second one but I can check the links @drdanz provided if we chose this way.

paulfitz commented 10 years ago

For @barbalberto's bullet point 2: it is straightforward to get the generator built before libYARP_dev builds, but it is not straightforward to get the generator built before libYARP_dev is configured. To be able to configure libYARP_dev, we need to know the list of files in it, and to complete that list, we currently need the generator (or to follow bullet point 1).

There is one other option: we could have the generator produce a list of source/header files in a file, and commit that file (rather than the generated source/header files themselves). That list is all we need during configuration. The generator already produces such a file, with a bit of clean up we could do the following:

We have foo.thrift, we use it with yarp_idl_to_dir(foo.thrift ....)
- Works, but not from a clean build, since generator needed at configure time
Generator produces a foo.idl file with source/header inventory and name of foo.thrift
We commit foo.idl alongside foo.thrift, and change to using yarp_idl_to_dir(foo.idl ...)
- Works from a clean build, since generator no longer needed at configure time

The foo.idl file might look like this (just a regular cmake file so we can read it by include()ing it):

set(IDL_ORIGIN foo.thrift)
set(IDL_SOURCES src/Thing1.cpp src/Thing2.cpp)
set(IDL_HEADERS include/Thing1.h include/Thing2.h)

elen4 commented 10 years ago

@barbalberto: sorry, I though that current code was placing generated files inside the build dir, and I didn't realize I wasn't testing properly. Anyway, the idea in my commit was actually skipping the configure-time generation, and only leave the build-time command (and related target), to add as a dependency to YARP_dev

if (CBW_USE_YARP_THRIFT)
    add_dependencies(YARP_dev stateExt_thrift)
endif()

using the "target" generator in the command, as @drdanz also pointed out. Then, I was thinking of adding the explicit list of generated files instead of including stateExt_thrift.cmake (on the lines of what @paulfitz is suggesting), and do a

set_source_files_properties(<list_of_generated_files> PROPERTIES GENERATED TRUE)

which I think is a reasonable assumption inside Yarp (I mean, the list of generated files can be expected to always be up-to-date with the IDL generator, since they are in the same repo). The problem with the current code is that files get generated in one place and are used in another one, and the set_source_files_properties command mentioned above does not seem to work in this case, but with some clean-up you should be able to sort it out :smile:

lornat75 commented 10 years ago

As @elen4 says the idea was initially to implement @barbalberto's bullet point 1 solution, to avoid exposing to users the complexity of generated code. I know it is not a good practice to commit generated code but I would be inclined to consider it acceptable in this case, it seems that generating it on the fly adds a good amount of complexity. What do you guys think?

If we change where the thrift generator places generated files let's not forget to update online tutorials!

pattacini commented 10 years ago

I'm in favor of keeping the generated code committed in the repo: for icub-main and wysiwyd projects is already like that and it's fine. Besides the complexity mentioned by @lornat75, I'd say that committed code represents also a sort of buffer that decouples changes made to yarp-idl components from the sister projects so that the final user can always decide whether to include or not those changes to idl code, keeping things under control and being aware of what is going on.

Conversely, if for any reason the final user wants to incorporate new idl features straightaway, he can set the ALLOW_IDL_GENERATION cmake option always to ON (is it cached?).

barbalberto commented 10 years ago

Well, in any case solution 1 is easy to undo. If we find a reliable way to solve the issue, we can later change the way those files are handled. In the meanwhile we have something to start with for testing and debugging.

paulfitz commented 10 years ago

Yes, in this case I'd be ok with committing the generated files. It felt unnecessary since this case is in the yarp library so doesn't really need the decoupling @pattacini mentions. But we don't seem to be converging on a simple alternative.

pattacini commented 10 years ago

@paulfitz indeed I was more referring to sister projects rather than yarp, but yes it seems there is no a unique approach

elen4 commented 10 years ago

Sorry, I think I created some confusion here :confused: I was just trying to clarify why the tutorials (at least the Thrift ones I wrote) suggest to place generated files inside the source tree (i.e. the decoupling, especially in the past when IDL generators were not enabled by default), even if it's not the best practice. By the way, let me just point out that the yarp_idl_to_dir macro makes the generator create the files always inside the build dir, then copies them to the user-defined destination, so there is no need to change the generator or the macro - what the tutorials say only depends on the practice you would like to enforce, and from @pattacini 's comment the current approach seems fine for users. As for the YARP repository itself, the only issue I see for committing generated code is if you assume that some users will disable the compilation of the IDL generators; otherwise, you could just generate files on-the-fly, and the only difference would be that YARP developers would have to mantain an explicit list of the generated files instead of relying on the list the generator creates. IMHO it's not too much of a burden for a YARP developer :sweat_smile:

barbalberto commented 9 years ago

ROS message is created with the following fields

name; position; velocity; effort;

Where name is filled with "rosNameTest" plus joint number. How do we get the joint name? From config file I guess... Suggestion @arocchi @EnricoMingo? Should this name match the one in Gazebo? @traversaro

traversaro commented 9 years ago

In my opinion, we should get the name of the joint (Axis) from the IAxisInfo interface. If this interface is not available, we should fallback to the current solution.

In this way we will continue to decouple the wrapper and the underlying device. For CanBusMotionControl and embObjMotorControl I propose we use the joint names that are present in http://wiki.icub.org/wiki/ICub_joints ( more discussions on this topic in https://github.com/robotology/icub-main/issues/57 ).

barbalberto commented 9 years ago

Pull request merged into master. Good point the IAxisInfo interface, I'll open a issue on that.

robotology / yarp

ControlBoardWrapper2 ROS topic #209