Skip to content

iCubGenova11 add files for fineCalibrationChecker #748

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

Draft
wants to merge 10 commits into
base: devel
Choose a base branch
from

Conversation

MSECode
Copy link
Contributor

@MSECode MSECode commented Jul 28, 2025

This PR adds file that are needed for using the fineCalibrationChecker on iCubGenova11
They are mainly useful for the PI demo but it might be useful to have them for further tests or for actually using the fine calibrator when we observe calibration errors with the robot

MSECode added 2 commits July 28, 2025 12:10
Update config files demo
Add file for checking fineCalibrationChecker tool on an amc board
Finalization for demo
@MSECode MSECode requested a review from Nicogene July 28, 2025 14:43
@MSECode MSECode self-assigned this Jul 28, 2025
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @MSECode, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

I've added necessary configuration files to enable the use of the fine calibration checker on the iCubGenova11 robot. This primarily involves setting up a new calibration checker device, configuring a raw data publisher for the left arm, and integrating these new components into the robot's main configuration. These changes are intended to support the PI demo and provide tools for future calibration tests and adjustments.

Highlights

  • Calibration Order Adjustment: I've updated the calibration order for both the left and right arms by removing the last element '15' from the final group in their respective calibration XML files. This adjustment likely refines the calibration sequence for these robotic arms.
  • New Fine Calibration Checker Configuration: I've introduced a new configuration file for a 'fineCalibrationChecker' specifically for the iCubGenova11's left arm. This file includes detailed parameters for axes, gold positions, calibration deltas, and encoder resolutions, which are crucial for precise calibration checks.
  • Raw Values Publisher for Left Arm: I've added a new wrapper configuration that sets up a raw values publisher for the left arm. This will allow raw encoder data to be published, which is essential for the fine calibration checker to function correctly.
  • System Integration of New Components: I've integrated the newly added fine calibration checker and raw values publisher configurations into the main 'icub_all_no_legs.xml' file, ensuring they are loaded and active when the robot system starts up.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds configuration files for the fineCalibrationChecker on iCubGenova11. The changes look mostly correct for enabling this feature. However, there is a critical issue with a misnamed wrapper file, which will cause configuration problems. I've also pointed out some minor inconsistencies in the new checker configuration file that could be improved for better clarity and maintainability. Please see my detailed comments.

Copy link
Member

@pattacini pattacini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MSECode

CI failed with the following reports:

Also, there's one critical comment raised by @gemini-code-assist requiring your attention.

MSECode and others added 2 commits July 28, 2025 17:13
Copy link
Member

@pattacini pattacini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of doubts regarding joint 15, @MSECode

@@ -31,7 +31,7 @@
<param name="startupPosThreshold"> 2 2 2 2 90 90 90 90 90 90 90 90 90 90 90 90 </param>
</group>

<param name="CALIB_ORDER">(0 1 2 3) (4) (5 6 7) (8 9 11 13) (10 12 14 15)</param>
<param name="CALIB_ORDER">(0 1 2 3) (4) (5 6 7) (8 9 11 13) (10 12 14)</param>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MSECode, was this change intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First commit I did was from the robot. Thus it might be due to changes left there that I took when checking out. I know that lately there were some problems with the finger. Thus I'm not sure if it was due to that to remove the last joint. Anyway I can remove that change from this PR and we can double check that directly on the robot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway I can remove that change from this PR and we can double check that directly on the robot.

Yep, thanks! I'd go with this solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@pattacini pattacini Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that they're disabled locally to the robot doesn't imply that they should be upstream in the repo. Something to consider with care, I'd say.

I would still keep 15 upstream.
iCubGenova11 follows a different workflow/pace wrt ergCubs.

Copy link
Member

@pattacini pattacini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI fails now while checking XML schema:

/dependencies/install/share/ICUBcontrib/robots/iCubGenova11/wrappers/motorControl/left_arm-mc-rawval-nws_wrapper.xml:4: element device: Schemas validity error : Element 'device', attribute 'type': [facet 'enumeration'] The value 'rawValuesPublisherServer' is not an element of the set {'controlBoard_nws_yarp', 'controlBoard_nws_ros', 'controlBoard_nws_ros2'}.
/dependencies/install/share/ICUBcontrib/robots/iCubGenova11/wrappers/motorControl/left_arm-mc-rawval-nws_wrapper.xml:8: element paramlist: Schemas validity error : Element 'paramlist': This element is not expected. Expected is ( param ).
/dependencies/install/share/ICUBcontrib/robots/iCubGenova11/wrappers/motorControl/left_arm-mc-rawval-nws_wrapper.xml fails to validate
Wrappers XSD check failed!

