Skip to content

Conversation

bparks13
Copy link
Member

This PR combines the AnalogIO and DigitalIO devices into one composite device (AuxiliaryIO). This includes adding a new UI tab that inherits the respective UIs from both devices.

With these updates, the DigitalIO device is now polled at 25 kHz, the same as the sample rate that the AnalogIO is downsampled to in this plugin. To ensure that events are recorded at the correct timestamps, the AuxiliaryIO device waits for frames from both devices and then processes the frames simultaneously so that the events from DigitalIO are correctly recorded in the AnalogIO event channel.

bparks13 added 3 commits June 20, 2025 17:06
- Introduced AuxiliaryIO class to encapsulate both AnalogIO and DigitalIO devices.
- Modified DigitalIO to configure sample period based on AnalogIO's sample rate.
- Implemented AuxiliaryIOInterface for UI management of both devices.
- Refactored OnixSource to initialize AuxiliaryIO instead of individual AnalogIO and DigitalIO.
- Adjusted UI components for proper layout and interaction with the new AuxiliaryIO.
- Cleaned up unused code and improved overall structure for better maintainability.
@bparks13 bparks13 added this to the Breakout Board Firmware 2.0 milestone Jun 24, 2025
@bparks13 bparks13 requested review from aacuevas and jonnew June 24, 2025 16:46
@bparks13 bparks13 self-assigned this Jun 24, 2025
Copy link
Contributor

@jonnew jonnew left a comment

Choose a reason for hiding this comment

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

Behavior looks good with the following notes.

  1. The event channels should be mapped to the digital inputs instead of the buttons and first two digital inputs. Its unfortunate but I think its just too random to have this mixture of sources in the event channel selections. I guess we could push the GUI team to make the number of channels actually match the number of digital signals that are coming into an LFP viewer. Or have the option to plot them like a normal LFP channel.

  2. We found a huge performance issue while testing on Jon's machine. The CPU usage on two cores is at 100% during acquisition. These cores seem to correspond to those used for plotting the Neuropixels probe configuration outside of acquisition, during which their usage is high to begin with (over 50%). This is a separate issue but needs to be addressed.

Base automatically changed from issue-77 to main July 1, 2025 15:45
@aacuevas
Copy link
Collaborator

aacuevas commented Jul 1, 2025

A couple of comments looking at the code:

  • I see a number of lines like const GenericScopedLock<CriticalSection> frameLock(frameArray.getLock());, in most cases followed by a simple array operation. Is there any reason for this? Juce arrays use the critical section inside each call to lock the thread-sensitive parts of the code, so extra locking is not necessary, unless you need to lock a bigger section of code for some specific reason.
    In fact, in
    const GenericScopedLock<CriticalSection> frameLock(frameArray.getLock());

    or
    const GenericScopedLock<CriticalSection> frameLock(frameArray.getLock());

    These locks are locking a huge block of code without any apparent reason.

Locks should be used sparsely and with care, as a big lock blocks other threads, and can have a big negative impact in overall performance.

  • this is mostly nitpicking, but when using bitmasks, please use hexadecimal. With (buttonState & 63) it's difficult to know how many bits are you taking, (buttonState & 0x3F) is clear that it's 6 bits

Regarding the bit order for events that @jonnew comments, that is an issue only for visualization on the LFP, right? when saving to disk all TTLs should be accounted for. I agree on both remarks, it should be the inputs and we should push towards the LFP supporting more than 8 lines somehow.

@bparks13
Copy link
Member Author

bparks13 commented Jul 1, 2025

@aacuevas the reason for the locks is a lack of understanding on my part. I didn't realize that passing the CriticalSection parameter handled all locking internally, I assumed from my reading of the documentation that the only thing this did was to create a lock that then I could access when needed. If this does have a big performance impact, it might be part of the cause for the performance hits we observed today. I can remove these excess locks that don't do anything useful, and test to see if the performance increases.

For the bitmasks, that is easy to fix, and makes more sense as well. Thanks for the nitpick!

Regarding the events, yes this is only an issue for visualization. Currently I believe that up to 256 TTL bits can be saved per event channel, so we can easily record everything. The visualization only goes up to the first 8 TTL lines, which means we need to make a choice on which 8 to show. I agree that the 8 lines shown should be the digital inputs, however I will state that when we do this, if there is nothing connected to the digital inputs all 8 lines will be pulled high by default, which will greatly clutter the viewer unless the user toggles off all 8 channels.

bparks13 added 5 commits July 1, 2025 14:04
- Map digital inputs to TTL channels 0-7
- Map buttons to TTL channels 8-13
- Remove excess GenericScopedLock calls, these are handled internally by the Array
- Remove extra calls to port controllers to process frames
- Stop acquiring data before clearing individual device arrays to prevent any data being added after clearing the arrays
@bparks13 bparks13 requested a review from jonnew July 1, 2025 21:24
Copy link
Contributor

@jonnew jonnew left a comment

Choose a reason for hiding this comment

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

  • Get rid of enable button on Digital Input because its tied to analog input now
  • Combined the top level heading in the configuration to Analog & Digital IO
  • Change the order of tabs so Aux IO is first and Clock out is last

@bparks13 bparks13 requested a review from jonnew July 8, 2025 19:20
Copy link
Contributor

@jonnew jonnew left a comment

Choose a reason for hiding this comment

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

Tested. Used Analog and digital IO disable and it got rid of stream from lfp viewer. Looks good!

@bparks13 bparks13 merged commit 550b7b2 into main Jul 9, 2025
@bparks13 bparks13 deleted the issue-42 branch July 9, 2025 13:42
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.

Combine AnalogIO and DigitalIO into a single abstract device Events and ONI

3 participants