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

fix: (hack) Ensure vizinterface knows about RWModels enum. #845

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

dpad
Copy link
Contributor

@dpad dpad commented Nov 11, 2024

  • Tickets addressed:
  • Review: By file
  • Merge strategy: Merge (no squash)

Description

Work-around to fix a minor issue when trying to read the RWModel value of RWConfigLogMsgPayloads when vizInterface is also available. Here, Python would interpret RWConfigLogMsgPayloads as actually coming from the vizinterface module. However, the vizinterface module was not aware of the "RWModels" enum, so SWIG did not know how to interpret it as an integer, nor how to destroy it (leading to some "detected a memory leak of type 'RWModels *'" warnings). With this fix, RWModels correctly returns an integer, and the warning disappears.

NOTE: This is not a proper fix, but only works around the issue in the same way we've been doing before. We should really tackle the root cause at some point.

Root cause: This kind of issue is also present in other types, e.g. SysModel, seemingly because of how we're copying SWIG types across many modules. For example, the SysModel definition re-appears in each individual message module. Fixing this (e.g. such that SysModel is only defined in one module) would probably significantly reduce the size of the total compiled Basilisk.

Verification

I don't really have a minimal way to test this, but basically, when getting the value of a RWConfigLogMsgPayload's .RWModel, it was returning a Swig Object of type 'RWModels *' instead of an integer. After this fix, it correctly returns an integer.

Documentation

Future work

@dpad dpad requested a review from a team as a code owner November 11, 2024 09:29
@dpad dpad force-pushed the fix/vizinterface-RWModels-enum branch 2 times, most recently from ea71108 to 05328af Compare November 11, 2024 09:35
@schaubh schaubh force-pushed the fix/vizinterface-RWModels-enum branch from 05328af to d65c3c4 Compare November 11, 2024 21:52
@schaubh schaubh self-assigned this Nov 12, 2024
@schaubh schaubh added the bug Something isn't working label Nov 12, 2024
Copy link
Contributor

@schaubh schaubh 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 fix. The pre-commit CI test failed due to trailing white space in some files. I fixed that and pushed a revised version.

@schaubh schaubh merged commit de29b28 into AVSLab:develop Nov 12, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants