Add rawValuesPublisherInterface and related nws and nwc to stream and save raw values #983

MSECode · 2024-09-18T16:17:39Z

This PR add the interface rawValuesPublisher and related network wrappers (client and server) to get the raw values and their metadata from the fw level (boards) and stream them to yarp ports so that they can be read on the port and be read by the telemetryDeviceDumper plugin so that a user can save them to a MATLAB file.
In this updates we have also modified iCubDev CMakeLists.txt to compile and installed the thrift generated files related to the new interface. (As already discussed with @Nicogene we need to revise these updates).
I've also add to the tools a module used initially for testing the whole pipeline without using the telemetryDeviceDumper.
This PR is, in fact, related to robotology/robometry#190
and as for that is still under review since it brings lots of changes and it's better to discuss some of the choices, such as the interface and its methods, to check if they have been designed as desired.

cc: @valegagge @Nicogene @traversaro

src/libraries/iCubDev/include/iCub/IRawValuesPublisher.h

traversaro · 2024-09-19T13:07:37Z

I am a bit afraid of depending on YARP 3.10 functionality until that is actually released, as this will prevent us to be able to release icub-main until YARP 3.10 is released. Do we have any specific plan to deal with this?

valegagge · 2024-09-20T06:57:50Z

I am a bit afraid of depending on YARP 3.10 functionality until that is actually released, as this will prevent us to be able to release icub-main until YARP 3.10 is released. Do we have any specific plan to deal with this?

ugh no! When we discussed this activity this issue was not pointed out. 😢
Now, I'm going to find a solution to this. Any suggestions are welcome. 😸

traversaro · 2024-09-20T08:20:45Z

ugh no! When we discussed this activity this issue was not pointed out. 😢

How did you tested the code? The master branch of superbuild and the conda package use YARP 3.9, so I guess you had to manually change something to be able to use the master branch of YARP (unless you were cloning and. compiling all the dependencies manually).

traversaro · 2024-09-20T08:21:42Z

Now, I'm going to find a solution to this. Any suggestions are welcome. 😸

Depending on the timeline for this we can try to understand with Randaz if there is an expected release date of YARP 3.10.

MSECode · 2024-09-20T16:38:01Z

ugh no! When we discussed this activity this issue was not pointed out. 😢

How did you tested the code? The master branch of superbuild and the conda package use YARP 3.9, so I guess you had to manually change something to be able to use the master branch of YARP (unless you were cloning and. compiling all the dependencies manually).

More or less you're right. Basically I've compiled the superbuild as unstable and then I've enabled the DEVEL mode for icub-main and YARP. And I was based on a stable branch on the master branch thus to use the paramParserGenerator.

MSECode · 2024-09-23T14:42:09Z

Now, I'm going to find a solution to this. Any suggestions are welcome. 😸

Depending on the timeline for this we can try to understand with Randaz if there is an expected release date of YARP 3.10.

I've updated the code so that we can locally use the paramParserGenerator on the nws and nwc and then, once we have the generated files and pushed them on the repo, we can disable that plugin so that we can temporarily let icub-main being not dependent by yarp 3.10

app/motorTemperaturePublisher/conf/config.ini

src/libraries/icubmod/fakeRawValuesPublisher/FakeRawValuesPublisher_params.md

valegagge · 2024-09-27T07:14:28Z

Hi @MSECode ,
I added the comment with some suggestions. please check it.

I still need to check embObjMotioncontrol

valegagge

Hi @MSECode , well done! you completed a very complex activity! 🚀

I request some changes that I can summarize in the following point:

move in cpp all the includes that are not necessary in .h file in to avoid unuseful dependency.
in EmbObJMotionControl add the function getBoardInfo() in the log prints in order to have an accurate log.
REmove all debug code or put under a macro compilation if you think you still need in some cases.
Verify if the client gets an error if it doesn't receive data for a configurable timeout.

thanks heap!

app/fakeRawDataPublisherTester/plotRawDataUpperArm.xml

src/libraries/iCubDev/include/iCub/rawValuesPublisher.xml

src/libraries/icubmod/embObjMotionControl/embObjMotionControl.cpp

src/libraries/icubmod/rawValuesPublisherServer/RawValuesPublisherServer.cpp

src/libraries/icubmod/rawValuesPublisherServer/RawValuesPublisherServer.h

src/tools/motorTemperaturePublisher/src/MotorTemperaturePublisher.h

src/tools/motorTemperaturePublisher/src/MotorTemperaturePublisher.cpp

valegagge · 2024-10-04T19:34:40Z

Hi @MSECode,
I think now all is ok except for:

a small improvement in embObjMotionControl.I suggest implementing the getRawDataMAp link in the FakeDevice relying on the getRawData_core function.
moving the header files from the .h to the implementation file of the client.
in the git stash you performed, you left a very huge commit message
thanks

This feature allows to publisher raw data coming from the sensor boards to yarp ports, so that they can be then collected by the TelemetryDeviceDumper and added to a MATLAB file. Together with the Interface we have the network wrappers server and client and the fake device that can be used for testing of new raw data Interface has been made exploiting thrift and wrappers use the paramparsergenerator Regarding this for now the genrated files are pushed and their re-generation is disabled waiting for yarp 3.10

MSECode · 2024-10-07T13:00:23Z

Hi @MSECode , well done! you completed a very complex activity! 🚀

I request some changes that I can summarize in the following point:

move in cpp all the includes that are not necessary in .h file in to avoid unuseful dependency.

in EmbObJMotionControl add the function getBoardInfo() in the log prints in order to have an accurate log.

REmove all debug code or put under a macro compilation if you think you still need in some cases.

Verify if the client gets an error if it doesn't receive data for a configurable timeout.

thanks heap!

Fixed all. I think now we can move the PR to ready. I'll double check everything but I suppose we are ok

pattacini · 2024-10-09T11:37:05Z

Thanks for your contribution @MSECode 👍🏻
I'll do a shallow review in the next couple of days. I'll try to be quick.

Nicogene

Few prelimar comments, I will give a second look soon

Nicogene · 2024-10-09T13:00:12Z

src/libraries/iCubDev/CMakeLists.txt

