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

Segmentation Fault Fix/FreqRefUserInfo temperature fix #19

Merged
merged 13 commits into from
Apr 30, 2024

Conversation

dboulware
Copy link
Collaborator

This fixes the consistent and mysterious segmentation faults that have been occurring. The primary fix appears to have been correcting the _DEVINFO_MAX_STRLEN to match the header file which causes the temperature to correctly populate in the FreqRefUserInfo struct. In addition to this, this also changes filenames to a pointer POINTER in the IQStreamFileInfo Struct and adds some debug logging. Finally, it increments the version to 1.3.3. Thanks to @jhazentia for the assistance.

@jhazentia
Copy link
Member

You may want to consider posting the filenames pointer issue on the Tektronix RSA_API repo. For the filenames pointer issue, the Cython version looks correct with double pointer but ctypes version looks wrong. For the FreqRefUserInfo fix, I don't see it in ctypes version and commented out in cython version.

Copy link
Member

@aromanielloNTIA aromanielloNTIA left a comment

Choose a reason for hiding this comment

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

Looks good! Tested with RSA300 on Linux. I made a few changes:

  • Removed most of IQSTREAM_Tempfile() and replaced with with simple calls to configure the device, then call IQSTREAM_Tempfile_NoConfig() directly. This removes some duplicate code and makes sure same wait-for-data behavior occurs in both functions.
  • I went to the source RSA_API.h header file to check out all the ReturnStatus values, and filled in the missing ones in the Python wrapper. This change also included fixing a few typos where Enum values were set to integers.

I'm still testing on my end with RSA 500- seeing segfaults again when running unittest with RSA500

Copy link
Member

@aromanielloNTIA aromanielloNTIA left a comment

Choose a reason for hiding this comment

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

Made a few more edits to align this wrapper with the struct definitions and char array allocations from RSA_API.h. Seems to have fixed some segfault behavior that would occur at the end of running unit tests. I also replaced DEVICE_GetInfo with a more direct function call, which works now, since the string lengths are handled correctly.

I think this is ready to merge! Going to hold off for a bit on creating a release, to update pre-commit stuff and test maximum supported Python version.

@aromanielloNTIA aromanielloNTIA merged commit 41c154f into main Apr 30, 2024
2 checks passed
@aromanielloNTIA aromanielloNTIA deleted the StructsFix branch April 30, 2024 16:59
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