search

jhu-dvrk / sawIntuitiveResearchKit

cisst/SAW stack for the da Vinci Research Kit (dVRK)
https://github.com/jhu-dvrk/sawIntuitiveResearchKit/wiki
117 stars 68 forks source link

Fix joint velocity abs bug in GC controller #117

Closed zchen24 closed 5 years ago

vincent-hui commented 5 years ago

@adeguet1 do you think we should move this patch to master branch to make a patch release?

adeguet1 commented 5 years ago

Re. minor release, this is really up to you since I don't know how much impact this bug has.

Does the patch also require to recalibrate the arms, i.e. acquire data and/or re-run the parameter identification code? I see two recent commits on https://github.com/jhu-dvrk/dvrk-gravity-compensation and it seems that we can use the existing gc-MTMX-12345.json.

If the users don't have to recalibrate, I would do a minor release (1.7.1). If we need to recalibrate the arm, the question is: how bad is the current code? If it's not too bad, I would patch the devel branch and wait for the next release. On the other hand, if the bug has potentially catastrophic consequences (apply random/high torques) and might damage some hardware, we should create a new release (1.8) with a new file format revision (3) for the gc-MTMX-12345.json (to force users to recalibrate). 1.8 would be forked from master@1.7, not the devel branch since it is somewhat untested.

linhongbin commented 5 years ago

Thanks for your reply. Sorry for the code mistake. I might try to answer the points you asked.

Q1: -Does the patch also require to recalibrate the arms? A1: No, it is not necessary. There are two new commits in the matlab code, which are for correcting the equation and tuning down the effect of friction compensation respectively. The first one is not related to the CPP code and the later one just is to reduce the drift of MTM by tuning down the friction compensation ratio.

Q2: -how bad is it using the existing code(1.7.0)? A2: I think the difference of result between released(1.7.0) and minor patch(1.7.1 maybe) is minor. The code difference of these releases is related to friction compensation, which is on a relatively small scale compared to gravity compensation with respect to the compensate torques. In addition, we actually have implemented some saturated function for dealing the case like high output torques,(i.e. if the output torques surpass the saturated output torques, we will output the saturated ones).

Q3: -how bad is the current code if we do not re-calibrate the 'gc-MTMX-12345.json'. A3: I think the only difference of results between 'gc-MTMX-12345.json' with and without re-calibration is friction compensation ratio. The JSON file without re-calibration just worked fine. I hope I could further improve the performance by tuning parameters in the JSON files, but not changing the Matlab code. And the recalibration does not need to collect new data again.

Cheers.

vincent-hui commented 5 years ago

@adeguet1 this patch does not require to acquire data, re-running the parameter identification code is optional. If users prefer higher friction, they can re-run the parameter identification code to get new friction compensation ratio, dynamic parameters will not be changed. If it does not take you too much effort, would you do a minor release (1.7.1) after one or two weeks? I think the most easiest way to do the minor release is to cherry pick this commit to master branch. Thank you

adeguet1 commented 5 years ago

Since it is a one line code change I indeed intended to patch the master branching release 1.7.1. You mentioned waiting for a week or too, is this so you can test/verify the code further? If not I can release asap but if you need further time to validate your code, let me know.

vincent-hui commented 5 years ago

Yes, we need more time to review our code and test our code with hardware.

vincent-hui commented 5 years ago

Hi @adeguet1, I think we can release 1.7.1. For matlab code, please merge devel branch into master branch. Thank you very much.

adeguet1 commented 5 years ago

Sorry for the delay, I was hoping to find some time to do the release while on vacation but that didn't happen. I should be able to release within a couple of days.