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

feat: add support for wireless fpga-based manchester daq #14

Merged
merged 24 commits into from
Apr 9, 2024

Conversation

phildong
Copy link
Contributor

@phildong phildong commented Oct 23, 2023

add support for wireless fpga-based manchester daq

TODO before merging:

  • Test with Opal kelly FPGA Board
  • Test with UART interface

…s_fpga_daq/blob/decoder/python/source/okDAQ.py

WIP, currently broken because:
- no dependencies has been specified
- `ok_utils` doesn't exist yet
- has to figure out how to set LD_LIBRARY etc.

more info here: https://docs.opalkelly.com/fpsdk/frontpanel-api/programming-languages/
@phildong
Copy link
Contributor Author

phildong commented Oct 23, 2023

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

@phildong phildong changed the base branch from main to dev October 23, 2023 17:19
@phildong phildong marked this pull request as draft October 23, 2023 17:19
@phildong phildong self-assigned this Oct 23, 2023
@sneakers-the-rat sneakers-the-rat added wireless enhancement New feature or request labels Oct 23, 2023
@sneakers-the-rat
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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/

Copy link
Collaborator

@sneakers-the-rat sneakers-the-rat left a comment

Choose a reason for hiding this comment

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

Nice - so the main things that are must fix

  • sourcing the opal kelley module
  • sourcing the bitfile

Then there are things that are like highly recommended we should fix this sooner rather than later, low effort high reward things:

  • Docs!!!!! Let's make a habit of documenting things as we write them. Let's not get into a place where when it comes time to release this we have a shitload of docs debt and need to rush through that. let's write them as we go so that we can communicate what things are doing to ourselves/avoid needing to read the source to see what something is doing.
  • Unify logging as a function in another module - presumably other classes/modules will want logging so we might as well set that up now, since we init loggers in like 6 different places in different ways here.
  • Make dataclasses for configs - move them out of hardcoded variables and function signatures into separate classes so that we have a clear division between configuration, state, and runtime code.
  • Split out opencv plotting from reading logic

Then lower priority, could happen now or later:

  • Let's think about what we want as far as making a uniform API across these different I/O modules - eg SDCard uses read() -> np.ndarray rather than capture() -> None: As we start splitting up different modules, that will be necessary to have a clear structure there so we don't need to special case a bunch of things in eg. the opencv reader methods if hasattr(cls, capture):... elif hasattr(cls, read):...
  • refactor CLI argparsers and add them to the scripts in pyproject.toml - maybe move all CLI entrypoints to their own module so that we can keep track of them.
  • Tests obvi, but not sure on if this is a "lets merge this ASAP" kind of PR, but ya in the same way as docs, should avoid accumulating technical debt when possible.

miniscope_io/uart_daq.py Outdated Show resolved Hide resolved
import numpy as np
import serial
from bitstring import Array, BitArray, Bits
from routine.ok_utils import okDev

# Parsers for daq inputs
daqParser = argparse.ArgumentParser("uart_image_capture")
Copy link
Collaborator

Choose a reason for hiding this comment

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

move argparser to its own function, out of module level

updateDeviceParser.add_argument('baudrate', help="baudrate")
updateDeviceParser.add_argument('module', help="module to update")
updateDeviceParser.add_argument('value', help="LED value")
updateDeviceParser.add_argument("port", help="serial port")
Copy link
Collaborator

Choose a reason for hiding this comment

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

more docs, defaults, and types for these.

updateDeviceParser.add_argument('value', help="LED value")
updateDeviceParser.add_argument("port", help="serial port")
updateDeviceParser.add_argument("baudrate", help="baudrate")
updateDeviceParser.add_argument("module", help="module to update")
Copy link
Collaborator

Choose a reason for hiding this comment

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

[docs] what are the possible modules? what do they do?

miniscope_io/uart_daq.py Outdated Show resolved Hide resolved
break # esc to quit
print('End capture')
break # esc to quit
print("End capture")

while True:
Copy link
Collaborator

Choose a reason for hiding this comment

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

another strategy to end nicely might be to put a special message through the queue signaling an end - does this end things abruptly or are the processes capable of handling a shutdown command? does that matter?

break # esc to quit
print('End capture')
break # esc to quit
print("End capture")
Copy link
Collaborator

Choose a reason for hiding this comment

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

did all that work to set up loggers only to print here! make this a log message :)

while 1: # Seems like GUI functions should be on main thread in scripts but not sure what it means for this case
while (
1
): # Seems like GUI functions should be on main thread in scripts but not sure what it means for this case
Copy link
Collaborator

Choose a reason for hiding this comment

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

definitely, ya - so this function should probably be a generator where we are yield ing each frame from the queue since you are already (nicely!) stashing those. We should make the last "top level" buffer that holds the frames pretty large (like ~1024 frames or something) to allow for async reading. then ya move plotting to its own class that can consume a method like this if we want plotting (or saving video to disk, etc.)

