robotology / robometry

Telemetry suite for logging data from your robot 🤖
https://robotology.github.io/robometry
Other
14 stars 9 forks source link

Add to TelemetryDeviceDumper streaming of rawValues from low level #190

Closed MSECode closed 2 weeks ago

MSECode commented 2 months ago

Add to the telemetryDeviceDumper a device based on the nwc rawValuesPublisherClient thus to save in the MATLAB file generated by this plugin the raw values (and whatever one desired for debugs based on the Interface iCub::debugLibrary::IRawValuesPublisher) coming from the boards at the fw level.

Side Note: I'll open this PR since I've tested the update done on my bench setup. However, the work, even though at a good point, cannot be considered fully completed. The PR will be kept in draft for now and used for discussion on the updates done and to revise them together

cc: @valegagge @Nicogene @traversaro

S-Dafarra commented 1 month ago

To be honest, I believe that the dependency on iCub is not the best here, as it would complexify its usage with other robots. An alternative could be to instead create a similar device in icub-main instead

valegagge commented 1 month ago

To be honest, I believe that the dependency on iCub is not the best here, as it would complexify its usage with other robots. An alternative could be to instead create a similar device in icub-main instead

Hi @S-Dafarra , I don't understand your suggestion, sorry! :( The new device belongs to icub-main, so here we add the dependence from it.

valegagge commented 1 month ago

Hi @MSECode , for me is ok. I found a small bug related to the use of logControlBoardQuantities with falsevalue, but it seems not dependent on your changes. I fixed here. Asap I'll put the code (I need the permission.)

S-Dafarra commented 1 month ago

The new device belongs to icub-main, so here we add the dependence from it.

That's the problem, the new dependency from icub-main, while it should be the other way around. In other words, it should be icub-main to depend from robometry, and this can be done by moving or copying the TelemetryDeviceDumper device into icub-main.

Nicogene commented 1 month ago

The new device belongs to icub-main, so here we add the dependence from it.

That's the problem, the new dependency from icub-main, while it should be the other way around. In other words, it should be icub-main to depend from robometry, and this can be done by moving or copying the TelemetryDeviceDumper device into icub-main.

Note that the dependency from icub-main would be added only if telemetryDeviceDumper is compiled, and it is disabled by default.

In any case, I am more comfortable to move it eventually in icub-main, rather than have yet another logger device around

S-Dafarra commented 1 month ago

In any case, I am more comfortable to move it eventually in icub-main, rather than have yet another logger device around

I agree! Your call anyway 👍

Nicogene commented 1 month ago

Another possibility is to add a find_package(iCub QUIET) and then use a ifdef in the telemetryDeviceDumper code, this would keep it compilable also without icub-main

valegagge commented 1 month ago

Hi @Nicogene and @S-Dafarra , I'm sorry for not getting back to you sooner.

@S-Dafarra : when we decided to add this functionality we knew we were adding a dependency to icub-main, but agreed with @Nicogene we decided to go ahead. As you point out maybe it is too strong to make this dependency permanent. If I can post my few cents, I think we can keep the dependency from Telemetrydumper optional as @Nicogene suggested in the previous comment.

What do you think?

S-Dafarra commented 1 month ago

If I can post my few cents, I think we can keep the dependency from Telemetrydumper optional as @Nicogene suggested in the previous comment.

What do you think?

I believe it is not ideal. If in a near future we decide to enable it by default, this can increase the compilation time of the supebuild for example, as everything that depends on robometry will need to wait for icub-main to be compiled, that in turn depends on yarp. The other way around would make it smoother

valegagge commented 1 month ago

Hi @MSECode , this is the commit of the fis I mentioned here.

MSECode commented 1 month ago

Hi @MSECode , this is the commit of the fis I mentioned here.

Yes, I saw it and squashed to 1 single commit for the PR. Let's now decide how to managed the CMakeLists.txt file for what said in the thread and eventually add the fixes and proceed with the PR. Is it OK?

valegagge commented 1 month ago

As discussed with @MSECode and @Nicogene , we'll organize a meeting with all the involved people to define if moving the device to icub-main or not or, in any case, point out the best solution.

S-Dafarra commented 1 month ago

Let me add that the device is already dependent on yarp, as it should be, so in any case, we already need for yarp to be compiled before. In general, I keep not liking to have icub-main as a dependency here because it hinders any usage of robometry within icub-main. Still, I don't want to make a big fuzz out of this, so feel free to proceed with the solution you feel more suitable.

valegagge commented 1 month ago

I created a meeting to address this discussion.

pattacini commented 1 month ago

I didn't follow this discussion in the first place, but probably a better place for the I/F's would have been YARP itself. The implementation of the devices can stay in icub-main, but we usually put the I/F's in YARP right to address this kind of situation.

At any rate, we can discuss this topic with the ad-hoc meeting you scheduled.

traversaro commented 1 month ago

I didn't follow this discussion in the first place, but probably a better place for the I/F's would have been YARP itself. The implementation of the devices can stay in icub-main, but we usually put the I/F's in YARP right to address this kind of situation.

At any rate, we can discuss this topic with the ad-hoc meeting you scheduled.

We can discuss in person, but the idea was to have the interface somewhere maintained by iCub Tech to be able to quicky iterate if necessary.