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

Controls+pto sim #40

Merged
merged 8 commits into from
Sep 20, 2023
Merged

Controls+pto sim #40

merged 8 commits into from
Sep 20, 2023

Conversation

jtgrasb
Copy link
Contributor

@jtgrasb jtgrasb commented Aug 22, 2023

This PR adds a controls example to demonstrate how controls can be used with PTO-Sim. It demonstrates a reactive (PI) controller with a simplified direct drive PTO model (WEC-Sim PR #1106) consisting of a drivetrain and generator. The corresponding docs can be found in WEC-Sim PR #1108.

@jleonqu
Copy link
Contributor

jleonqu commented Sep 11, 2023

Hi @jtgrasb
I was checking this PR and everything works well on my end. My only comment is that you mentioned that there is documentation in the WEC-Sim PR #1106 but I couldn't find it. Does this refer to the documentation on the WEC-Sim Webpage?

@jtgrasb
Copy link
Contributor Author

jtgrasb commented Sep 11, 2023

Hi @jleonqu, sorry for the confusion, the docs are actually in WEC-Sim PR #1108.

Copy link

@nathanmtom nathanmtom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtgrasb and @jleonqu thank you for pulling together this applications case to illustrate how the controls development can be completed with PTO-Sim. I was able to successfully run the model and reproduce the plots on the Documentation pages under PR#1108.

My request for additions comes from having consistency across applications cases:

  1. Please add a README.md file in the applications folder. A good example can be found in WEC-Sim_Applications\WECCCOMP\WECCCOMP_Nonlinear_Model_Predictive\README.md. This gives you credit for building the example, highlight the version the example was built in, and listing steps on how to run the code to reproduce example results. Understand you have the new docs but adding an abbreviated step-by-step will be helpful.
  2. Add this case to the TestControls.m found in the Controls folder.
  3. Minor comment about the Controls folder, but under hydroData do we need to keep the rm3.out as all examples seem to be using the sphere.out. Second there seems to be an mcrCases.mat in the hydroData folder which seems out of place and each subfolder has an mcrBuild...m file to generate the .mat file to run MCR. Third, in the geometry folder can we delete the float.stl as similar to the hydrodynamics that all examples appear to use sphere and not RM3?

If the above additions and corrections can be implemented then can approve this PR. I will also flag that the free decay test is failing for the latest release, but across all PRs, so should be investigated but should not hold up this PR.

@jtgrasb
Copy link
Contributor Author

jtgrasb commented Sep 12, 2023

@nathanmtom Thanks for the review. I agree with all of your suggestions and updated accordingly.

@jtgrasb jtgrasb marked this pull request as ready for review September 12, 2023 16:07
Copy link
Contributor

@jleonqu jleonqu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything works well on my end. I was able to run the models and use the application cases.

Copy link

@nathanmtom nathanmtom 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 responding to my changes @jtgrasb. I approve this PR now.

@nathanmtom nathanmtom merged commit 1609bc4 into WEC-Sim:dev Sep 20, 2023
32 of 35 checks passed
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