Acquisition device refactor #122

lawhead · 2021-01-14T01:00:34Z

Overview

Refactored the acquisition module to separate the concept of a device (ex. DSI-24 headset) and a connection method to that device (TCP or LSL).

Ticket

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

Contributions

Created additional parameters for acq_device and acq_connection_method.
Added DeviceSpec class and a module for loading a list of supported devices from configuration; refactored many classes to take a DeviceSpec parameter, including the fake data servers.
Created an enumeration for supported ConnectionMethods.
Refactored previous concept of Device -> Connector.
Refactored fake data servers to simplify their usage. Renamed data_server module to tcp_data_server.
Fixed all acquisition demos.
Refactored lsl_connector in preparation for future work with multiple sources.

Test

The new acquisition parameters should appear in the params form and be editable.
All acquisition module tests should run without error.
All acquisition demos should run correctly.
Data acquisition during an experiment should use the configured device and connection method parameters and should behave the same as before, including trigger calibration.

Documentaion

Updated the README in the acquisition module.

…configuration.

…tor; fixed acquisition demos and tests; added unit tests for new functionality

tab-cmd

Some suggestions, but works locally for me and I like the approach!

tab-cmd · 2021-01-14T23:14:19Z

bcipy/acquisition/client.py

@@ -87,6 +90,9 @@ def __init__(self,
        self._max_wait = 0.1  # for process loop

        self.marker_writer = NullMarkerWriter()
+
+        # column in the acquisition data used to calculate offset.
+        self.offset_column = OFFSET_COLUMN_NAME


I think it might be more meaningful to call it a trigger column and require that it's present. We're going to refactor the way we handle offset correction in a future ticket.

Yeah, I went back and forth on this one. I'll go ahead and rename it to trigger_column for clarity.

tab-cmd · 2021-01-14T23:16:05Z

bcipy/acquisition/datastream/lsl_server.py

-        info = StreamInfo(stream_name,
-                          "EEG", len(channels),
-                          self.sample_hz,
+        log.debug("Starting LSL server for device: %s", device_spec.name)


use f strings here

Logging is actually the one place where python is optimized to use the old string formatting style and f-strings can lead to some inefficiencies: https://docs.python.org/3/howto/logging.html#optimization. In this particular case it's probably not a big deal, but some code linters will provide a warning.

I didn't know that! I just assumed f strings are more performant.

tab-cmd · 2021-01-14T23:17:48Z

bcipy/acquisition/devices.json

+        "description": "Wearable Sensing DSI-VR300"
+    },
+    {
+        "name": "g.USBamp-1",


I like that we're registering our devices this way

tab-cmd · 2021-01-14T23:18:13Z

bcipy/acquisition/devices.py

+                      description=config['description'])
+
+
+SUPPORTED_DEVICES = {}


let's move this to the top of the file

tab-cmd · 2021-01-14T23:18:50Z

bcipy/acquisition/devices.py

+
+def supported_devices() -> Dict[str, DeviceSpec]:
+    """Returns the currently supported devices keyed by name."""
+    global SUPPORTED_DEVICES


why is this call needed?

I should prefix SUPPORTED_DEVICES with an underscore. My intent is that it's a private implementation detail of the module and shouldn't be accessed directly.

tab-cmd · 2021-01-14T23:19:21Z

bcipy/acquisition/devices.py

+
+
+# Initialize the SUPPORTED_DEVICES
+load()


Could this be called in init_acquistion or in some other fashion?

I'll re-work this a bit to call load when SUPPORTED_DEVICES is first accessed. There is other code that uses this module, so I don't want to make that code always have to explicitly load devices, but I agree that this doesn't need to be done at import time.

tab-cmd · 2021-01-14T23:21:21Z

bcipy/acquisition/protocols/lsl/lsl_connector.py

@@ -46,49 +49,87 @@ def inlet_name(inlet) -> str:
    return name.replace('-', '_')


-class LslDevice(Device):
-    """Driver for any device streaming data through the LabStreamingLayer lib.
+def channel_names(stream_info: pylsl.StreamInfo) -> List[str]:


seems like these could be a part of the lsldevice class?

I originally had this as a class method, but it didn't reference any instance variables and I got a linter warning about it so I pulled it out into a utility function. It may be useful in other contexts when we are looking at multiple LSL sources.

tab-cmd · 2021-01-14T23:21:38Z

bcipy/acquisition/protocols/lsl/lsl_connector.py

@@ -231,8 +230,7 @@ def read_data(self):
            if not marker.is_empty and timestamp >= marker.timestamp:
                trg = marker.trg
                log.debug(("Appending %s marker %s to sample at time %s; ",


use f strings

tab-cmd · 2021-01-14T23:24:20Z

bcipy/acquisition/tests/test_client.py

+        data = [
+            mock_record(n_channels) + [0 if (i + 1) < trigger_at else 1]


It could be useful to define a function to produce this or set it as a class value

Yeah, I still want to take a pass through the code and revisit mocked EEG data.

lawhead added 9 commits December 14, 2020 15:40

Renamed data server to tcp_server. Refactored to simplify tcp_server …

c0c076a

…configuration.

Acquisition refactor; WIP

c9d6782

Refactor code to configure the MarkerWriter when using the LSL connec…

11864bf

…tor; fixed acquisition demos and tests; added unit tests for new functionality

Merge latest from 1.4.3 branch

cfaa7cb

Renamed device to connector

6a169c3

Fixed viewer demo; added convenience method for creating a connector

9cbd1dc

Updated documentation for the acquisition module

a26e405

Code cleanup

4706fc6

Merged latest from upstream

bc813ef

lawhead requested a review from tab-cmd January 14, 2021 01:00

tab-cmd approved these changes Jan 14, 2021

View reviewed changes

lawhead added 2 commits January 15, 2021 15:19

Refinements for device loading

63b8755

Merged latest from 1.4.3

493e703

lawhead merged commit ddbcf51 into 1.4.3 Jan 15, 2021

tab-cmd mentioned this pull request Jan 20, 2021

1.5.0 #96

Merged

4 tasks

lawhead deleted the acq-device-refactor branch January 29, 2021 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Acquisition device refactor #122

Acquisition device refactor #122

lawhead commented Jan 14, 2021

tab-cmd left a comment

tab-cmd Jan 14, 2021

lawhead Jan 15, 2021

tab-cmd Jan 14, 2021

lawhead Jan 15, 2021

tab-cmd Jan 15, 2021

tab-cmd Jan 14, 2021

tab-cmd Jan 14, 2021

tab-cmd Jan 14, 2021

lawhead Jan 15, 2021

tab-cmd Jan 14, 2021

lawhead Jan 15, 2021 •

edited

Loading

tab-cmd Jan 14, 2021

lawhead Jan 15, 2021

tab-cmd Jan 14, 2021

tab-cmd Jan 14, 2021

lawhead Jan 15, 2021

		data = [
		mock_record(n_channels) + [0 if (i + 1) < trigger_at else 1]

Acquisition device refactor #122

Acquisition device refactor #122

Conversation

lawhead commented Jan 14, 2021

Overview

Ticket

Contributions

Test

Documentaion

tab-cmd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawhead Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawhead Jan 15, 2021 •

edited

Loading