-
Notifications
You must be signed in to change notification settings - Fork 11
Update .h file and caller to v2 API #16
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
base: main
Are you sure you want to change the base?
Conversation
akeeste
left a comment
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 @jtgrasb. Can you clarify why we need to wait for a new WEC-Sim release before merging this?
| function lineForces = moordynCaller(disp,vel,time,dt) | ||
| % function calls MoorDyn library to calculate and return the mooring | ||
| % line forces at every time step. | ||
| mdStruct = evalin('base','mdStruct'); |
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.
Same comment about evalin and base workspaces as in WEC-Sim/WEC-Sim#1566
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.
mdStruct is a libpointer. These cannot be loaded into Simulink. The best alternative I found is to use evalin here to load it into the MATLAB function. A couple other options I found seem to be to declare it as a global variable or us an S-function in Simulink, which both seemed like worse options. We could also go back to v1 API and the MoorDyn team may be able to edit the source to avoid the graphics popup window.
|
I am suggesting waiting for the next WEC-Sim release because this updates the API used to couple WEC-Sim with MoorDyn and a number of function calls in the mooring class and library. If we merge this before PR#1566 is included in the main branch, this repo will not be compatible with WEC-Sim's main branch. The other option would be to merge an equivalent of PR#1566 into the main branch directly. |
|
I'm in favor of moving #1566 into main. It's not a bug for users with a graphical interface, but could be one when running on e.g. an HPC without a graphic interface, as it is for our CI. It would be nice to have those tests working soon too |
This PR updates the .h file and the moordynCaller function to the new v2 API. Previously, we were using version 2, but using the API from version 1. However, with this approach MoorDyn was attempting to open a graphics window which seemed to crash MATLAB in GitHub actions sometimes. Refer to this discussion for more info.
Note that I removed a lot of lines from the original MoorDyn2.h file as they would require additional .h files that are unnecessary for WEC-Sim. This means that we may need to make manual changes if MoorDyn releases a new .h file format in the future.
This PR should be merged only after PR#1566 on WEC-Sim is merged and a new release is created. This should ensure compatibility with the main branch remains consistent.
For validation, I compared the results with the v1 .h file to the v2 .h file:
