add comments from the beamline

csaxs_bec/devices/epics/delay_generator_csaxs/ddg_1.py

@@ -127,14 +127,27 @@ class DDG1(PSIDeviceBase, DelayGeneratorCSAXS):
         Sets DEFAULT_IO_CONFIG into each channel, sets the trigger source to DEFAULT_TRIGGER_SOURCE,
         and turns off burst mode.
         """
-        self.burst_disable()  # it is possible to miss setting settings if burst is enabled
+        # NOTE First we make sure that there is nothing running on the DDG. This seems to 
+        # help to tackle that the DDG occasionally freezes during the first scan
+        # after reconnecting to it. Do not remove.
+        self.stop_ddg()
+
+        # NOTE Setting DEFAULT configurations for IO config, trigger config and references.
+        # The three dictionaries above 'DEFAULT_IO_CONFIG', 'DEFAULT_TRIGGER_SOURCE' and 
+        # 'DEFAULT_REFERNCES' should be used to adapt configurations if needed.
         for channel, config in DEFAULT_IO_CONFIG.items():
             self.set_io_values(channel, **config)
         self.set_trigger(DEFAULT_TRIGGER_SOURCE)
         self.set_references_for_channels(DEFAULT_REFERENCES)
-        # Set proc status to passively update with 5Hz (0.2s)
+
+        # NOTE Set state proc_status to be event based. This triggers readouts of the EventStatusLI bit
+        # based on events. This was empirically found to be a stable solution in combination with the poll
+        # loop of the state.
         self.state.proc_status_mode.put(PROC_EVENT_MODE.EVENT)
 
+        # NOTE Burst delay should be set to 0, don't remove as this will not be checked
+        self.burst_delay.put(0)
+
     def on_stage(self) -> None:
         """
         Stage logic for the DDG1 device, being th main trigger delay generator for CSAXS.
@@ -143,47 +156,87 @@ class DDG1(PSIDeviceBase, DelayGeneratorCSAXS):
 
         This DDG is always not in burst mode.
         """
+        # NOTE Only set relevant channels on burst_mode channel
+        # After mutliple tests with the HW, this procedure has been determined empirically 
+        # to improve stability and avoid HW getting stuck in triggering cycles
+        # Please also note that this should happen first, before setting delay times on the chabnnels.
+        if self.burst_mode.get() == 0:
+            self.burst_mode.put(1)
+
         exp_time = self.scan_info.msg.scan_parameters["exp_time"]
-        self.burst_enable(1, 0, exp_time)
-        exp_time = self.scan_info.msg.scan_parameters["exp_time"]
+        if self.burst_period.get() != exp_time:
+            self.burst_period.put(exp_time)
+
+        if self.burst_delay.get() != 0:
+            self.burst_delay.put(0)
+
+        #########################################
+        ### Setup delay pairs for acquisition ###
+        #########################################
+
         frames_per_trigger = self.scan_info.msg.scan_parameters["frames_per_trigger"]
+
         # Trigger DDG2
         # a = t0 + 2ms, b = a + 1us
         # a has reference to t0, b has reference to a
         self.set_delay_pairs(channel="ab", delay=2e-3, width=1e-6)
+        
         # Trigger shutter
-        shutter_width = 2e-3 + exp_time * frames_per_trigger + 1e-3
         # d = c/t0 + 2ms + exp_time * burst_count + 1ms
         # c has reference to t0, d has reference to c
+        shutter_width = 2e-3 + exp_time * frames_per_trigger + 1e-3
         self.set_delay_pairs(channel="cd", delay=0, width=shutter_width)
+
         # Trigger extra pulse for MCS OR gate
         # f = e + 1us
         # e has refernce to d, f has reference to e
         self.set_delay_pairs(channel="ef", delay=0, width=1e-6)
-        time.sleep(
-            0.2
-        )  # After staging, make sure that the DDG HW has some time to process changes properly.
+
+        # NOTE Add additional sleep to make sure that the IOC and DDG HW process the values properly
+        # This value has been choosen empirically after testing with the HW. Please acknowledge that
+        # this is called in parallel, so it should not add significant overhead to acquisition. It's 
+        # also just called once per scan. 
+        time.sleep(0.2)
 
     def _prepare_mcs_on_trigger(self, mcs: MCSCardCSAXS) -> None:
         """Prepare the MCS card for the next trigger.
         This method holds the logic to ensure that the MCS card is ready to read.
         It's logic is coupled to the MCS card implementation and the DDG1 trigger logic.
         """
-        mcs.stop_all.put(1)
-        mcs._omit_mca_callbacks.clear()  # Ensure that callbacks are not suppressed
+
+        # NOTE: It is crucial to first wait for the MCS card to finish it's acquisition before
+        # the DDG moves on to the next trigger cycle.
+        status = CompareStatus(mcs.acquiring, ACQUIRING.DONE)
+        self.cancel_on_stop(status)
+        status.wait(timeout=5)
+
+        # NOTE: Important logic on the MCS card, this makes sure that callbacks from the MCA channels
+        # are not surpressed. Please check MCS card and 'erase_all' comment.
+        mcs._omit_mca_callbacks.clear()  
         status_acquiring = TransitionStatus(mcs.acquiring, [ACQUIRING.DONE, ACQUIRING.ACQUIRING])
         self.cancel_on_stop(status_acquiring)
-        mcs.erase_start.put(1)  # Don't use erase_start as this may emit data through callbacks
-        time.sleep(0.2)  # Allow some time for the MCS to process the erase
-        status_acquiring.wait(timeout=10)  # Allow 10 seconds in case communication is slow
+        mcs.erase_start.put(1)  
+
+        # NOTE: Now we wait for the card to go to Acuiring after we've called erase_start
+        # Please increase the timeout if this turns out to be problematic
+        status_acquiring.wait(timeout=3)  
 
     def _poll_event_status(self) -> None:
         """
         Poll the event status register in a background thread. Control
         the polling with the _poll_thread_run_event and _poll_thread_kill_event.
         """
+        # NOTE hook to kill the loop, only needed if device is destroyed
         while not self._poll_thread_kill_event.is_set():
+
+            # The thread will wait in this event if IDLE. Polling can be started
+            # by setting 'poll_thread_run_event.set()'. Please check usage for software
+            # triggered scans from BEC within on_trigger.
             self._poll_thread_run_event.wait()
+
+            # NOTE Event to indicate that polling is taking place currently. This is needed as there
+            # are sleeps of 20ms in the poll loop which were empirically determined after long testing
+            # to improve stability in communication with the HW.
             self._poll_thread_poll_loop_done.clear()
             while (
                 self._poll_thread_run_event.is_set() and not self._poll_thread_kill_event.is_set()
@@ -195,29 +248,36 @@ class DDG1(PSIDeviceBase, DelayGeneratorCSAXS):
                     logger.error(
                         f"Exception in polling loop thread, polling continues...\n Error content:\n{content}"
                     )
-
+            # NOTE Important to set the event again. The next trigger loop waits for the poll thread to become
+            # IDLE again. Do not remove.
             self._poll_thread_poll_loop_done.set()
 
     def _poll_loop(self) -> None:
         """
         Poll loop to update event status.
+
         The checks ensure that the loop exist after each operation and be stuck in sleep.
         The 20ms sleep was added to ensure that the event status is not polled too frequently,
         and to give the device time to process the previous command. This was found empirically
         to be necessary to avoid missing events.