-                                                  "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>")
+                                                  "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include/iCub>"
+                                                  "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>"
+                                                  "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/iCub>")
 target_link_libraries(${PROJECT_NAME} YARP::YARP_os


Let's see if @traversaro has a better solution for this one we found

I think the problem is that the thrift classes do not have any namespace (that is true also for generated public classes that are just defined in the global namespace). Since robotology/yarp#2028, the include should be placed in include/<namespace>, so if we use the iCub namespace in thrift files we should be good to go and there will be no need to specifiy additional include directories as done here.

See https://github.com/robotology/icub-main/pull/983/files#r1793662892 .

If it's allowed by the automation, I'd like to segregate generated files in dedicated directories.

Sure, in that case you just need to have two directories passed as BUILD_INTERFACE interface, and only one install (I am not a big fan of using thrift-generated files in public interfaces due to this), something like:

target_include_directories(${PROJECT_NAME} PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>" "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/autogenerated/include>" "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>"

As per discussion w/ @valegagge and @Nicogene I'm gonna fix the CMakeLists.txt as proposed by @traversaro. Thus also the generated files should not be together with the hand-coded ones.

Thanks heaps!

Just one thing.
I tried to compile everything but without this line:
"$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/autogenerated>" in src/libraries/iCubDev/CMakeLists.txt
robometry compilation keeps failing with errors in finding the include:

[ 88%] Building CXX object src/telemetryDeviceDumper/CMakeFiles/yarp_telemetryDeviceDumper.dir/TelemetryDeviceDumper.cpp.o In file included from /home/iiticublap258/Workspace/robometry/src/telemetryDeviceDumper/TelemetryDeviceDumper.h:22, from /home/iiticublap258/Workspace/robometry/build/src/telemetryDeviceDumper/yarp_plugin_telemetryDeviceDumper.cpp:10: /home/iiticublap258/Workspace/robotology-superbuild/build/install/include/iCub/IRawValuesPublisher.h:9:10: fatal error: iCub/rawValuesDataVectorsMap.h: No such file or directory 9 | #include <iCub/rawValuesDataVectorsMap.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. [ 96%] Built target CB_to_matfile_example make[2]: *** [src/telemetryDeviceDumper/CMakeFiles/yarp_telemetryDeviceDumper.dir/build.make:76: src/telemetryDeviceDumper/CMakeFiles/yarp_telemetryDeviceDumper.dir/yarp_plugin_telemetryDeviceDumper.cpp.o] Error 1 make[2]: *** Waiting for unfinished jobs.... In file included from /home/iiticublap258/Workspace/robometry/src/telemetryDeviceDumper/TelemetryDeviceDumper.h:22, from /home/iiticublap258/Workspace/robometry/src/telemetryDeviceDumper/TelemetryDeviceDumper.cpp:8: /home/iiticublap258/Workspace/robotology-superbuild/build/install/include/iCub/IRawValuesPublisher.h:9:10: fatal error: iCub/rawValuesDataVectorsMap.h: No such file or directory 9 | #include <iCub/rawValuesDataVectorsMap.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[2]: *** [src/telemetryDeviceDumper/CMakeFiles/yarp_telemetryDeviceDumper.dir/build.make:90: src/telemetryDeviceDumper/CMakeFiles/yarp_telemetryDeviceDumper.dir/TelemetryDeviceDumper.cpp.o] Error 1 make[1]: *** [CMakeFiles/Makefile2:236: src/telemetryDeviceDumper/CMakeFiles/yarp_telemetryDeviceDumper.dir/all] Error 2 make: *** [Makefile:136: all] Error 2

src/libraries/iCubDev/include/iCub/IRawValuesPublisher.h

Nicogene · 2024-10-09T13:03:05Z

src/libraries/icubmod/embObjLib/FeatureInterface.cpp

@@ -170,7 +171,7 @@ eObool_t feat_manage_motioncontrol_addinfo_multienc(eOipv4addr_t ipv4, eOprotID3
    {
        multienc->update(id32, yarp::os::Time::now(), rxdata);
    }
-
+    */


Instead of commenting out, isn't better to remove it? There is a plan for re-enabling it?

I think there might be a plan to re-enabling it, even though it is probably gonna be removed soon as part of this: #929
But there might be a moment in which we need to re-enable it before removing. I'm not 100% sure about that.
@valegagge, do u have a more clear idea about this?

@MSECode you are right. My idea is to leave the code in a comment and remove it when we remove the multienc device (#929 ) in order to make the work clean.

src/libraries/icubmod/fakeRawValuesPublisher/FakeRawValuesPublisher_ParamsParser.cpp

traversaro · 2024-10-09T14:44:43Z

src/libraries/iCubDev/include/iCub/rawValuesPublisher.thrift

+# CopyPolicy: Released under the terms of the GNU GPL v2.0.
+#
+# rawValuePublisher.thrift
+


Suggested change

namespace yarp iCub

Adding a namespace here should ensure that all the classes are generated in the correct iCub namespace. See https://github.com/ami-iit/bipedal-locomotion-framework/blob/da3d948990fdab29daf8da9ccb90fb93a093804b/src/YarpUtilities/thrifts/BipedalLocomotion/YarpUtilities/VectorsCollection.thrift#L1 for an example of this (if you want more nested namespaces, you can nest them with iCub.debugLibrary.

(I updated the comment for more details).

pattacini

Hi @MSECode

Well done 👍🏻

I've got two main concerns though:

I don't think it's a good strategy to mix hand-coded files with generated files in the same directory. This happens for files located in iCubDev/include. Can you instead reserve a dedicated folder for generated files?
The names of the plugins that include yarp_. It's way better to remove the prefix in my view.

src/libraries/iCubDev/CMakeLists.txt

src/tools/fakeRawDataPublisherTester/src/FakeRawDataPublisherTester.cpp

src/libraries/iCubDev/CMakeLists.txt

MSECode · 2024-10-10T16:46:51Z

Hi @MSECode

Well done 👍🏻

I've got two main concerns though:

I don't think it's a good strategy to mix hand-coded files with generated files in the same directory. This happens for files located in iCubDev/include. Can you instead reserve a dedicated folder for generated files?

The names of the plugins that include yarp_. It's way better to remove the prefix in my view.

Hi @pattacini,
For point 1 --> I'll fix the struct of files and dir, let me know if its fine rn
For point 2 --> Sure, I'll remove that.

…make compilation better

pattacini · 2024-10-11T09:55:52Z

Thank you @MSECode 👍🏻
All is good with me.

The only point remaining is whether it is convenient or not to commit autogenerated files as per our T2T update, or maintain only the .thrift file.

pattacini · 2024-10-16T13:41:48Z

Another quite important point to consider is robotology/robometry#190 (comment).

cc @MSECode

pattacini · 2024-10-25T13:45:30Z

The only point remaining is whether it is convenient or not to commit autogenerated files as per our T2T update, or maintain only the .thrift file.

As discussed, it's ok to keep the autogenerated files committed to the repo 👍🏻

pattacini

Hi @MSECode

As discussed during today's meetup, two things have to be addressed before merging:

Increasing the minor number of icub-main release (major.minor.patch). I think minor is more appropriate than patch here.
Fixing the cmake installation of the generated headers in the correct path.

MSECode · 2024-10-28T08:15:06Z

Yes that's correct.
Basically the output of the meeting can be summed up in the following points:

as things now we are going to release this new feature as it is by just adding a small update on the CmakeLists.txt for iCubDev (I can install the thrift generated files related to IRawValuesPublisher interface in the same folder as the one for iCubDev files ) and upgrading the minor version for icub-main.
In the future we are gonna generate a new repository, which can be called something as iCubTechDebugInterfaces or similar where we are gonna add this and other new interfaces that are gonna come, as well as the wrapper devices related to the rawValuesPublisher.
Doing so robometry won't need to depend on the whole icub-main repo that is quite heavy but just on a smaller repo since we decided that it won't make sense to move it to icub-main or somewhere else
As soon as we are going to have the new repo ready, with the support of @traversaro, for the cmake part we will make the modifications described.
As soon as yarp 3.10 will be released we are going to remove the workarounds done for compiling
What said goes to clarify also the points opened for robometry PR, so we can move to ready that too.

This should be all. If I missed something please add.

Cc: @pattacini @Nicogene @traversaro @valegagge @S-Dafarra

pattacini · 2024-10-28T08:21:24Z

Pitch perfect 👍🏻

MSECode · 2024-10-28T11:52:19Z

Pushed fixed requested and checked compilation on a clean container and behavior on a setup.

MSECode marked this pull request as draft September 18, 2024 16:17

MSECode self-assigned this Sep 18, 2024

MSECode requested review from valegagge, traversaro and Nicogene September 18, 2024 16:18

pattacini self-requested a review September 19, 2024 05:39

traversaro reviewed Sep 19, 2024

View reviewed changes

src/libraries/iCubDev/include/iCub/IRawValuesPublisher.h Outdated Show resolved Hide resolved

pattacini reviewed Sep 25, 2024

View reviewed changes

app/motorTemperaturePublisher/conf/config.ini Outdated Show resolved Hide resolved

valegagge reviewed Sep 26, 2024

View reviewed changes

src/libraries/icubmod/fakeRawValuesPublisher/FakeRawValuesPublisher_params.md Outdated Show resolved Hide resolved

MSECode force-pushed the feature/rawValuesInterface branch 4 times, most recently from f45ae6f to a79fa4e Compare October 1, 2024 14:13

MSECode mentioned this pull request Oct 2, 2024

Feature/publish encoder raw values robotology/icub-firmware#520

Merged

MSECode force-pushed the feature/rawValuesInterface branch from 4aa8774 to f4f6953 Compare October 2, 2024 14:31

valegagge requested changes Oct 2, 2024

View reviewed changes

MSECode force-pushed the feature/rawValuesInterface branch from 2acd90b to a8f5d19 Compare October 4, 2024 15:56

MSECode force-pushed the feature/rawValuesInterface branch from a8f5d19 to 54a0125 Compare October 7, 2024 12:58

MSECode marked this pull request as ready for review October 8, 2024 13:51

MSECode requested a review from valegagge October 9, 2024 09:07

valegagge approved these changes Oct 9, 2024

View reviewed changes

Nicogene reviewed Oct 9, 2024

View reviewed changes

traversaro reviewed Oct 9, 2024

View reviewed changes

pattacini requested changes Oct 9, 2024

View reviewed changes

pattacini reviewed Oct 10, 2024

View reviewed changes

src/libraries/iCubDev/CMakeLists.txt Outdated Show resolved Hide resolved

Update CMakeLists.txt in iCubDev to re-roder files and dirs tree and …

054ebb5

…make compilation better

MSECode force-pushed the feature/rawValuesInterface branch from 771458e to 054ebb5 Compare October 10, 2024 17:10

pattacini requested changes Oct 25, 2024

View reviewed changes

Update iCubDev CMakeLists.txt and update icub-main to v2.7.0

9d82e00

MSECode requested a review from pattacini October 28, 2024 12:17

pattacini approved these changes Oct 28, 2024

View reviewed changes

pattacini merged commit 0e64379 into robotology:devel Oct 28, 2024
4 checks passed

MSECode mentioned this pull request Oct 28, 2024

Update ems v3.95, mc2plus v3.75, mc4plus v3.96, amc v2.10 robotology/icub-firmware-build#168

Merged

Nicogene mentioned this pull request Nov 13, 2024

Cleanup workaround in rawValues devices param parsers #991

Open

Nicogene mentioned this pull request Nov 28, 2024

Add couplingICubHandMk2 device coupling handler #995

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rawValuesPublisherInterface and related nws and nwc to stream and save raw values #983

Add rawValuesPublisherInterface and related nws and nwc to stream and save raw values #983

MSECode commented Sep 18, 2024

traversaro commented Sep 19, 2024

valegagge commented Sep 20, 2024

traversaro commented Sep 20, 2024

traversaro commented Sep 20, 2024

MSECode commented Sep 20, 2024

MSECode commented Sep 23, 2024

valegagge commented Sep 27, 2024 •

edited

Loading

valegagge left a comment •

edited

Loading

valegagge commented Oct 4, 2024 •

edited

Loading

MSECode commented Oct 7, 2024

pattacini commented Oct 9, 2024 •

edited

Loading

Nicogene left a comment

Nicogene Oct 9, 2024

traversaro Oct 9, 2024

traversaro Oct 9, 2024

pattacini Oct 9, 2024

traversaro Oct 9, 2024

MSECode Oct 10, 2024

pattacini Oct 10, 2024

MSECode Oct 10, 2024 •

edited

Loading

Nicogene Oct 9, 2024

MSECode Oct 10, 2024

valegagge Oct 12, 2024

traversaro Oct 9, 2024 •

edited

Loading

traversaro Oct 9, 2024

pattacini left a comment

MSECode commented Oct 10, 2024 •

edited

Loading

pattacini commented Oct 11, 2024 •

edited

Loading

pattacini commented Oct 16, 2024

pattacini commented Oct 25, 2024

pattacini left a comment •

edited

Loading

MSECode commented Oct 28, 2024

pattacini commented Oct 28, 2024

MSECode commented Oct 28, 2024

Add rawValuesPublisherInterface and related nws and nwc to stream and save raw values #983

Add rawValuesPublisherInterface and related nws and nwc to stream and save raw values #983

Conversation

MSECode commented Sep 18, 2024

traversaro commented Sep 19, 2024

valegagge commented Sep 20, 2024

traversaro commented Sep 20, 2024

traversaro commented Sep 20, 2024

MSECode commented Sep 20, 2024

MSECode commented Sep 23, 2024

valegagge commented Sep 27, 2024 • edited Loading

valegagge left a comment • edited Loading

Choose a reason for hiding this comment

valegagge commented Oct 4, 2024 • edited Loading

MSECode commented Oct 7, 2024

pattacini commented Oct 9, 2024 • edited Loading

Nicogene 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

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

MSECode Oct 10, 2024 • 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

traversaro Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattacini left a comment

Choose a reason for hiding this comment

MSECode commented Oct 10, 2024 • edited Loading

pattacini commented Oct 11, 2024 • edited Loading

pattacini commented Oct 16, 2024

pattacini commented Oct 25, 2024

pattacini left a comment • edited Loading

Choose a reason for hiding this comment

MSECode commented Oct 28, 2024

pattacini commented Oct 28, 2024

MSECode commented Oct 28, 2024

valegagge commented Sep 27, 2024 •

edited

Loading

valegagge left a comment •

edited

Loading

valegagge commented Oct 4, 2024 •

edited

Loading

pattacini commented Oct 9, 2024 •

edited

Loading

MSECode Oct 10, 2024 •

edited

Loading

traversaro Oct 9, 2024 •

edited

Loading

MSECode commented Oct 10, 2024 •

edited

Loading

pattacini commented Oct 11, 2024 •

edited

Loading

pattacini left a comment •

edited

Loading