refactor(mcs-card): adjust mcs card to only have mca channels. #114
holler
perl_d
wyzula_j
menzel
diaz
wakonig_k
Reference in New Issue
Block a user
Delete Branch "refactor/mcs_card_refactoring"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
This PR revisited and partially rewrote the integration of the MCS card and the DigitalDelayGenerator (DDG) devices at the beamline. Hardware components were extensively tested to navigate and compensate any HW (or IOC) limitations of the individual devices, and integrated them for any software triggered scans running from BEC. The MCS card can now be operated in burst mode (frames_per_trigger >1) and we currently limit acquisitions to <5000Hz.
This limits is currently established on the DelayGenerator as the lowest deadtime, but can be adjusted if needed. The IOC of the MCS card was adjusted to allow 1e5 (or 1e6, to be checked) points. This limits the total number of points within a scan. The card is programmed to buffer all data before it is being read out. It should be checked if continuous readouts is going to be faster (and stable). As discussed, the MCS card now simply exposes the 32 input channels. We will discuss to create virtual devices who will use the channels to calculate new signals, e.g. a 'bpm' device. These virtual devices are not part of this PR.
Documentation
As both device integration are quite complex and are already heavily involved with beamline logic, I have spent additional time to write a documentation for the ophyd classes. This includes comments about HW (or IOC) limitations and obstacles that need to be considered as some operations have unexpected side effects that have to be considered in the integration.
In addition, due to the central timing source (master trigger), different devices share dependencies if certain procedures have to be executed in order. This creates additional complexity in the integration, in particular with the imposed limitations of the HW. All of this is documented as good as possible in README.md files in both modules, as well as directly in the source code of the device integrations.
To Do's
be5c0d870bto077359d63e077359d63etoe7f18db24ae046412eb8to47ff1c6699b9a9b5302ftob5369682deb5369682deto351e7a0e3dlooks good! - Only some minor comments.
@@ -0,0 +28,4 @@bec: BECClient # type: ignore[no-redef]gui: BECGuiClient # type: ignore[no-redef]as discussed offline, gui is not a builtin. It is better to access it through bec.gui
@@ -0,0 +49,4 @@name = re.sub(r"[^a-zA-Z0-9_]", "", name)if not name:return "_"I guess if there is no valid name, it should raise, no?
@@ -0,0 +60,4 @@class DebugTools:"""A collection of debugging tools for the BEC IPython client at cSAXS."""_PURPOSE = " ".join(I don't understand why this is a joined list of strings. Why not just a single string?
@@ -0,0 +113,4 @@if "mcs" not in dev:raise RuntimeError("MCS device is not loaded in the current active BEC session.")def _check_if_ddg_is_loaded(self):probably worth merging these methods into a single one. Something along the lines of
Also, I would suggest to print the exact name in the error message of the device, not capitalized. Otherwise one might be tempted to add a device "MCS" rather than "mcs"
@@ -0,0 +137,4 @@self._check_if_ddg_is_loaded()if mode == "high_frame":burst_frames = np.random.randint(10000, 100000) # between 10000 and 100000just a minor hint: you can set separators for large numbers, e.g. 10_000 instead of 10000. Could make it easier to read. See https://peps.python.org/pep-0515/
@@ -0,0 +246,4 @@return Falsedef open_it_service_page(self):"""Open the overview of IT services hosted by Science IT (Leo Sala) for cSAXS."""let's remove the personal name
@@ -210,3 +278,1 @@if mapped_signal_name == "count_time":value = value / self._mcs_clockvalue = float(value.mean())value = value.tolist() # Convert numpy array to listI guess it can only be numpy, single value or list? Maybe put it as type hint to the signature
351e7a0e3dto11c887b078