Add support for UCLA Miniscope V4 #115
Conversation
- This device needs some work arounds because its clock is completely out of spec for the SERDES it uses. The back channel must be used to configure a PLL on the camera in order to bump the PCLK rate before it will like. For this reason, I mad ConfigureFmcLinkController. CheckLinkState virtual so I could override its behavior with these steps before checking the link state - Added configuraiton and streaming functionality. - There are few outstanding issues: 1. Something is wrong with the DS90UB9x configuration resulting in 1 missing column from each frame and imaging data that "slides" by one column for each image produced 2. The axis map for the BNO needs to be set correctly 3. I was unable to verify that the EWL was functioning. It might be fine, but I was unable to verify.
|
@aacuevas Please have a look at the ConfigureUclaMiniscopeV4Camera.ConfigureMiniscope function to see if there is anything that would result in the "sliding" image (we seem to be missing 1 row of raw data per image) |
There was a problem hiding this comment.
The issue is not that it is skipping rows, but we are not telling the software which row is the first. Since startup is asynchronous and each ONI frame is one picture row, we need to mark the first row of every picture frame so if acquisition starts mid picture (or if there were some transmission error), the software can ignore the first rows until a new picture frame starts, so it can create the image proper.
| device.WriteRegister(DS90UB9x.TRIGGER, (uint)DS90UB9xTriggerMode.HsyncEdgePositive); | ||
| device.WriteRegister(DS90UB9x.SYNCBITS, 0); | ||
| device.WriteRegister(DS90UB9x.DATAGATE, (uint)DS90UB9xDataGate.VsyncPositive); | ||
| device.WriteRegister(DS90UB9x.MARK,0); |
There was a problem hiding this comment.
This should be DS90UB9xMarkMode.VsyncRising instead of 0
Since each ONI frame is one picture row, we need to know which row is the first on every picture frame. This marks the first row with a specific bit.
| var hubClockBuffer = new ulong[UclaMiniscopeV4.SensorRows]; | ||
| var clockBuffer = new ulong[UclaMiniscopeV4.SensorRows]; | ||
|
|
||
| var frameObserver = Observer.Create<oni.Frame>( |
There was a problem hiding this comment.
I am not sure where in this lib it should go, but need a filter to wait until the first row is received. On the old lib we did it with SkipWhile(f => (f.Sample[5] & 0x8000) == 0)
Additionally, this "first row" bit needs to be ignored, if it were to cause an issue later (Miniscope data is only 12 bits after all)
|
Thanks for the feedback. While I did see this in the old library, I was a bit confused why it was needed. My understanding is that I will try these suggestions regardless because I might be misunderstanding. |
|
Here is a GIF illustrating the issue (note that this does not use Pixel Mark). Adding pixel mark would appear to solve this issue by resetting the phase back to 0 for each image, but it looks like data is being lost and this is independent of that.
|
- Consistent naming with with ConfigureUclaMiniscopeV4Camera - Capture configuration properties outside of ConfigureDevice in ConfigureUclaMiniscopeV4Camera
OpenEphys.Onix/OpenEphys.Onix/ConfigureUclaMiniscopeV4Camera.cs
Outdated
Show resolved
Hide resolved
The gate is just that, a gate. It just tells the raw device "ignore anything that happens when vsync is not high", but vsync is high for the entire duration of a pixel frame. The ´SkipWhile´ statement in the observer is required so Bonsai skips anything until it knows it's going to have a full picture frame. |
|
FWIW, I used another miniscope PCB and the sliding phase shift in the image data disappeared. This indicates that although we are not guaranteed to start at the correct row, the data integrity is another issue entirely. Some scope PCBs seem to have significant data loss. Not sure if this has to do with miniscope design or the generic DS90UB9x receiver, or some combination of the two. In the meantime I have added the MarkMode so that images are aligned to row zero. The data loss issue is something else though. |
- Address code syntax and naming issues - Add Vsync rising mark to frame data so that we can align images to the first row in the face of asynchronous bonsai fraime collection - There is a larger issue about data integrity that seems to have more to do with the miniscope pcb design than issues with our hardware this library that is documented in #115
|
Can we compare the same faulty devices with the old lib? We had never noticed this specific behavior, so it could be that the old lib handled data losses better somehow, that there is still some software issue going on or that we were just lucky then and unlucky now. |
| readonly BehaviorSubject<double> ledBrightness = new(0); | ||
| readonly BehaviorSubject<UclaMiniscopeV4SensorGain> sensorGain = new(UclaMiniscopeV4SensorGain.Low); | ||
| readonly BehaviorSubject<double> liquidLensVoltage = new(47); // NB: middle of range |
There was a problem hiding this comment.
Should these be in the data node or the configure node? On one hand I can see the appeal of dynamic configuration while the device is streaming, on the other hand I would expect the configure node to be able to fully configure the sensor even if no downstream data node is immediately used. It also makes it more confusing that settings are distributed across two nodes.
Maybe this is one of the cases where we want to allow for the configure node to write to the device while the workflow is running, or alternatively we could have an editor that previews the camera for configuration.
Related to #114
There was a problem hiding this comment.
I think I agree. I put them here because I for some reason thought they should belong to the Data node. I think its better not to split configuration.
|
Aaron suggests testing the same hardware with the deprecated Bonsai.ONIX library to see if there is a different behavior. |
There was a problem hiding this comment.
@jonnew What was the motivation to rename UclaMiniscopeV4DataFrame to UclaMiniscopeV4Image? It wasn't mentioned in the commit message, but it seems confusing since it might give people the wrong impression that you can directly manipulate this type as if it was an image, which is not true.
In fact, the type has an Image property which is what effectively gives you the image. It also creates inconsistency with both the rest of the library, and with other camera packages, e.g. Spinnaker, Pylon, etc which also use the DataFrame terminology.
There was a problem hiding this comment.
The motivation is that cameras produce images. All data types contain data. "UclaMiniscopeDataFrame" could be BNO data since that is also a device on the scope that produces a DataFrame.
Inconsistency: this was already present. I tried to bring this up with e.g. AnalogInout versus AnalogInputData. Or with the location of the Behavior Subject properties. But in those cases, we went forward with inconsistency to preserve user clarity (maybe...?). I was doing the same here.
We could do UclaMiniscopeV4CameraData instead.
There was a problem hiding this comment.
The output of the AnalogInput node is still AnalogInputDataFrame. In that case the discussion was what to name the "data" node, not the "data frame" type.
The BNO data for the UCLA miniscope is just a Bno055DataFrame since that is what comes out of the BNO055 device.
You are right that the Miniscope headstage has multiple devices inside it, but we are naming DataFrame types after device names, not after headstage names, and I think there is a hard constraint in ONI that for a single device there can be at most a single data frame stream, is that right? So I don't think we really need to have any inconsistency here.
There was a problem hiding this comment.
My point was that in other circumstances, we didn't follow conventions in the case where clarity was at stake. I think UclaMiniscopeV4CameraData is a good compromise.
Remember that the Miniscope is not an ONI device. The ONI device is the link controller in passthrough mode. If we were going to follow this convention rigidly, then Miniscope would not even appear in the type.
In any case, UclaMiniscopeV4CameraData is a good compromise, so lets use that.
There was a problem hiding this comment.
Remember that the Miniscope is not an ONI device.
It is not an ONI device, but it is registered in the package DeviceManager as a virtual device. The only reason this is not a real ONI device is an implementation detail due to the constraints of the passthrough mode. Even conceptually from a user-centric perspective I feel it is probably better to call it a device.
I don't think we should use the technical details of passthrough mode devices as an argument to guide our naming conventions. If we did, then we would have to change the other passthrough DataFrame types like the NPX 1.0e, 2.0e, etc.
The reason we didn't use the Data suffix for these types to begin with is that we use Data as a suffix for the data node, as that is more readable in the workflow and we can't have name clashes, so the NeuropixelsV1eData node produces a sequence of NeuropixelsV1eDataFrame objects.
From looking further in the commit, I see the reason you didn't fall into this name clash is that you named the data node as UclaMiniscopeV4Camera instead of UclaMiniscopeV4CameraData as per the other devices.
The reason we use the Data suffix for data nodes is that we use the device name for the static class containing the device registers. In this case you called this class UclaMiniscopeV4 which is why you didn't get a name clash there either, but this class really feels like it should be called UclaMiniscopeV4Camera since the registers here should be the passthrough registers for the pass-through camera device.
Given the above, I don't think we should agree on this compromise, unless we change our naming convention for all the other devices in the package.
|
@aacuevas brought a very obvious test to do: Make sure the miniscopes I'm using function correctly in Bonsai.ONIX because its using the same mechanism to collect data. Then look for the deltas between passthrough settings and how data is processed in that library versus this one. |
|
This work was completed in #275 |
ConfigureFmcLinkController.CheckLinkStatevirtual so I could override its behavior with these steps before checking the link state