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

Wrong coefficients coupling the eyes' vergence? #228

Closed
pattacini opened this issue Jan 11, 2021 · 22 comments · Fixed by #574
Closed

Wrong coefficients coupling the eyes' vergence? #228

pattacini opened this issue Jan 11, 2021 · 22 comments · Fixed by #574
Assignees
Labels

Comments

@pattacini
Copy link
Member

pattacini commented Jan 11, 2021

Given that the vergence Vg amounts to the difference between the single eyes' joints L-R (see the wiki), I think that the values contained in the eyes' coupling matrices should be -1.0 1.0 instead of -0.5 0.5.

Differently, as Vs = (L + R) / 2 it turns out that the line immediately above reporting 0.5 0.5 is spot on.

Can this reasoning be assumed correct?

This is somehow linked to robotology/icub-firmware#71.

cc @davidetome @randaz81 @ale-git @traversaro @plinioMoreno

@pattacini
Copy link
Member Author

Any comments on this @ale-git @randaz81 ?

@pattacini pattacini self-assigned this Jan 19, 2021
@pattacini
Copy link
Member Author

Discussed this with @randaz81 today, who agrees with what said above.
However, the wrong matrix coefficients get subsumed by the PID gains, hence there ain't actually any real control problem.

Given that it's more correct to have -1.0 1.0, it'd be worth updating the conf files anyway.

@pattacini
Copy link
Member Author

pattacini commented Mar 2, 2021

Apparently, also matrixE2J should contain coupling terms!

@pattacini
Copy link
Member Author

/remind September 03 let's address this once for good.

@octo-reminder
Copy link

octo-reminder bot commented Jul 28, 2023

Reminder
Sunday, September 3, 2023 10:00 AM (GMT+02:00)

let's address this once for good.

@pattacini pattacini added the bug label Jul 28, 2023
@octo-reminder
Copy link

octo-reminder bot commented Sep 3, 2023

🔔 @pattacini

let's address this once for good.

@pattacini
Copy link
Member Author

Link in the OP fixed.

@pattacini
Copy link
Member Author

pattacini commented Sep 18, 2023

To sum up, here's the list of things that we need to do:

  • Stick to one iCub with eyes; most likely iCubGenova11.
  • Update iCubGenova11 following these instructions.
  • Test that the eyes will still move correctly w/o big overshoots.
  • Update all the remaining maintained robots.

@martinaxgloria
Copy link
Contributor

Hi @pattacini,

as you suggested, I booked iCubGenova11 to do the tests with the new values of eyes' vergence. I took a video of the eyes' movement after the upload:

IMG_1507.mp4

I have little experience with it, but the movement seems smooth, also compared to the previous version with the wrong vergence values:

IMG_1505.mp4

Do you agree? If yes, I'll proceed with the update of all the maintained robots.

@martinaxgloria
Copy link
Contributor

Just to clarify, I only update this line with -1.0 1.0 instead of -0.5 0.5.

@pattacini
Copy link
Member Author

Great, @martinaxgloria 👍🏻

That's all correct!

The videos are nice. However, given the control context, I would expand on the validation phase by including some data acquisitions of the commands and joint feedback. These traces would help us appreciate small details that are not visible in the videos, like overshoots, rising times...

@martinaxgloria
Copy link
Contributor

You're right @pattacini! I'll do the acquisitions and I'll report here the results.

@pattacini
Copy link
Member Author

You can talk to @davidetome and/or @mfussi66, who do have some code snippets for logging both commands and feedback you may consider reusing.

@martinaxgloria
Copy link
Contributor

Today I plotted the data I've acquired on iCubGenova11 after the change in the vergence coupling matrix and here they are:

image

I hope that these quantities are the ones that @pattacini referred to. In particular, I plotted the encoder, motor encoder, pwm and current values for the eyes_tilt, eyes_vers and eyes_verg joints. These values were obtained by setting a sequence from the yarpmotorgui from the home position, to the upper limit, then the lower limit and then home again for the three eyes' joints. I set a time of 5 sec for each movement (e.g. from home to upper limit in 5 sec). Then I start the acquisition and I ran the code to save the values in order to plot them at the end.

The only particular thing that stands out is the peak of current and duty cycle percentage around the second 45 that corresponds to the eyes_verg joint reaching its upper limit. In your opinion, is it consistent with the movement?

cc @pattacini @mfussi66

@pattacini
Copy link
Member Author

pattacini commented Oct 2, 2023

Great that you managed to plot these results 👍🏻
A couple of important points though:

  • The joint references are missing.
  • The 5-second movements are far slower than the dynamics we're interested in with the eyes. Eyes need to move much much faster, meaning fractions of a second. For example, let's stick to 0.3 s.

We could also plot the profiles before and after the change.

@martinaxgloria
Copy link
Contributor

Yesterday I repeated the tests acquiring the joint references and the other quantities with much faster movements (0.3 sec per movement). To make a comparison, I also do the tests and the acquisitions before the change:

old
and the comparison between the joint encoder angles and the reference values:

encoder_pid_old

After the changes, instead:

new

encoder_pid_new

@mfussi66 noticed that the sampling time it's high (when I did again the acquisitions maybe I accidentally change this parameter and I didn't notice it). So far, I decided to post here the results, but I'll do again the acquisitions and the plots with more samplings to have a more accurate analysis.

cc @pattacini

@pattacini
Copy link
Member Author

Overall, I'm happy with the results and for me it's green light 🟢
Thus, we can proceed with the update of robots-configuration.

@mfussi66 noticed that the sampling time it's high (when I did again the acquisitions maybe I accidentally change this parameter and I didn't notice it). So far, I decided to post here the results, but I'll do again the acquisitions and the plots with more samplings to have a more accurate analysis.

It seems so, yeah. However, what was the sample time you used?

@martinaxgloria
Copy link
Contributor

It seems so, yeah. However, what was the sample time you used?

I computed it from the acquired samples and it is 0.1s

@pattacini
Copy link
Member Author

pattacini commented Oct 4, 2023

It seems so, yeah. However, what was the sample time you used?

I computed it from the acquired samples and it is 0.1s

Ok, it's fairly low, but the graphs above demonstrate that the reference is well-tracked anyhow.

For the record, a good sample time when dealing with control issues is 10 ms at the YARP layer.

@martinaxgloria
Copy link
Contributor

Yes, thanks for the clarification @pattacini!
So, can I proceed with the update of robots-configuration or it's better to re-do the acquisitions with the right sample time before?

@pattacini
Copy link
Member Author

So, can I proceed with the update of robots-configuration or it's better to re-do the acquisitions with the right sample time before?

Let's proceed with robots-configuration 👍🏻

@martinaxgloria
Copy link
Contributor

martinaxgloria commented Oct 4, 2023

PR here:

cc @pattacini

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

Successfully merging a pull request may close this issue.

2 participants