We can split out the to_video file in io.SDCard as well, so we then have multiple "sinks" for reading to - one to show images, another to save to video, etc. we should probably refactor the package anyway since "io" no longer makes sense as a module name for just the SD Card, and I never liked having sdcard not be the module where SDCard lives.


def updateDevice():
args = updateDeviceParser.parse_args()
moduleList = ['LED', 'EWL']
moduleList = ["LED", "EWL"]

ledMAX = 100
Copy link
Collaborator

Choose a reason for hiding this comment

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

move all these values to another dataclass and document - what does this function do? what are these values?

if module == "LED":
assert value <= ledMAX and value >= ledMIN
if module == "EWL":
assert value <= ewlMAX and value >= ewlMIN
except AssertionError as msg:
Copy link
Collaborator

Choose a reason for hiding this comment

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

most of these checks (checking if something is an integer, etc.) can be moved to the argparser - setting type as int does this for you, as does setting allowed values, etc.) this function could be made much simpler.

@sneakers-the-rat
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

sneakers-the-rat commented Oct 23, 2023

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
Copy link
Contributor Author

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

@sneakers-the-rat
Copy link
Collaborator

sneakers-the-rat commented Oct 24, 2023

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
Copy link
Collaborator

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

* Create `devices` module to separate derivative code from vendor code. avoids more `utils` and allows room for future improvement
* Added weak import test for opalkelly

Doing this initial commit that lacks `libokFrontPanel.so` and `okimpl_fpoip.so` and the `include` contents to see if they are needed.
@sneakers-the-rat
Copy link
Collaborator

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.

@sneakers-the-rat sneakers-the-rat mentioned this pull request Oct 25, 2023
@phildong
Copy link
Contributor Author

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:
    Screenshot from 2023-10-29 17-43-10
    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 🤣

@sneakers-the-rat
Copy link
Collaborator

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
Copy link
Collaborator

opalkelly plz do not describe python as ripe
Screen Shot 2023-10-30 at 12 55 14 PM

@MarcelMB
Copy link
Contributor

MarcelMB commented Mar 8, 2024

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
Copy link
Collaborator

sneakers-the-rat commented Mar 8, 2024

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
Copy link
Contributor Author

phildong commented Mar 8, 2024

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:
Screenshot from 2023-10-29 17-43-10
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
Copy link
Collaborator

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
Copy link
Contributor

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.

Screenshot 2024-03-11 at 3 16 14 PM

@sneakers-the-rat
Copy link
Collaborator

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
Copy link
Collaborator

Do

python -m serial.tools.list_ports

To list current ports

@MarcelMB
Copy link
Contributor

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
Copy link
Collaborator

RIP FPGA

@@ -319,8 +328,8 @@ def _fpga_recv(

def _buffer_to_frame(
self,
serial_buffer_queue: multiprocessing.Queue[bytes],
frame_buffer_queue: multiprocessing.Queue[list[bytes]],
Copy link
Contributor

Choose a reason for hiding this comment

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

multiple instances removed [bytes]

Comment on lines 792 to 809
HAVE_OK = False
try:
from miniscope_io.devices.opalkelly import okDev

HAVE_OK = True
except (ImportError, ModuleNotFoundError) as e:
warnings.warn(f"Cannot import OpalKelly device, got exception {e}")
if not HAVE_OK:
raise ImportError('Requested Opal Kelly DAQ, but okDAQ could not be imported, got exception: {ok_error}')

daq_inst.capture(source="fpga")

Copy link
Contributor

Choose a reason for hiding this comment

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

pushed HAVE_OK to begining of file and added instance of if not HAVE_OK

@MarcelMB
Copy link
Contributor

MarcelMB commented Apr 9, 2024

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
Copy link
Collaborator

t-sasatani commented Apr 9, 2024

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
Copy link
Collaborator

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

@t-sasatani
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

MarcelMB commented Apr 9, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

MarcelMB commented Apr 9, 2024

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
Copy link
Collaborator

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 sneakers-the-rat mentioned this pull request Apr 9, 2024
5 tasks
@sneakers-the-rat
Copy link
Collaborator

#19

@MarcelMB
Copy link
Contributor

MarcelMB commented Apr 9, 2024

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

@MarcelMB MarcelMB marked this pull request as ready for review April 9, 2024 19:24
@MarcelMB MarcelMB merged commit 7a7e8c5 into dev Apr 9, 2024
9 checks passed
@sneakers-the-rat
Copy link
Collaborator

hell ya.

@t-sasatani
Copy link
Collaborator

t-sasatani commented Apr 10, 2024

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!

@t-sasatani t-sasatani deleted the wireless_fpga branch December 13, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wireless
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants