New feature: SI Tune Feedback

[VitorSouzaLNLS]

SI Tune Feedback – Initial Support

Summary

This PR adds the initial support for the SI Tune Feedback application in siriuspy/opticscorr.

The implementation introduces the infrastructure required to operate a closed-loop tune correction system for the Storage Ring, including new PVs, loop control, tune source selection, PID configuration, beam-status monitoring, and automatic correction logic.

What changed

• Added new enums and constants for:

  • feedback loop state (Open / Closed);
  • tune source selection;
  • default loop frequency;
  • default PID gains;
  • default reference tunes;
  • maximum allowed tune error.

• Expanded the tune feedback PV database with:

  • loop state and loop frequency PVs;
  • measured tune PVs;
  • reference tune PVs;
  • maximum tune error PVs;
  • PID tuning PVs;
  • tune source selection PVs;
  • tune source PV-list support.

• Extended TuneCorrApp to support feedback-loop operation:

  • manual tune and correction updates are blocked while the loop is closed;
  • log messages are suppressed during automatic feedback operation to avoid excessive logging.

• Added SITuneCorrApp, which implements:

  • loop open/close handling;
  • tune source switching;
  • reference tune configuration;
  • PID gain configuration;
  • stored-beam monitoring;
  • background automatic correction thread;
  • tune validation against the configured tolerance;
  • automatic loop opening when stored beam is lost.

Notes

• The tune source can now be selected between TuneSpec and BbB-based tune measurements.
• When the loop is closed, manual updates to tune or correction parameters are rejected to avoid conflicts with the feedback thread.
• The feedback loop automatically opens when stored beam is lost.
• The current implementation is intended as the initial version of the SI Tune Feedback application and may still require operational refinements based on machine studies and user-shift experience.

Operational findings

During early use of this version, two operational issues were identified:

• During a beam dump in a user shift, the StoredEBeam-Mon PV used to open the loop when there is no stored beam was found to have an incorrect calibration.
• In another event, it was observed that under anomalous conditions where the loop is opened, the loop state may become inconsistent. This is a known bug and still needs to be addressed.

Testing

• Verified PV creation and default values.
• Checked loop state transitions.
• Checked tune source switching and PV-list updates.
• Confirmed that manual updates are blocked while feedback is active.
• Verified that the loop opens when stored beam is lost, although the reliability of this mechanism depends on the correct calibration of StoredEBeam-Mon.

To do

🗹 Fix the known loop-state inconsistency bug.
🗹 Review and correct the StoredEBeam-Mon calibration dependency.
🗹 Merge these fixes as part of this PR before final integration.

[VitorSouzaLNLS]

The latest bug we spotted is still to be fixed (one commit away). Please check the _do_auto_corr function: when exiting the while loop, self._inloop is not updated while resetting the loop state, only self._loop_state is changed correctly. This led to the incorrect behavior we observed on Monday and Tuesday.

Note: the property self._inloop will be deleted. Only self._loop_state is necessary.

[xresende]

@VitorSouzaLNLS , integration test is failing, did you notice it ?

[VitorSouzaLNLS]

@VitorSouzaLNLS , integration test is failing, did you notice it ?

Yes, I did. The first failure occurs when the cmd_apply_corr function is called. The test tries to assert False when the "apply command" completes successfully, which was the expected return behavior before this PR.

However, I modified cmd_apply_corr and cmd_set_newref to return True when the command executes successfully. From my point of view, these functions should return True when everything goes well. What do you think?

Some command functions, such as cmd_config_ti, cmd_config_ps, and cmd_meas_config_save, exhibit the same behavior of returning False on success. However, cmd_meas_config returns True when the command completes successfully.

Perhaps we should standardize this return-value behavior, either by always returning True on success or always returning False.

@fernandohds564 @anacso17 what do you think about this? I can create a new PR to discuss and handle this differences. (In the meantime, I'll revert the modification I made in cmd_apply_corr and cmd_set_newref).

[xresende]

@VitorSouzaLNLS , integration test is failing, did you notice it ?

Yes, I did. The first failure occurs when the cmd_apply_corr function is called. The test tries to assert False when the "apply command" completes successfully, which was the expected return behavior before this PR.

However, I modified cmd_apply_corr and cmd_set_newref to return True when the command executes successfully. From my point of view, these functions should return True when everything goes well. What do you think?

Some command functions, such as cmd_config_ti, cmd_config_ps, and cmd_meas_config_save, exhibit the same behavior of returning False on success. However, cmd_meas_config returns True when the command completes successfully.

Perhaps we should standardize this return-value behavior, either by always returning True on success or always returning False.

@fernandohds564 @anacso17 what do you think about this? I can create a new PR to discuss and handle this differences. (In the meantime, I'll revert the modification I made in cmd_apply_corr and cmd_set_newref).

I think we could use the old return stardard for this PR, just like it is being checked in the unittest, but we should discuss broadly if it makes sense to use a unique standard for all our pcaspy-based IOCs. If other people think this discussion is usefull, I would open a new PR so that it does not get forgotton in our daily duties.

[fernandohds564]

I don't remember why we defined this standard. Do you remember, @anacso17? I think it worth discussing this separately, depending on Ana's answer.

[VitorSouzaLNLS]

After discussing with @fernandohds564, we identified that returning False here is a workaround to handle the counter of -Cmd PVs. Currently, the App (in dev-packages) writes the counter internally and returns False so that the Driver (in the machine-applications) skips writing/overwriting it.

[fernandohds564]

After discussing with @fernandohds564, we identified that returning False here is a workaround to handle the counter of -Cmd PVs. Currently, the App (in dev-packages) writes the counter internally and returns False so that the Driver (in the machine-applications) skips writing/overwriting it.

We believe this is not the ideal behavior and we will adapt the code to do something similar to what SOFB and the HLTiming IOCs do, which is handling the counter at the Driver in the machine applications code. This changes will be made in in another PR. Do you agree with these changes, @anacso17?

[anacso17]

Hi guys! I didn't have time to review this PR yet, but regarding the topic of -Cmd PVs, I agree we can standardize with the SOFB softIOC bahavior.

[xresende]

@VitorSouzaLNLS , is there not a default connecion_timeout constant from a base class or one in another class you can import and expose more visibly than having a hard-coded constant burried in the code implementation ?