robotology / whole-body-estimators

YARP devices that implement estimators for humanoid robots.
26 stars 12 forks source link

[WBD] update `calibStandingWithJetsiRonCubMk1` for iRonCub-Mk1_1 #136

Closed HosameldinMohamed closed 2 years ago

HosameldinMohamed commented 2 years ago

Related PR #113 which was done for iRonCub-Mk1. We modify the RPC command to be compatible with iRonCub-Mk1_1.

HosameldinMohamed commented 2 years ago

It's a simple PR but to be sure I am not forgetting anything!

prashanthr05 commented 2 years ago

I believe you must also commit the autogenerated IDL files?

HosameldinMohamed commented 2 years ago

I believe you must also commit the autogenerated IDL files?

I suppose not @prashanthr05. After #131 we no longer save the autogenerated files.

traversaro commented 2 years ago

Why remove the old method calibStandingWithJetsiRonCubMk1 ?

HosameldinMohamed commented 2 years ago

Why remove the old method calibStandingWithJetsiRonCubMk1 ?

I wanted to avoid duplicating the code. Because the only change between the methods is in the idle thrust of the arms jets.

Also, I don't think we will be using iRonCub-Mk1 again, unless we really have to. But now that you raised this point. It's better to double-check with the team to have a definitive answer.

traversaro commented 2 years ago

Why remove the old method calibStandingWithJetsiRonCubMk1 ?

I wanted to avoid duplicating the code. Because the only change between the methods is in the idle thrust of the arms jets.

We could have an helper method that just takes idle thrust and arm jets as parameters, and then you call that one on the different calibtStandingWithJets*** methods?

traversaro commented 2 years ago

Are you sure that it is ok to have a breaking change? If you prefer to still have calibStandingWithJetsiRonCubMk1 you can still keep this method and just call calibStandingWithJetsiRonCub("mk1") in its implementation, but this is up do you.

HosameldinMohamed commented 2 years ago

Are you sure that it is ok to have a breaking change? If you prefer to still have calibStandingWithJetsiRonCubMk1 you can still keep this method and just call calibStandingWithJetsiRonCub("mk1") in its implementation, but this is up do you.

@traversaro I see, Which of the two below did you mean?

Just to understand the better practice. But in this particular case, only the iRonCub team are using it. So I think it should be fine to add a breaking change.

HosameldinMohamed commented 2 years ago

Thanks!

traversaro commented 2 years ago

Yes I was referring to both cases you were referring to.

HosameldinMohamed commented 2 years ago

Yes I was referring to both cases you were referring to.

Ok. I agree that in general this kind of change can be harmful but in this case it should be ok. I added some documentation of the RPC commands #137.