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

Added impedance mode for ANT neuro devices #751

Merged
merged 20 commits into from
Jan 13, 2025

Conversation

bdieudonne
Copy link
Contributor

No description provided.

@bdieudonne bdieudonne changed the title Added impedance mode for ANT neuro devices Draft: Added impedance mode for ANT neuro devices Dec 23, 2024
@bdieudonne bdieudonne marked this pull request as draft December 23, 2024 15:15
@bdieudonne bdieudonne marked this pull request as ready for review December 23, 2024 15:51
@bdieudonne bdieudonne changed the title Draft: Added impedance mode for ANT neuro devices Added impedance mode for ANT neuro devices Dec 23, 2024
@Andrey1994
Copy link
Member

Hi, thanks, I will review tomorrow

Copy link
Member

@Andrey1994 Andrey1994 left a comment

Choose a reason for hiding this comment

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

there is get_resistance_channels methods in brainflow api which is supposed to be used to mark impedance values.
ANT neuro devices have quite a lot of channels so I dont think that we should extend the whole package with 2 times more channels, but we can add resistance_channels into brainflow_boards.cpp and make them the same as exg channels. simplified example:

eeg_channels: {1,2,3,4...}
resistance_channels: {1,2,3,4...}

Then in user api it will be possible to write smth like:

resistance_channels = BoardShim.get_resistance_channels(board_id)
impedance_data = data[resistance_channels]


.. code-block:: python

params = BrainFlowInputParams()
board = BoardShim(BoardIds.ANT_NEURO_EE_410_BOARD, params)
board = BoardShim(BoardIds.ANT_NEURO_EE_410_BOARD, params) # 8 channel amplifier
Copy link
Member

Choose a reason for hiding this comment

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

that should not be here, its meant to be just a part to init the device
device specific code samples can be pushed into python_package/examples/tests
You can create an example with impedance checking and add a link for it to the docs

@bdieudonne
Copy link
Contributor Author

bdieudonne commented Jan 3, 2025

Thanks for reviewing! I pushed some updates based on your comments. Some remarks:

  • The listing of eeg/emg/resistance/... channels manually feels a bit error prone. E.g., when you accidentally set the board to AntNeuroEE410 instead of AntNeuroEE411, you lose all EEG data (because that one has no eeg_channels defined) -- while the ANT sdk itself could easily work device agnostic.
  • I put the sampling counter at the end instead of at the beginning of the data package array (I saw the same is done for the gtec Unicorn), to avoid that corresponding channels would have different indices in EEG mode versus in impedance mode.
  • I also removed sampling_rate from the brainflow_boards.cpp, as it is not fixed. Example to explain why:
params = BrainFlowInputParams()
board = BoardShim(BoardIds.ANT_NEURO_EE_410_BOARD, params)
board.prepare_session()
board.config_board('sampling_rate:500')
print(board.get_sampling_rate(board.board_id))

--> this would yield 2000, while I'd expect 500

@Andrey1994
Copy link
Member

I also removed sampling_rate from the brainflow_boards.cpp, as it is not fixed. Example to explain why:

that should be restored, its known that sampling rate returned by BoardShim can be wrong if its manually overridden with config_board, it comes from the fact that its static info but there must be a default

src/board_controller/ant_neuro/ant_neuro.cpp Outdated Show resolved Hide resolved
src/board_controller/ant_neuro/ant_neuro.cpp Show resolved Hide resolved
src/board_controller/brainflow_boards.cpp Show resolved Hide resolved
{"timestamp_channel", 10},
{"marker_channel", 11},
{"package_num_channel", 0},
{"package_num_channel", 9},
Copy link
Member

Choose a reason for hiding this comment

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

package num is the first one for all boards and makes sense to keep it consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that ANT EEG streams have N+2 channels (N EEG + triggers + sample counter) and impedance streams have N+2 channels as well (M=N EEG + reference electrode + ground electrode). To have them both fit in the brainflow channels (now: N+2 + marker + time stamp), I saw four "reasonable" options:

  1. Keep EEG stream as before --> EEG and impedance channels don't match, and there's no (room for) package num for impedances
    • EEG (as before):
      • chan 0: package num (= sample counter)
      • chan 1-N: EEG channels
      • chan N+1: triggers
      • chan N+2: time stamp channel
      • chan N+3: marker channel
    • impedances:
      • chan 0-N-1: EEG channels
      • chan N: reference channel
      • chan N+1: ground channel
      • chan N+2: time stamp channel
      • chan N+3: marker channel
  2. Set package num at N+1 for the EEG stream --> similar to gtec Unicorn integration, agrees exactly with the default channel numbering of ANT SDK, and EEG and impedance channels match (also no package num for impedances)
    • EEG:
      • chan 0-N-1: EEG channels
      • chan N: package num (= sample counter)
      • chan N+1: triggers
      • chan N+2: time stamp channel
      • chan N+3: marker channel
    • impedances:
      • chan 0-N-1: EEG channels
      • chan N: reference channel
      • chan N+1: ground channel
      • chan N+2: time stamp channel
      • chan N+3: marker channel
  3. Add an extra channel (N+5 channels) --> everything fits as well, but needs extra channel. Channels don't match exactly between EEG and impedances.
    • EEG:
      • chan 0: package num (= sample counter)
      • chan 1-N: EEG channels
      • chan N+1: triggers
      • chan N+2: empty
      • chan N+3: time stamp channel
      • chan N+4: marker channel
    • impedances:
      • chan 0: empty/manual package num
      • chan 1-N: EEG channels
      • chan N+1: reference channel
      • chan N+2: ground channel
      • chan N+3: time stamp channel
      • chan N+4: marker channel
  4. Add two extra channel (N+6 channels) --> everything fits as well, every channel has its own dedicated index, but needs 2 extra channels
    • EEG:
      • chan 0: package num (= sample counter)
      • chan 1-N: EEG channels
      • chan N+1: empty (~reference)
      • chan N+2: empty (~ground)
      • chan N+3: triggers
      • chan N+4: time stamp channel
      • chan N+5: marker channel
    • impedances:
      • chan 0: empty/manual package num
      • chan 1-N: EEG channels
      • chan N+1: reference channel
      • chan N+2: ground channel
      • chan N+3: empty (~triggers)
      • chan N+4: time stamp channel
      • chan N+5: marker channel

I think option 1 is a no-go as it is confusing to have the EEG channels not match; I choose option 2 as it agrees best with ANT's sdk. Let me know what you think

Copy link
Member

Choose a reason for hiding this comment

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

I like option 4 with manual package num for impedance, its closer to what is implemented in other boards.

It also will be a breaking change since brainflow has playbackfileboard and read_file methods and all these 4 options will lead to the need to update recorded files. In option 4 it can be made with less harm, package number is at index 0 as before and eeg channels are at indexes 1-N as before, so for example this code will just work with older files:

data = DataFilter.read_file("old_ant.data")
eeg_channels = BoardShim.get_eeg_channels(board_id)
eeg_data = data[eeg_channels]

We technically can keep timestamps and markers working with old files if we put

  • chan N+1: reference channel
  • chan N+2: ground channel
  • chan N+3: empty (~triggers)

After timestamp and marker channels and dont change locations for timestamp and marker. But it will look a little bit strange, its common to put timestamps and markers in the end. So lets update it exactly like you propose in option 4

Copy link
Member

Choose a reason for hiding this comment

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

  • chan N+1: reference channel
  • chan N+2: ground channel
  • chan N+3: empty (~triggers)

And

  • chan N+1: empty (~reference)
  • chan N+2: empty (~ground)
  • chan N+3: triggers

Should go to other_channels, right? Update in docs will be needed to reflect that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how other_channels is defined. Would you consider ground and reference as "other channels", if they are already listed in resistance_channels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated according to option 4 (hope i didnt make any typos)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(it is still a breaking change, as the trigger channel, timestamp channel and marker channel have moved, as you noted)

@bdieudonne bdieudonne marked this pull request as draft January 9, 2025 15:55
@bdieudonne bdieudonne marked this pull request as ready for review January 9, 2025 16:55
@bdieudonne bdieudonne requested a review from Andrey1994 January 9, 2025 16:55
Signed-off-by: Andrey Parfenov <[email protected]>
Signed-off-by: Andrey Parfenov <[email protected]>
@Andrey1994 Andrey1994 merged commit 7a63d28 into brainflow-dev:master Jan 13, 2025
11 checks passed
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