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

Updated livescript for PID fingers tuning and related scripts #247

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

simeonedussoni
Copy link
Member

in this PR some improvements are implemented, the most important:

  • interactive choice of the data files for calibration
  • save of the report in dedicated file with speaking name together with .m dataset
  • script to reset workspace

@simeonedussoni
Copy link
Member Author

@mfussi66 it's up to you. I'll be glad to show you the functionalities added

@mfussi66 mfussi66 assigned mfussi66 and simeonedussoni and unassigned mfussi66 May 15, 2024
@Nicogene Nicogene requested a review from mfussi66 May 15, 2024 09:50
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.

Some inline comments to address.

src/tools/ergocub-fingers-tuning/init_ws.m Outdated Show resolved Hide resolved
src/tools/ergocub-fingers-tuning/scripts/Reshape_dataset.m Outdated Show resolved Hide resolved
src/tools/ergocub-fingers-tuning/README.md 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
Copy link
Member

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.

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.

Some more hints.

I'll leave the review of the live script to @mfussi66.

src/tools/ergocub-fingers-tuning/README.md Outdated Show resolved Hide resolved
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.

I've just merged my suggested edits straight away.
Fine with me 👍🏻

Awaiting @mfussi66's review.

Copy link
Member

@mfussi66 mfussi66 left a 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?

@simeonedussoni
Copy link
Member Author

I left only the most useful couple of matlab files to save the values of Kp,Ki and Kd for the fingers.
Also, I comented the lines to produce the .m file and the .pdf report (an eye check can be done when running the livescript that the plots are ok and the step response also)

@Nicogene Nicogene requested a review from mfussi66 May 17, 2024 07:38
@mfussi66
Copy link
Member

mfussi66 commented Jun 7, 2024

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 init_ws call at the beginning of the live script.

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.

3 participants