Update opalkelly bitfiles

[MarcelMB]

only two out of the opal kelly btifiles were working previously 6 and * MHZ but now added more frequencies. Keeping the old bit files for opal kelly but also uploaded the new ones which require no change on FPGA hardware or any other changes in Miniscope-io despite modifying the bitstream variable in the .yml config file for a recording.

so would be easy to add to main since only bitfiles will be added. no general change in code

[MarcelMB]

ohh my bet. I thought I could only pull request this one commit of bitfiles and not all previous commits and branches from the last month. Sorry about that

[sneakers-the-rat]

Haha yes if you rebase that last commit off main then you can do that. I can help with that on monday

[t-sasatani]

I was playing around locally for my understanding, so I'm posting anyway. @MarcelMB If you do:

git checkout update_opalkelly_bitfiles
git rebase --onto origin/main origin/feature_continuous_run

It should be like this, with only the FPGA bit file changes commited. So, changes after origin/feature_continuous_run are stuck to origin/main. (origin/ isn't really needed, but just in case the local main is not updated.)

You might have to force push after this to rewrite history, but that shouldn't cause problems as long as you're on the Opalkelly branch.

Ref: https://git-scm.com/docs/git-rebase

[t-sasatani]

Some updates I'd like to request other than rebasing are:

• Notes for the FPGA bitfiles convention (in case we forget things like what J2_2 or these frequencies mean). I think this can go somewhere in docs/api/stream_daq.md.
• Can you change the dir name of previous_bitfiles to a convention that expresses what these other bitfiles are and delete files that should technically be the same as the new ones? Or if that is uncertain, I think just deleting stuff in previous_bitfiles should be good because we can always return to earlier commits.

[sneakers-the-rat]

Agreed - ideally we never have anything old just hanging out without a purpose, and also ideally we have a structured system that describes all the parameters that go into making a bitfile and then name/organize them accordingly. If one bitfile supercedes another, it should just take its place bc there is only one name/position/etc. For a bitfile with those parameters.

[MarcelMB]

• I did the rebasing as @t-sasatani gave a hint on hot to do this
• I added nomenclature for the bitfiles so its clear what their names mean
• I got ride of the old bitfiles in this repo

[MarcelMB]

but still having the same pull request with all previous commits to be pushed to main not just this one branch. So not sure

[t-sasatani]

• I moved the nomenclature description out of the YAML example code block and organized it a bit in bullet points.
• Looking at the git graph, it seems like the rebased didn't work out correctly.

Also, I noticed that GitKraken has a nice interactive rebase interface, which might be more intuitive (though it might be worth making it work with CLI). I'm leaving the origin branch untouched except for one commit so you can try it out. But I can also just do it.

So first, make sure you pull and check out the update_opalkelly_bitfiles branch. Then right-click on the latest main commit and select Interactive Rebase update_opalkelly_bitfiles onto main

Then, this should show up.

Then drop the unrelated commits and start rebase,

Then it should end up like those commits directly extend main. If you force-push this, the Git history on the origin repository should also be updated.

[MarcelMB]

I did the interactive rebase as @t-sasatani suggested and worked all well. Thanks!!!

[MarcelMB]

After rebasing is there still a PR needed since its already on main?

[t-sasatani]

Awesome! The Git graph looks good now.

After rebasing is there still a PR needed since its already on main?

Yes, we need this because it's just been rooted from the main branch, and the changes haven't been merged to the main yet.
Also, we want the tests to pass. The failures are on the old commit ID and due to the tests added in the continuous run branch. It seems like force pushes don't re-run tests on the new commit ID (and manually re-running only does old commit ID), so can you add another commit? I think it'll be great if you can bump up the version to 0.4.2 in pyproject.toml and add on Changelog (miniscope-io/docs/meta/changelog.md) in this commit.

[coveralls]

coverage: 74.586% (-0.02%) from 74.603%
when pulling 9fafc94 on update_opalkelly_bitfiles
into 56b32b7 on main.

[sneakers-the-rat]

OK pushed a couple fixes here -

in the last failing run, the error message was super opaque and we hit a timeout. The problem is ultimately that the paths to the bitstream files weren't updated when the filenames changed, so the fpga_recv process errored out while the rest of the processes were just waiting for it.
https://github.com/Aharoni-Lab/miniscope-io/actions/runs/11621943298/job/32367337672

Added a few guardrails for that:

• one is just that we should fail much earlier than that. We shouldn't even try to start a capture process if the bitfile doesn't exist. I added a field validator to the config object so we throw the error before even trying to capture: 08a0da3
• then, if we somehow manage to lose the bitfile in between calling capture and actually reaching the capture method, i just let the following processes know by putting a None in the queues (which is the signal to end iteration): 08a0da3
• And then finally i resolved the error: 636c118

Do y'all remember why we have two sets of configs, one in the tests and one in the main package? Since we aren't really doing anything specific with the tests in the config and their format is still settling down, we should probably just test against all the configs in the package itself, right? Also I'm not sure what the status is on the example.yml config, seems like it's just going to drift out of date, and an incorrect example is worse than no example at all.

[sneakers-the-rat]

Also could we avoid putting .'s in filenames unless they're an extension? seems like we're already using - as a delimiter and _ as a whitespace character there, so for example rather than 8.33mhz could we have 8_33mhz? doesn't need to be changed in this PR but for future reference

[t-sasatani]

Oh yes, verifying the bitstream path in the model makes more sense. I'd appreciate it if either of you can do these updates because my current network situation is horrible atm and can't pull.

Do y'all remember why we have two sets of configs, one in the tests and one in the main package?

I would rather want to have both because the CI test data and the current hardware data format specifics can be different. We should update the test binary a bit more often, but it'll be painful to replace the test binary and hashes for each OS every time we make minor changes like a single buffer length or preamble. We might want some new devops stuff to update test data and hash,, though that could be future PR.

Also I'm not sure what the status is on the example.yml config, seems like it's just going to drift out of date, and an incorrect example is worse than no example at all.

There's no reason to leave this so let's delete this.

Also could we avoid putting .'s in filenames unless they're an extension? seems like we're already using - as a delimiter and _ as a whitespace character there, so for example rather than 8.33mhz could we have 8_33mhz? doesn't need to be changed in this PR but for future reference

Not as scary as spaces but agreed. I prefer doing it in this PR.

[sneakers-the-rat]

the CI test data and the current hardware data format specifics can be different.

I think the goal of the tests should be to mimic the actual behavior of the device as closely as possible, otherwise we aren't really testing whether the software works in the wild, but point taken on this being tricky, and think that we should have versioned schema for all our data formats - it's fine to update data format on the firmware, especially while still in beta testing, but we should have record and notice of that with predictable compatibility guarantees.

for the sake of this PR this is fine to leave as is, but we should probably revisit this later about what kind of consistency guarantees we want to make between hardware, firmware, and software so that updates don't break things :). I'll make an issue for this now.

There's no reason to leave this so let's delete this.
Not as scary as spaces but agreed. I prefer doing it in this PR.

i'll do these now. edit: done

[t-sasatani]

Thanks for the edits!

I think the goal of the tests should be to mimic the actual behavior of the device as closely as possible, otherwise we aren't really testing whether the software works in the wild.

There might be some confusion here. I mean, some parameters differ, but the structure is the same. So it's like testing add(a,b) function used as add(1,2) in a specific module with test functions add(1,1) and add(100,1), ..., etc., which often happens (though it's true it's better to test add(1,2) once stuff is a bit fixed). Adding more configs/data binaries into tests would make sense, though manually doing it will be a bit painful.

[t-sasatani]

The PRs are accumulating, so I will merge those passing the test and have review approval.