-
Notifications
You must be signed in to change notification settings - Fork 18
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
Updated livescript for PID fingers tuning and related scripts #247
base: master
Are you sure you want to change the base?
Conversation
@mfussi66 it's up to you. I'll be glad to show you the functionalities added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some inline comments to address.
src/tools/ergocub-fingers-tuning/results/middle6-cus_PID - Copy.pdf
Outdated
Show resolved
Hide resolved
src/tools/ergocub-fingers-tuning/results/middle7-cus_PID - Copy.pdf
Outdated
Show resolved
Hide resolved
src/tools/ergocub-fingers-tuning/results/rinky2-cus_PID - Copy.pdf
Outdated
Show resolved
Hide resolved
src/tools/ergocub-fingers-tuning/results/rinky3-cus_PID - Copy.pdf
Outdated
Show resolved
Hide resolved
4. Run it by pressing "Run" or F5 | ||
- sometimes (very often) the final command 'export' doesn't produce a complete pdf file. re-run this line by selecting it until the pdf looks fine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm 🤔
This should be fixed upfront. Fine for now, but then the root cause ought to be sorted out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more hints.
I'll leave the review of the live script to @mfussi66.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just merged my suggested edits straight away.
Fine with me 👍🏻
Awaiting @mfussi66's review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I suggest the following to avoid putting too many files in the repo:
- remove all the .csv files that are not actually used for the tuning
- remove the pdf files, looking at the live script and launching it should be sufficient
- very minor, maybe replace the .mat files with simple text files?
I left only the most useful couple of matlab files to save the values of Kp,Ki and Kd for the fingers. |
Apologies for the delay, thanks for the changes @simeonedussoni ! I opened the script and it works well, the only thing I would change is adding the |
in this PR some improvements are implemented, the most important:
.m
dataset