+
         IMPORTANT: Do not remove sleeps or try to optimize this logic. This seems to be a
         fragile balance between polling frequency and device processing time. Also in between
         start/stop of polling. Please also consider that there is a sleep in on_trigger and
         that this might also be necessary to avoid that HW becomes unavailable/unstable.
         """
         self.state.proc_status.put(1, use_complete=True)
-        time.sleep(0.02)  # 20ms delay for processing, important for not missing events
+        #NOTE: Important sleep that has been empirically determined after testing for a long time
+        # Only remove if absolutely certain that the DDG logic of polling the EventStatusLI works without it.
+        time.sleep(0.02) 
         if self._poll_thread_kill_event.is_set() or not self._poll_thread_run_event.is_set():
             return
         self.state.event_status.get(use_monitor=False)
         if self._poll_thread_kill_event.is_set() or not self._poll_thread_run_event.is_set():
             return
-        time.sleep(0.02)  # 20ms delay for processing, important for not missing events
+        #NOTE: Again important sleep that has been empirically determined after testing for a long time
+        # Only remove if certain that logic can be replaced to not risk HW failures.
+        time.sleep(0.02) 
 
     def _start_polling(self) -> None:
         """Start the polling loop in the background thread."""
@@ -260,15 +320,22 @@ class DDG1(PSIDeviceBase, DelayGeneratorCSAXS):
         return status
 
     def on_trigger(self) -> DeviceStatus:
-        """Note, we need to add a delay to the StatusBits callback on the event_status.
-        If we don't then subsequent triggers may reach the DDG too early, and will be ignored. To
-        avoid this, we've added the option to specify a delay via add_delay, default here is 50ms.
+        """
+        This method is called from BEC as a software trigger.
+        It first stops any active polling if still running. The sleep of 20ms is important
+        for proper functionality of the card. Then it checks if the 'mcs' card is in the config
+        and enabled, and prepares the card for triggering. For now this is still relevant, but may 
+        be moved to a high level logic in BEC in the future (neeeds).
+
+        Then a status_object is prepared that receives the EventStatusLI epics channel  (self.state.event_status),
+        and attaches a callback that resolves once the burst is done. The polling thread is enabled to manually
+        trigger a reading of the event status before a software trigger is sent via trigger_shot. 
         """
         # Stop polling, poll once manually to ensure that the register is clean
         self._stop_polling()
         self._poll_thread_poll_loop_done.wait(timeout=1)
-        # IMPORTANT: Keep this sleep setting, as it is necessary to avoid that the HW
-        # becomes unresponsive. This was found empirically and seems to be necessary
+        # NOTE: THis sleep is important for the HW to process the event and avoid that
+        # becomes unresponsive. This was found empirically after long testing.
         time.sleep(0.02)
 
         # Prepare the MCS card for the next software trigger

csaxs_bec/devices/epics/delay_generator_csaxs/delay_generator_csaxs.py

@@ -541,6 +541,7 @@ class DelayGeneratorCSAXS(Device):
         write_pv="BurstDelayAO",
         name="burst_delay",
         kind=Kind.omitted,
+        auto_monitor=True,
         doc="Delay before bursts start in seconds. Must be >=0.",
     )
     burst_period = Cpt(

csaxs_bec/devices/epics/mcs_card/mcs_card_csaxs.py

@@ -63,7 +63,7 @@ class MCSCardCSAXS(PSIDeviceBase, MCSCard):
     be realibly changed on the SIS3820 card's IOC through mux_output, so it is fixed here.
     Mux_output should therefore also be set to 32 in the IOC configuration.
     """
-
+    USER_ACCESS = ["mcs_recovery"]
     NUM_MCA_CHANNELS: int = 32
 
     # All counter from the MCS card.
@@ -120,10 +120,8 @@ class MCSCardCSAXS(PSIDeviceBase, MCSCard):
         """
         Called when the device is connected.
         """
-        # Make sure card is not running
         self.stop_all.put(1)
-        # Make sure the card is erased
-        self.erase_all.put(1)
+
 
         # Setup the MCS card settings
         self.channel_advance.set(CHANNELADVANCE.EXTERNAL).wait(timeout=self._pv_timeout)
@@ -147,6 +145,8 @@ class MCSCardCSAXS(PSIDeviceBase, MCSCard):
         # Subscribe the progress signal
         self.current_channel.subscribe(self._progress_update, run=False)
 
+        self.mcs_recovery()
+
         # Subscribe to the mca updates
         for sig in self.counters.component_names:
             sig_obj: EpicsSignalRO = getattr(self.counters, sig)
@@ -154,6 +154,7 @@ class MCSCardCSAXS(PSIDeviceBase, MCSCard):
 
         # Start monitoring thread
         self._scan_done_thread.start()
+        
 
     def _on_counter_update(self, value, **kwargs) -> None:
         """
@@ -317,3 +318,13 @@ class MCSCardCSAXS(PSIDeviceBase, MCSCard):
         """
         self.stop_all.put(1)
         self.erase_all.put(1)
+
+    def mcs_recovery(self, timeout:int=2) -> None:
+        """Recovery procedure for the mcs card"""
+        sleep_time = 1
+        logger.info(f"Running recovery procedure of {sleep_time}s sleep, calling stop_all and erase_all, and another {sleep_time}s sleep")
+        self.erase_start.put(1)
+        time.sleep(sleep_time)
+        self.stop_all.put(1)
+        self.erase_all.put(1)
+        time.sleep(1)