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

Workaround for impedance measurement bug #19

Merged
merged 4 commits into from
May 1, 2024

Conversation

shababo
Copy link
Contributor

@shababo shababo commented Apr 14, 2024

Addressing the issue regarding impedance test bugs with the new board.

After some debugging, it seemed that the issue with running impedance measurements at sampling rates other than 30 kS/s was failing due to the estimate of how many blocks to write/read. The original line to compute numBlocks was

int numBlocks = ceil((numPeriods + 2) * period / 60.0);

numBlocks is used to determine the maxTimeStep and the number of frames to read via setting the number of Rhd2000DataBlocks. Feels like the magic number, 60.0 is a potential culprit... doesn't really make sense... the denominator here should probably be float(SAMPLES_PER_DATA_BLOCK(board->evalBoard->isUSB3()).

I tried to sus out what the correct calculation should be by digging through other parts of the code and the chip datasheets - but after much obsessive sleuthing and several changes that made things better but still fail, I decided I should just pass on what I've found to someone more familiar with how the chip works.

As a workaround, I noticed that in DeviceThread::scanPorts, the sampling rate is temporarily set to 30 kS/s. For now, I've used this approach for impedance measurements as well and it seems to work. I have a component with a known of impedance of 4.5 kOhm which I used to verify this change.

Some potential clues on the core issue:

  • In the original code, the issue was an infinite loop that occurred when reading frames in board->evalBoard->readDataBlocks(numBlocks, dataQueue);. The infinite loop is a result of (*frame)->dev_idx == DEVICE_RHYTHM being false for the frames that are read.
  • When you hard code numBlocks = 11 which is the value computed for 30 kS/s, there's no more infinite loop for all sampling rates but the calculation only correct for 30 ks/S. Not sure if there's anything to this, but it makes me think that somewhere else in the code, there's another value that implicitly depends on the sampling rate being 30 kS/s - like 60.0. Or maybe some chip setting for outputs isn't set or isn't compatible with subsampled rates?
  • There's potentially some interplay with setting the maxStepSize.
  • I changed 60.0 to float(SAMPLES_PER_DATA_BLOCK(board->evalBoard->isUSB3()), and the impedance test still works for 30 kS/s even though numBlocks = 3 now. However, it still fails for other rates. Nonetheless, I left this change in since I think it's correct.

In any case, the current workaround seems fine, and it doesn't add much time to switch sampling rates.

@jsiegle
Copy link
Contributor

jsiegle commented Apr 18, 2024

Thanks a lot for opening this PR. I don't see any reason why we can't just change the sample rate prior to impedance testing, then switch it back. @aacuevas does this seem reasonable to you? If so, we can merge and put out a new release.

@shababo
Copy link
Contributor Author

shababo commented Apr 19, 2024

@jsiegle Also, I coded up some config messages for the new acq board to set the impedance channels to test and to call the impedance test.

Line 283 in DeviceThread.cpp is what needs confirmation - there were several ways to trigger the impedance measurement and this one seems most reliable.

Also, thinking about adding channel selection to the GUI, but not sure I have time soon to figure that out. Was looking at some of the other GUI elements to pattern match/steal from. Not sure if y'all would want to add this if you think it wouldn't take you much time.

Another non-essential feature that would be nice is opening the impedance visualizer window programmatically - again, something that would be hard for me to prioritize in the very near future.

@jsiegle
Copy link
Contributor

jsiegle commented Apr 29, 2024

That impedance triggering method looks good! Can you make the following changes and then push your code?

  1. Only allow impedance settings to change via config messages. Broadcast messages are short commands that are sent to all processors during acquisition, whereas config messages are sent to specific processors, can be much longer, and can be sent at any time.
  2. Check that !acquisitionIsActive is true before running any impedance tests. This will prevent accidentally starting impedance testing while data is streaming.

@jsiegle
Copy link
Contributor

jsiegle commented Apr 29, 2024

Re: channel selection -- can you provide more details about what you mean by this?

For opening the impedance window programmatically -- visualizer windows will open automatically in version 1.0, which is coming soon, so you won't have to worry about this in the future.

@anjaldoshi anjaldoshi merged commit beb1770 into open-ephys-plugins:main May 1, 2024
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