Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup workaround in rawValues devices param parsers #991

Closed
Nicogene opened this issue Nov 13, 2024 · 8 comments · Fixed by #1006
Closed

Cleanup workaround in rawValues devices param parsers #991

Nicogene opened this issue Nov 13, 2024 · 8 comments · Fixed by #1006
Assignees

Comments

@Nicogene
Copy link
Member

In:

the rawValues debug sw pipeline has been added (interfaces, nwc, nws) to icub-main, and the devices exploit the new feature of param parsing generation of YARP.

Since YARP was not released yet @MSECode added manually the header needed, but since that now YARP has been released:

This workaround can be removed, and icub-main can require 3.10.0 as yarp required version.

cc @valegagge

@MSECode
Copy link
Contributor

MSECode commented Jan 8, 2025

Cleaned up the workaround with this commit: MSECode@208ad39

Ready for the PR. To clean old modified files I'll pushed just this time the generated files.

@MSECode
Copy link
Contributor

MSECode commented Jan 13, 2025

Hi @Nicogene,
should I enable the plugins of the rawValuesPublisher by default or should I keep them OFF when building icub-main by default, what do you think?
See this line in the PR: https://github.com/robotology/icub-main/pull/1006/files#diff-934ad2319d5a8a8b5b1f397926d0de21a9cacc9660734c9d9fb22dba954ba55aL11

@pattacini pattacini linked a pull request Jan 13, 2025 that will close this issue
@pattacini
Copy link
Member

Hi @MSECode

Turn it OFF, please.

@traversaro
Copy link
Member

Should we enable them somewhere, for example in a preset ( https://github.com/robotology/icub-main/blob/master/CMakePresets.json#L23 )? Otherwise, how can users (even internal to IIT, that will be the main target of this feature) use this interface?

Other places were we may want to enable them (that I would like to substitute with preset at some point):

@pattacini
Copy link
Member

My idea was to handle this module like the Cartesian and the Gaze components, which should be OFF by default.

Now, if it's a matter of exposing only the I/F in any condition we could do that but we should keep the building of the relative implementation plus the fake stuff still OFF.

A shortcut could be relying on preset anyway as you suggested.

@traversaro
Copy link
Member

My idea was to handle this module like the Cartesian and the Gaze components, which should be OFF by default.

I totally agree, that make sense, but indeed then we enable the cartesian and gaze devices everywhere else, including in the default preset. I guess @MSECode has an idea on how these devices will be used by users, and can suggest which options needs to be enabled (at least in the superbuild).

@MSECode
Copy link
Contributor

MSECode commented Jan 17, 2025

Yes leaving them off it is totally fine since that module should mainly have debug purposes thus it is fine that if someone needs it it will enable and use it.

@traversaro
Copy link
Member

Yes leaving them off it is totally fine since that module should mainly have debug purposes thus it is fine that if someone needs it it will enable and use it.

At least related to the superbuild, based on my experience I am not sure if this is a good idea. Enabling one (or several) CMake options in a subproject of the superbuild is not an intuitive (and/or fast operation) for most users, and if you are using Docker (depending on how the image is structure) it may require a long rebuild of the full image, or it simply impossible if you are using conda binaries. Documenting a long list of CMake options to enable and/or disable depending on what a user needed to do was what we did in the past (https://icub-tech-iit.github.io/documentation/sw_installation/icub_head_manual/) and it was always a source of confusion as people forgot or did errors in setting the required options. Can you at least document the options that are required to enable this functionality somewhere so that we can independently enable these options in the superbuild?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants