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

Seg Fault due to hub model and external inflow #1227

Merged
merged 2 commits into from
Aug 25, 2022

Conversation

psakievich
Copy link
Contributor

Feature or improvement description

The hub index is 1 in most of the code (first entry in the array), but here it is the last one. I've had segfaults in nalu-wind and amr-wind that both trace back to this line of code when running actuators with the hub turned on.

Interestingly we only see this issue with GCC, and in testing a RelWithDebugInfo build of OpenFAST the failures were intermittent as many memory leaks are.

Related issue, if one exists

Exawind/nalu-wind#979

Impacted areas of the software

Any turbine run using the external inflow and hub model.

Additional supporting information

Pinging @andrew-platt @rafmudaf @gantech

Test results, if applicable

I am not an openfast developer and am not aware of the tests. I assume this is just not getting covered in the test suite. We've had a segfault in the nalu-wind test suite for months now.

https://my.cdash.org/test/60805600

@andrew-platt andrew-platt self-requested a review August 24, 2022 00:21
@andrew-platt andrew-platt self-assigned this Aug 24, 2022
@andrew-platt andrew-platt added this to the v3.3.0 milestone Aug 24, 2022
Copy link
Collaborator

@andrew-platt andrew-platt left a comment

Choose a reason for hiding this comment

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

@andrew-platt andrew-platt changed the base branch from main to dev August 24, 2022 00:40
@andrew-platt
Copy link
Collaborator

andrew-platt commented Aug 24, 2022

Unless there are objections, I'm going to merge this into the dev branch. It will be released with v3.3.0

@bjonkman
Copy link
Contributor

bjonkman commented Aug 24, 2022

I think this is a fine temporary fix, but technically this last point is the nacelle and not the hub. The hub point (index 1) gets sent to ServoDyn (see SrvD_InputSolve), so it is omitted in subroutine AD_InputSolve_IfW and is the reason the node index starts at 2 there instead of 1.

We added a nacelle point in AeroDyn for some other work (hydropower? + some syncing with Envision), but the OpenFOAM interface portion of that in OpenFAST apparently didn't get updated. I was actually working on a fix for this interface last week, but it will also require that everything interfacing with this (nalu-wind, amr-wind, and similar) will need to supply another point for wind at the nacelle... or you could just assume that the wind at the nacelle was close enough to the hub and leave it as a (small?) source of error.

@psakievich
Copy link
Contributor Author

psakievich commented Aug 24, 2022

Hmm this is interesting. If we do add another point I would request that you add an option to make the addition of the new point for the nacelle optional. I've always thought we were lumping the contributions from the hub and nacelle together for the CFD coupling. It would be nice if we had an option for this behavior. From an actuator line implementation standpoint this will require us to add an additional actuator point, code infrastructure etc. This is inconsistent in terms of restarts and can potentially add larger impacts on the codes coupling to OpenFAST since our user base uses different versions of OpenFAST. @tonyinme what are your thoughts? Do you see a benefit to the discrediting the hub nacelle fixture?

@tonyinme
Copy link
Contributor

hey all, @psakievich had a conversation offline and agreed that it is best to bundle the hub and nacelle contribution into one for the CFD coupling.

@andrew-platt
Copy link
Collaborator

@psakievich, could you add a comment in the code about using the hub point as the nacelle point for the CFD coupling? This could help prevent confusion when we are looking at that code later. I'll merge after that.

Copy link
Contributor

@bjonkman bjonkman left a comment

Choose a reason for hiding this comment

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

Could you revert the changes you made to remove all of the trailing spaces in FAST_Solver.f90? It makes resolving merge conflicts more difficult than it needs to be.

@psakievich
Copy link
Contributor Author

sorry. I forgot to turn off the auto-formatting on my neovim config. I don't ever work in fortran. will fix

@andrew-platt andrew-platt merged commit b888e4a into OpenFAST:dev Aug 25, 2022
@psakievich psakievich deleted the patch branch August 25, 2022 17:52
@rafmudaf rafmudaf mentioned this pull request Oct 27, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants