feat: add support for wireless fpga-based manchester daq

[phildong]

add support for wireless fpga-based manchester daq

TODO before merging:

• [x] Test with Opal kelly FPGA Board
• [ ] Test with UART interface

[phildong]

This is WIP. as the commit message suggests, still need to figure out how to specify dependencies and include opal kelly libraries

[sneakers-the-rat]

Alright! lets start with docs - can we get some module, class, and method-level docstrings to let us know what this module, class, and methods do? The docs are set up to use google style docstrings but regular style should work too

[sneakers-the-rat]

Added dependency for bitstring , i see that routine is the module that has the opal kelley stuff in it ( https://github.com/phildong/wireless_fpga_daq/tree/decoder/python/source/routine ) - where did that come from? what kind of license does it have? is it something we could package here, or would we need to refer people to download it elsewhere?

[sneakers-the-rat]

Enabled building docs for this branch so you can see the state of the docs as you write them: https://miniscope-io.readthedocs.io/en/wireless_fpga/ https://miniscope-io.readthedocs.io/en/wireless_fpga/api/uart_daq.html

see build status here: https://readthedocs.org/projects/miniscope-io/builds/

[sneakers-the-rat]

To make this testable, we would want to split out the operations that are happening in the individual functions from their iteration, which would be good to do generally - so rather than having a method that does some setup and then terminates in a while 1: ... loop, we have the method take arguments and return the thing that would go in the queue, and then have maybe even a single function or class that can be used to loop each of those methods getting and putting in the receive/send queues.

As is these methods would be sort of hard to test because of the way they are dependent on a ton of setup state, but it woudln't be that hard if eg. the queues or the multiprocessing Manager were stashed as class attributes that we could grab in a testing context.

[phildong]

ok I can answer the questions as much as I can, but in general, I'm wondering if we should do all the improvements in this PR or if we should separate them out? Ideally this PR is about adding fpga support rather than improving the script in general. And this PR already contains a lot more than there should be (ideally I should just plug in the fpga receiver code but that didn't work out). I'm happy to do everything in this PR and change the name to something like DAQ 2.0 lol but that will take much longer and I'm not sure if that fits @t-sasatani 's timeline.

[phildong]

Added dependency for bitstring , i see that routine is the module that has the opal kelley stuff in it ( https://github.com/phildong/wireless_fpga_daq/tree/decoder/python/source/routine ) - where did that come from? what kind of license does it have? is it something we could package here, or would we need to refer people to download it elsewhere?

it's proprietary and sort of behind a paywall (you can download for free only if you register and prove you've purchased opal kelly hardware), and there is no easy installer (some manual copy-pasting needs to happen after you download a zip from opalkelly), this is why initially I just git tracked the proprietary codes, but it's probably no longer a good idea.

[sneakers-the-rat]

ok I can answer the questions as much as I can, but in general, I'm wondering if we should do all the improvements in this PR or if we should separate them out?

ya i split review comment into "must fix," "should fix," and "nice to have" for this reason - i think resolving the two basic mustfixes should be done before merging (because the routine import would break the existing uart code, and without the bitfile the fpga part wouldn't work), and then we can address the others in separate PRs if we need this to be merged ASAP.

it's proprietary and sort of behind a paywall (you can download for free only if you register and prove you've purchased opal kelly hardware)

Is it distributed with a license? Wondering what we have in mind for this package - it's not unheard of to need to manually download some proprietary plugin, but its definitely not the most convenient thing. At least we need to provide docs about what it is and where to get it. If the license allows us to just package it here that would be ideal. I can help ID that if you link me to where it comes from :)

[phildong]

found the license! https://opalkelly.com/about-us/frontpanel-license/

[sneakers-the-rat]

well that sucks

Licensee shall not modify, distribute, resell or otherwise transfer Software for any purpose, commercial or non-commercial. Licensee may integrate and sub-license Software for distribution but Software must only be used in conjunction with Opal Kelly devices or devices incorporating Firmware.

So that poses a sort of big problem - we can't really... distribute this thing like that. at all. I am not sure what the grant of sub-licensing means - if we can do that at will, or if we have to ask them. Is it "you may sub-license this from us" or "by virtue of you already having the software, you may apply a different license." So either it's no problem at all and we can just copy/paste that into the package, or it's a huge deal and we have to rearchitect things.

edit: so what does that module do exactly? i see configure FPGA and read data, but like can we use a generic USB module for that?

[sneakers-the-rat]

It looks like OE vendors in the C libraries? https://github.com/open-ephys-plugins/rhythm-plugins/tree/main/Resources

[sneakers-the-rat]

Added in the opalkelly stuff, did some minor refactoring to keep vendor code separated from ours, and also made a devices module because it seems like we'll need that eventually anyway - ie. the fpga code and the uart code should eventually be separated and their common parts should use composition or inheritance rather than differently named modules.

But as you can see, the tests fail because the module can't be imported. We could do the import manually in the containing __init__ file, but when i try and import it directly on my mac I get a platform incompatibility error, which is sort of expected.

@phildong did you compile these yourself, or did the SDK download come with precompiled binaries? If it came with the source, we can just compile them as part of the packaging process as a C extension. Otherwise, we will need standalone binaries for win/mac as well as linux.

[phildong]

ok finally got around to work on this. Thank you so much @sneakers-the-rat for setting this up and sorry again for the lack of response! Just pushed some commits and I think there are 3 things left to do:

1. obtain the binaries for mac and windows. I can download those files but they're packaged in .dmg and .exe respectively, so we either need to find some windows/mac computers or need to figure out how to "unpack" those files on linux. (@MarcelMB should already have the mac files though)
2. figure out how to symlink based on OS. as you can see I pushed some symlink from the ubuntu-specific folder to the lib folder. We either need to figure out how to do this automatically based on OS (__init__.py maybe?), or come up with better solution than symlink.
3. figure out how to set environment variables before python execution as documented here: I was able to do this with a bash script: https://github.com/phildong/wireless_fpga_daq/blob/183f8d4a6c33ec0910a9c4791c65fdd72f31311d/python/start_interface.sh but I feel there must be a better solution

at the moment I have no clue what's the best way to do either of those 3 things and how they would interact with the rest of the repo, so I'd love some help before I come up with some very broken and hacky way to do them :rofl:

[sneakers-the-rat]

ok so @phildong can you send me a copy of what the SDK comes to you as, fresh from being downloaded from OK? that should help clarify what we need to do here :)

[sneakers-the-rat]

opalkelly plz do not describe python as ripe

[sneakers-the-rat]

Got mac import working, added a README file https://github.com/Aharoni-Lab/miniscope-io/blob/209b560d8909404bcd80bd4f7e536f6a68fd67b1/miniscope_io/vendor/opalkelly/README.md

[sneakers-the-rat]

OK Got this working for linux and mac, marked windows as not implemented.

Added documentation on what was done to make them work here: https://github.com/Aharoni-Lab/miniscope-io/blob/111109baf9d25ff0103f13f615ec898f98c47f45/miniscope_io/vendor/opalkelly/README.md

And also added special installation instructions needed for debian installs (this doesn't seem to be necessary in ubuntu, at least according tot he test runner): https://github.com/Aharoni-Lab/miniscope-io/blob/111109baf9d25ff0103f13f615ec898f98c47f45/docs/guide/installation.md

which is rendered here: https://miniscope-io.readthedocs.io/en/wireless_fpga/guide/installation.html

It would be great if y'all could also contribute some docs about how these things work and what they require to work before merging the code, but if this is for some reason a time-critical merge then we can pop another issue to remind us to it later, but we shouldn't leave new additions to the package undocumented or else the thing starts to turn into a pile of spaghetti where we lose visibility into what's actually in it

I have written a very weak test function that just tests the import of the module - to make sure this actually works, i would strongly recommend that the folks who know how this works add some test code that actually test the functionality of the packaged drivers. This can even be done by marking them as skip when running the tests normally, but runs them only when there actually is an FPGA board on the testing machine. ( see https://docs.pytest.org/en/6.2.x/skipping.html )

Otherwise the only guarantee we have that this code works is vibes and that is not a guarantee suitable for code that we distribute.

[sneakers-the-rat]

I think the only thing remaining here thats a must fix is adding the bitfile? @MarcelMB said @phildong generated it? Is that something that is a static file thats always the same, or is it something that might evolve with the FPGA (and thus would need to be versioned?)

Then I say we merge this and ill raise the rest of my comments in review as issues

[sneakers-the-rat]

OK Removed the bitfile from the method args and added some TODO items for documentation.

Also replaced the mutable defaults in the function signatures with data models that match the models used in the SDCard model. Note for myself in there to think about how that should be structured -- want to get it so all the I/O classes speak the same language.

I think those are all the must-fixes? so once Takuya approves then i think we can merge?

[t-sasatani]

Sorry to be so late. I'm finally working on this. It will take some time for me to read stuff about documenting noted in this thread (tho we need to do it for sure), so let's prioritize making it work in this PR.

1. I moved the import of the vendor modules to the main() function (which I believe @phildong is also calling first in the Opal Kelly config) because it stops execution for Windows, but there is probably a better way to take care of this.

2. @phildong I guess the format you're handing down the buffers to the first queue (serial_buffer_queue) is modified from what the microcontroller directly sends out. Could you share the format of these BitArrays?

3. An error I'm confused about now is the following. It seems that self.preamble is defined correctly in __init__ (I confirmed this by looking at the variable in the constructor), but it doesn't get called as I expected in _parse_header(). I assume it's a stupid misunderstanding on my side, but please let me know if you notice something. I think this will be called similarly in both UART and Opal Kelly mode, so it'll help if I can know if it works in @phildong 's setup

It gets the following errors when doing the len(self.preamble) operation:

  File "C:\Users\Takuya\GitHub\miniscope-io\miniscope_io\stream_daq.py", line 255, in _buffer_to_frame
    header_data, serial_buffer = self._parse_header(serial_buffer)
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Takuya\GitHub\miniscope-io\miniscope_io\stream_daq.py", line 114, in _parse_header
    pre_len = len(self.preamble)
              ^^^^^^^^^^^^^^^^^^
  File "C:\Users\Takuya\AppData\Local\pypoetry\Cache\virtualenvs\miniscope-io-A3GK8JhQ-py3.11\Lib\site-packages\bitstring\classes.py", line 669, in __len__
    return self._getlength()
           ^^^^^^^^^^^^^^^^^
  File "C:\Users\Takuya\AppData\Local\pypoetry\Cache\virtualenvs\miniscope-io-A3GK8JhQ-py3.11\Lib\site-packages\bitstring\classes.py", line 1569, in _getlength
    return len(self._bitstore)
           ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Takuya\AppData\Local\pypoetry\Cache\virtualenvs\miniscope-io-A3GK8JhQ-py3.11\Lib\site-packages\bitstring\bitstore.py", line 166, in __len__
    if self.modified:
       ^^^^^^^^^^^^^
AttributeError: 'BitStore' object has no attribute 'modified'
End capture

Here's what happens if I add a print(self.preamble) just before the len(self.preamble)

Process Process-3:
Traceback (most recent call last):
  File "C:\Users\Takuya\AppData\Local\Programs\Python\Python311\Lib\multiprocessing\process.py", line 314, in _bootstrap
    self.run()
  File "C:\Users\Takuya\AppData\Local\Programs\Python\Python311\Lib\multiprocessing\process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "C:\Users\Takuya\GitHub\miniscope-io\miniscope_io\stream_daq.py", line 256, in _buffer_to_frame
    header_data, serial_buffer = self._parse_header(serial_buffer)
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Takuya\GitHub\miniscope-io\miniscope_io\stream_daq.py", line 114, in _parse_header
    print(self.preamble)
  File "C:\Users\Takuya\AppData\Local\pypoetry\Cache\virtualenvs\miniscope-io-A3GK8JhQ-py3.11\Lib\site-packages\bitstring\classes.py", line 682, in __str__
    length = self.len
             ^^^^^^^^
  File "C:\Users\Takuya\AppData\Local\pypoetry\Cache\virtualenvs\miniscope-io-A3GK8JhQ-py3.11\Lib\site-packages\bitstring\classes.py", line 591, in __getattr__
    raise AttributeError(f"'{self.__class__.__name__}' object has no attribute '{attribute}'.")
AttributeError: 'Bits' object has no attribute 'len'.

[sneakers-the-rat]

Wat r we waiting on here @phildong @t-sasatani - lets merge this pup so we can get everyone running the same code, I think @MarcelMB is still using the old repo code

[sneakers-the-rat]

so wats da deal with this? there is some interest from @hsemwal in making a python interface to the griniscope, and i'm not sure what @MarcelMB is doing to test the thing, but it would be cool to merge this so we can all be using the same tools together. not sure what the wireless miniscope ppl are thinking re: timeline but it would probably be nice to launch with a polished interface, and so we could do like a joint paper re: redoing the miniscope daq stuff to say 'we not only want to make dope microscopes, but microscopes that are easy to use and maintain.'

thimkin bout issues like these https://github.com/Aharoni-Lab/Miniscope-DAQ-QT-Software/issues/70 that we can't really deal with bc we don't have a good interface distributed along with the scopes that can do stuff like run diagnostics or ensure system state to avoid common bugs, and the path there goes through having more shared code, and this is an easy, low stakes place to start <3

[hsemwal]

I am certainly down to write up an interface for all these Miniscopes. Im thinking something where the program can run different miniscopes (we are generalizing the firmware code to this end), and automatically configuring the program (so no click and dragging Java scripts to change imaging, ie FOV, FPS, gain). maybe we could start this week and have something setup by the start of summer when we are doing in vivo testing?

[t-sasatani]

I bought the Opal Kelly board and tried to run this, but found bugs around handing over data between queues. So @MarcelMB should be using the old repo.

• I need to figure out some stuff in the FPGA code before fixing this code.
• I'm thinking of ignoring UART compatibility for now because only I was using it.
• I'm planning to work on these issues soon, but am having trouble finding time to sit on the bench.
• This PR has so many topics mixed up now so I (or maybe @MarcelMB if he's down) will merge this PR as soon as it works on the Opal Kelly board.
• After merging, we'll have a primitive CLI interface and can think about making that better.

[sneakers-the-rat]

Im thinking something where the program can run different miniscopes (we are generalizing the firmware code to this end), and automatically configuring the program (so no click and dragging Java scripts to change imaging, ie FOV, FPS, gain).

that is what this package is for!

So we would have some base class like

from abc import ABC, abstractmethod

class Miniscope(ABC):

    @abstractmethod
    def set(self, cmd: 'Command') -> 'Result':
        ...

    @abstractmethod
    def get(self, value: 'Value') -> 'Result':
        ...

    @abstractmethod
    def start(self):
        ...

    @abstractmethod
    def stop(self):
        ...

    @abstractmethod
    @property
    def config(self) -> 'Config':
        ...

    @config.setter
    def config(self, key: str, val: 'Value'):
        ...

and so on with some minimal set of methods that each miniscope needs to implement, and then use composition to make single command modules for different protocol types, mappings between the common operations for a miniscope and whatever device-specific values are needed to use them, etc. Each of those implied classed like 'Value' and 'Result' and etc. can hold enums of possible values, carry results and do stuff like logging and error handling in a sane way, and so on.

Then we could build a GUI on top of that that would be general across scopes (yes, no manually editing and clicking and dragging JSON). you know, fun for the whole family.

So we are already started! merging this PR gets the wireless scope in the repo, and from there we can refine it into a common public API class like that. If we get some sample outputs from the hardware to mock in we coudl ensure that those tests don't break the code as we go, etc.

Like i'm taking a look at some of the stuff @phildong did here https://github.com/phildong/wireless_fpga_daq/pull/2 and it looks pretty dang good! Not going to link to specific files since it's a private repo, but like all these methods for debugging and analyzing the data could be made into general routines we test and make available to future miniscope users. if we are actively using the same code we intend to distribute internally as well, then it's easier for us in the long term and also makes sure that we don't have any extra secret knowledge that users don't have. otherwise, we get into situations like with the Qt DAQ where since none of us really actively use it and instead have our own personal set of tools to develop with, and it becomes a huge chore on both ends - to debug any problem with the public software, and to maintain 10 different sets of tools. We can make our lives easier <3

---

I bought the Opal Kelly board and tried to run this, but found bugs around handing over data between queues

thanks for getting to what you could @t-sasatani , i imagine it's hard to find time at the bench getting the new lab started! If @MarcelMB can replicate those then i would be happy to help patch them.

I'm thinking of ignoring UART compatibility for now because only I was using it.

Totally cool, we can just mark that as unmaintained and deprecate it after this PR if we end up not needing it, but i think it was still a good exercise in figuring out how to merge overlapping code :)

This PR has so many topics mixed up now so I (or maybe @MarcelMB if he's down) will merge this PR as soon as it works on the Opal Kelly board.

Totally agreed lol, we should address the final parts of it and then merge it so we can then handle the individual pieces separately in smaller increments. We have the luxury still of this not being used publicly (as far as I know) so we don't have to be so cautious about merging, and can iterate even if it's only partially working

[sneakers-the-rat]

Selfishly, I'd like to use this as a practice project to learn Rust: convert the C/C++ in the Qt DAQ repo into rust and then expose it to this package with PyO3, so thats why i am eager to get us working together in this module - i wanna practice writing super fast code to be able to do stuff like running multiple scopes that the other projects will need, but can't do that if all the fun real-world development is happening elsewhere :'(

[MarcelMB]

Hey,

I just saw the messages and my mentions here from the last days. For some reason, I didn't get any email notifications. Not sure why.

So I guess the goal of this PR is to see if we can get an actual image transfer working using the Opal Kelly FPGA board and the 'streaming' code here actually gets images. In the wirless_FPGA branch is only Python code I was just wondering if there is an MCU code here that is also to be used and tested or if I just use the MCU code we have on our Miniscope framework repository and that I used last year fall to do image acquisition?

[sneakers-the-rat]

Yes that is the goal of this package - to move away from having hardware, firmware, and software all tightly coupled in the same package to making software that can generalize across firmwares :) use what you intend to release. If it doesnt work, we fix it. By keeping them separate, we enforce a stable API

[phildong]

I realized this has been going on too long and I already forgot how things work lol. @sneakers-the-rat could you remind me how did you ended up handling the environment variable setup mentioned here?

ok finally got around to work on this. Thank you so much @sneakers-the-rat for setting this up and sorry again for the lack of response! Just pushed some commits and I think there are 3 things left to do: ...

3. figure out how to set environment variables before python execution as documented here: I was able to do this with a bash script: https://github.com/phildong/wireless_fpga_daq/blob/183f8d4a6c33ec0910a9c4791c65fdd72f31311d/python/start_interface.sh but I feel there must be a better solution

Did it get handled with installation script? If so, what's the best way to set things up for testing of this PR? Something like pip install +git:xxxxx?

cc: @MarcelMB

[sneakers-the-rat]

We vendored in the opalkelly code to the vendor subpackage, and used it in the devices subpackage.

We want there to be no env variable manipulation needed visible to the user, if possible, hence the vendoring. Patching env variables outside the package is only necessary if the library files can be in a variable location relative to the rest of the code, and we didnt want ppl to have to sign up for an OK account, download it, set env vars, etc. Patching env vars should happen as a part of importing that subpackage, and if we do need to support variable lib locations we should do so in a .env or config file rather than in a bash script. (I get that it was a temporary hack to get things running, not criticizing just stating design goals)

This is a normal python package, and we want it to behave like one - no special launching logic needed, so to run the code yes you can install it from git like that, but you probably should just clone the repo, switch to this branch, install in a venv. So either

python -m venv venv
source venv/bin/activate
pip install -e . 

or

poetry install
poetry shell

we can make any sort of entrypoints we want to mimic whatever existing scriptlike behavior yall are using, so like for this PR lets just make sure it works and then we can work on making CLI entrypoints, making windows mode work, etc. In future PRs

[MarcelMB]

I am working on it. I was just trying to get data streaming work with the repos we have. Just to check if the hardware is working properly before checking if this PR code is working here. But I can't even connect to the FPGA. There is some problem that I didn't figure out yet. Like I cannot build a connection with the FPGA as I did every other week without changing anything. So I'm stuck at the step. Find the FPGA at the moment. Which never caused a problem before.

[sneakers-the-rat]

Is that the full traceback? And it looks like thats still the old repo. Part of why we should move to a collectively maintained repo is so we can progressively improve UX problems like these as we work on the same tools we distribute for users, so in this case we would want to a) identify the bug, b) resolve the bug if possible, c) add more informative error message, d) add tests to prevent bug from recurring.

So yes exactly why we should merge this and be able to iterate on problems like these. Moving our level of confidence in the software from "I didnt change anything" to "we have tested code we know works under xyz conditions"

[sneakers-the-rat]

Do

python -m serial.tools.list_ports

To list current ports

[MarcelMB]

So this is unfortunately paused because the FPGA is dead. Need to wait until we get the new ones running. Sad but let's hope we can continue very soon

[sneakers-the-rat]

RIP FPGA

[MarcelMB]

I set up the hardware for wireless data transfer of test frames today and tested the stream_daq.py code

With the great help of @sneakers-the-rat we got the code to run together after multiple errors in the software. Evrything works now with the hardware and actually transmitted data wirelessly.

Pushed a new commit to this branch. Had to declare some variables earlier in the code. So just moved their position and removed some type annotations. No drastic changes

@phildong @t-sasatani Maybe you can quickly look over the code and let us know if we are good to go to do the Merge with main

https://github.com/Aharoni-Lab/miniscope-io/pull/14/commits/088c4489d84f9b2e9da5346e0d78e30182264a67

[t-sasatani]

Awesome, and thanks! The changes make sense, and I think they are ready to merge.

Just one thing: Would it be possible to add the FPGA bitstream file you're using in this repo? This can be another small PR, but I think you can also throw it in this PR.

[sneakers-the-rat]

@MarcelMB if you add the bit file in the ok folder I can make a simple lil provider function

[t-sasatani]

Thanks @sneakers-the-rat ! A provider function will be best and it seems like the frontpanel SDK can do that, but if it takes a bit of experimenting on HW, just having a fixed bitstream file here would already be better. The problem on my end is that the synthesis takes near an hour on my mobile PC and it's a bit inconvenient to change parameters and re-upload to find which config works.

[sneakers-the-rat]

Totally ya i just meant like a function that returns a path basically, so that theres one layer in between the file and the consuming class so we can eg. Switch between bitfiles later

[MarcelMB]

I looked through the stream_daq.py file and couldn't find where it is actually taking the .bit file for the FPGA for?

So I was wondering where the line of code is for taking such a file and uploading it to the FPGA? And how in yesterday's test it even knows at which baud rate we are operating. because we need a different bit file for different speeds of the wireless data transfer.

I assume this file is uploading the bit files opalkelly.py

Just asking because right now I don't even know which bit file to upload here since I cant find where it is getting it from.

[sneakers-the-rat]

Huh ya theres a method here but its not called in the main script file thing: https://github.com/Aharoni-Lab/miniscope-io/blob/088c4489d84f9b2e9da5346e0d78e30182264a67/miniscope_io/devices/opalkelly.py#L28

We'll want to clean this code up as we go to make a nice and tidy CLI interface, including exposing setting/reading config like bitfiles.

Basically we want to have a versioned collection of all the bitfiles that can/should be used with the device. I still dont know exactly what they are or do, so that should get documented, but ya we want everything that we would expect ppl to use in here.

[MarcelMB]

ok cool. Makes sense

So I am wondering what @t-sasatani meant? As we run the stream_daq OK yesterday it worked smoothly.

It took 2 seconds to start. So there must be a bit file here somewhere or is it always generated on the go when starting the script? I am just wondering what file I should upload.

[sneakers-the-rat]

Idk! There is no bitfile upload invoked when running that code, I assume it was running whatever was last on the device. Making bitfile selection part of the startup routine can be a future PR I think. Can we merge this and then iterate on that?

[sneakers-the-rat]

https://github.com/Aharoni-Lab/miniscope-io/issues/19

[MarcelMB]

for now, I have added a .bit file that was the last one uploaded to the FPGA into the devices folder next to opalkelly.py

I can go ahead and merge the PR. Haven't done that before but I can try

[sneakers-the-rat]

hell ya.

[t-sasatani]

I assume it was running whatever was last on the device.

Yup this is correct. It was taking time to generate the same bitstream file on my end.

I have added a .bit file that was the last one uploaded to the FPGA into the devices folder next to opalkelly.py

So yes this perfect! Thanks!