@pattacini
Copy link
Member

CI fails now while checking XML schema:

The schemas we're currently enforcing are available at:

cc @MSECode

@MSECode MSECode marked this pull request as draft July 28, 2025 15:44
@MSECode MSECode marked this pull request as ready for review July 29, 2025 09:27
@pattacini
Copy link
Member

Hi @MSECode

You may want to fiddle with the XML schemas on your local system instead of relying on the CI itself.
You may also consider exploiting the GH Codespaces on your fork, which are backed up by a docker.

The dry-run material is contained in https://github.com/robotology/robots-configuration/tree/master/tests/dry-run:

  • Run configure-tests.sh to build the check-xml binary and configure all the robots.
  • The executable check-xml is devoted to verifying the XML schemas stored in this repo. It may be that, aside from the schemas, the program needs some adjustment, depending on your use case.

@pattacini
Copy link
Member

pattacini commented Jul 30, 2025

Now that robotology/icub-main#1034 is merged, I'm regenerating the docker image:

This way, the "dev" dry-run (i.e., the proper dry-run of the YARP devices) should work.
The "xml" dry-run (responsible for verifying the XML schemas) still needs inspection, I guess.

@MSECode
Copy link
Contributor Author

MSECode commented Jul 30, 2025

Not sure if the CI is failing on the xml here:

image

because the xml file for the raw values for the right arm is present among the wrappers but commented out in the main xml file iCubGenova11/icub_all_no_legs.xml.
And why the left_arm wrapper is checked 2 times from the CI differently from the one of the right arm

@pattacini
Copy link
Member

Hi @MSECode

Can you please make a push, even an empty push, to trigger the CI afresh, after having regenerated the docker?

Then, we will look into the problems that may potentially occur again.

@pattacini
Copy link
Member

The following plugin is not available yet from the building:

  • rawValuesPublisherServer

Do we need to enable it somehow?

@MSECode
Copy link
Contributor Author

MSECode commented Jul 31, 2025

Yes,
since it is disabled by default it should be enabled by setting to true the Boolean value in icub-main cmake configuration.
Same for rawValuesPublisherClient and FineCalibrationCheker if needed

@pattacini
Copy link
Member

I'll do that then, with the docker.

Nicogene added a commit to robotology/robotology-superbuild that referenced this pull request Jul 31, 2025
…tionChecker

As per title.

This would come handy since these devices has been added to the icub_all.xml of iCubGenova11:
- robotology/robots-configuration#748

Xref:
- robotology/icub-main#1034
@pattacini
Copy link
Member

I'll do that then, with the docker.

Done 👍🏻
See https://github.com/robotology/robots-configuration/actions/runs/16649881946.

@pattacini
Copy link
Member

To be sure of the alignment of resources, we'd need a fake push to this PR.

@pattacini
Copy link
Member

pattacini commented Jul 31, 2025

The "dev" dry-run now fails with this error:

qt.qpa.xcb: could not connect to display 
qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

which is obviously related to the use of graphics in a headless environment.
By the way, what happens when YRI is launched remotely on a system w/o -X?

Options:

  1. We can install X and QT on the docker. Don't know if it will work on GH Action. Also, this will make the docker maintenance unnecessarily unwieldy.
  2. We develop the option --no-gui for the new plugin so that it can be used in this context. The option will be activated by the dry-run machinery or made the default (considering that launching YRI via yarpmanager could be the default).
  3. We rule out the plugin outright from the test.

My preference: 2 and then 3.
Option 1 was mentioned only for the record 😉


That said, there's still an error with the "xml" checker as pointed out in #748 (comment).

@pattacini
Copy link
Member

Putting the PR back to draft because of #748 (comment).

@Nicogene
Copy link
Member

Nicogene commented Aug 1, 2025

The "dev" dry-run now fails with this error:

qt.qpa.xcb: could not connect to display 
qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

which is obviously related to the use of graphics in a headless environment. By the way, what happens when YRI is launched remotely on a system w/o -X?

Options:

1. We can install X and QT on the docker. Don't know if it will work on GH Action. Also, this will make the docker maintenance unnecessarily unwieldy.

2. We develop the option `--no-gui` for the new plugin so that it can be used in this context. The option will be activated by the dry-run machinery or made the default (considering that launching YRI via yarpmanager could be the default).

3. We rule out the plugin outright from the test.

My preference: 2 and then 3. Option 1 was mentioned only for the record 😉

That said, there's still an error with the "xml" checker as pointed out in #748 (comment).

See:

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 this pull request may close these issues.

4 participants