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

Pause error #321

Merged
merged 3 commits into from
Apr 29, 2024
Merged

Pause error #321

merged 3 commits into from
Apr 29, 2024

Conversation

lawhead
Copy link
Collaborator

@lawhead lawhead commented Apr 25, 2024

Overview

  • Fixed an issue where BciPy would crash after a long pause.
  • Fixed a bug with segfaults when running BciPy on Macs.

Ticket

https://www.pivotaltracker.com/story/show/187261791

Contributions

  • Refactored the acquisition module to use multiprocessing, rather than threading (see discussion below).
  • Added a configuration for the max number of seconds allowed for pausing.
  • UI handling to end a task after a max pause duration has been reached.

Discussion

Segfault issue

The LSL library has methods which must be run on the main thread of execution on linux/Mac computers. Running them in a separate thread was causing segfaults to occur. By refactoring to use multiprocessing, each of the objects that use LSL are essentially acting as the main thread, which resolves this issue. Note that we should do a timing test to confirm that the static offsets are still the same.

Pause bug

Previously we calculated the max buffer for LSL based on the maximum possible duration of an inquiry. However, we did not account for pause length. I was making the assumption that the LSL buffer would continue to accumulate data and only retain the latest (like what would happen in a ring buffer data structure). However, LSL has undefined behavior when the StreamInlet max_buflen is exceeded. When you attempt to retrieve data after this maximum is exceeded some of the data is missing and what is returned may not even be in monotonic order. I was able to replicate this behavior without using BciPy in the lsl_buffer_test.py script. This script will allow us to test future versions of LSL in case we want to change our approach. Currently, the resolution is to set a fixed maximum buffer size in the configuration that accounts for the maximum amount of time we may want to pause during a session. If this is exceeded the task gracefully shuts down.

Test

  • Ran a calibration task and paused on the wait screen (before starting the task) for different durations to confirm the behavior is correct.
  • Ran the calibration task and paused after one or more inquiries. Confirmed that the task was able to resume if the wait time was less than the configured max.

lawhead added 2 commits March 22, 2024 17:03
…an a thread to fix segfaults; set a max pause parameter to avoid data issues when LSL's StreamInlet's buffer is too small for the amount of data.
@lawhead lawhead requested a review from tab-cmd April 25, 2024 21:34
Copy link
Contributor

@tab-cmd tab-cmd left a comment

Choose a reason for hiding this comment

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

Nice catch here! LSL keeping us on our toes! This should be a more resilient way to use it overall

self.inlet = StreamInlet(
stream_info,
max_buflen=4 * self.max_buffer_len, # TODO: revisit this value
max_buflen=MAX_PAUSE_SECONDS + 5,
Copy link
Contributor

Choose a reason for hiding this comment

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

To help with later debugging, I would advocate adding the 5 to the max pause seconds or making it a class variable _max_buffer_paddingor similar.


self.inlet.open_stream(timeout=5.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

or perhaps a timeout variable

bcipy/config.py Outdated
@@ -51,5 +51,6 @@

# misc configuration
WAIT_SCREEN_MESSAGE = 'Press Space to start or Esc to exit'
MAX_PAUSE_SECONDS = 360
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a variable we would ever want to configure in the parameters? I can imagine it being an experiment control along with max # of pauses. This pattern works well if not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. I think my preference is to leave it here until it comes up.


elapsed_seconds = time.time() - pause_start
if elapsed_seconds >= MAX_PAUSE_SECONDS:
log.info("Pause exceeded the allowed time. Ending task.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add the MAX_PAUSE_SECONDS to the error message to help with debugging

@tab-cmd
Copy link
Contributor

tab-cmd commented Apr 29, 2024

Nice catch here! LSL keeping us on our toes! This should be a more resilient way to use it overall

I added a bug ticket for the failing MacOS builds: https://www.pivotaltracker.com/story/show/187517298

@lawhead lawhead merged commit 04fe4e8 into 2.0.0rc4 Apr 29, 2024
4 of 6 checks passed
@lawhead lawhead deleted the pause-error branch April 29, 2024 22:57
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.

2 participants