Add set fan mode and rbv by SourabhHalli · Pull Request #64 · areaDetector/ADAndor

SourabhHalli · 2026-05-28T10:18:18Z

Please note that the Fan mode RBV is not present in the SDK2 manual but just the set value is. So mirroring the readback value

jwlodek

I don't have a camera to test with, but left some notes. When you have setpoint/readback records, you only actually need one parameter internally, if you tie the two records together with the same string.

SourabhHalli · 2026-05-28T15:44:20Z

I was just in a confusion whether to mirror the rbv or not, thanks for the suggestion. I have Andor Ixon Ultra and its works fine for me.

henriquesimoes

Hi, Sourabh Halli!

Do you mind squashing your current commits into a single one and adding a commit message documenting the why this change should be included the way to propose?

This way, your proposed changes are easier to review by us and by future contributors who read the repository history retroactively.

I'm leaving a few additional comments below.

henriquesimoes · 2026-06-11T13:44:26Z

-        /*
-        Each track must be defined by a group of six integers.
-            - The top and bottom positions of the tracks.
-            - The left and right positions for the area of interest within each track
-            - The horizontal and vertical binning for each track. */
-        /*
-        Andor use 1-based exclusive co-ordinates.
-        e.g. from SDK manual:
-        1 2 1 1024 1 1
-        3 4 1 1024 1 1
-        5 6 1 1024 1 1
-        7 8 1 1024 1 1
-        9 10 1 1024 1 1 */
-        TrackDefn[TrackNo * 6 + 0] = mMultiTrack.TrackStart(TrackNo) + 1;
-        TrackDefn[TrackNo * 6 + 1] = mMultiTrack.TrackEnd(TrackNo) + 2; // CCDMultiTrack uses 0-based inlcusive co-ordinates.
-        TrackDefn[TrackNo * 6 + 2] = minX + 1;
-        TrackDefn[TrackNo * 6 + 3] = minX + sizeX;
-        TrackDefn[TrackNo * 6 + 4] = binX;
-        TrackDefn[TrackNo * 6 + 5] = mMultiTrack.TrackBin(TrackNo);
+        /*
+        Each track must be defined by a group of six integers.
+            - The top and bottom positions of the tracks.
+            - The left and right positions for the area of interest within each track
+            - The horizontal and vertical binning for each track. */
+        /*
+        Andor use 1-based exclusive co-ordinates.
+        e.g. from SDK manual:
+        1 2 1 1024 1 1
+        3 4 1 1024 1 1
+        5 6 1 1024 1 1
+        7 8 1 1024 1 1
+        9 10 1 1024 1 1 */
+        TrackDefn[TrackNo * 6 + 0] = mMultiTrack.TrackStart(TrackNo) + 1;
+        TrackDefn[TrackNo * 6 + 1] = mMultiTrack.TrackEnd(TrackNo) + 2; // CCDMultiTrack uses 0-based inlcusive co-ordinates.
+        TrackDefn[TrackNo * 6 + 2] = minX + 1;
+        TrackDefn[TrackNo * 6 + 3] = minX + sizeX;
+        TrackDefn[TrackNo * 6 + 4] = binX;
+        TrackDefn[TrackNo * 6 + 5] = mMultiTrack.TrackBin(TrackNo);
    }


Unrelated EOL change. Please, change from CRLF to LF in a standalone commit, so that we know it is just fixing code styling (and can easily double-check it is correctly doing so.)

henriquesimoes · 2026-06-11T13:45:05Z

-    if ((mCapabilities.ulSetFunctions & AC_SETFUNCTION_VSAMPLITUDE) != 0)
-    {
+    if ((mCapabilities.ulSetFunctions & AC_SETFUNCTION_VSAMPLITUDE) != 0)
+    {


Same thing here. It can be in the same commit fixing the EOL of the entire file.

henriquesimoes · 2026-06-11T14:15:05Z

+    field(TWVL, "2")
+    field(INP,  "@asyn($(PORT),$(ADDR),$(TIMEOUT))ANDOR_FAN_MODE")
+    field(SCAN, "I/O Intr")
+}


Missing final end-of-line at the end of the file so this becomes a valid POSIX file.

henriquesimoes · 2026-06-11T14:24:31Z

      - Off
    - ANDOR_FAN_MODE
-    - AndorFanMode, AndorFanMode_RBV
+    - AndorFanMode


We still have the AndorFanMode_RBV PV. We have only removed the ANDOR_FAN_MODE_RBV drvInfo.

henriquesimoes · 2026-06-11T14:31:49Z

      } catch (const std::string &e) {
        asynPrint(pasynUserSelf, ASYN_TRACE_ERROR,
          "%s:%s: %s\n",
          driverName, functionName, e.c_str());
        status = asynError;
      }


Shouldn't we rollback the value of AndorFanMode in case of errors?

That's what this following code asks us to do:

ADAndor/andorApp/src/andorCCD.cpp

Lines 658 to 660 in 49fd195

// Set in param lib so the user sees a readback straight away. Save a backup in case of errors.

getIntegerParam(function, &oldValue);

status = setIntegerParam(function, value);

Although this could be implemented in a generic way at the end of writeInt32, the value will flicker, and users will get an incorrect event that the value has changed to the desired value (when it never actually happened in the hardware). Thus, this seems to be the correct place to rollback.

Henrique, though I agree that the value should be rolled back, users should not be able to observe a flicker in the value, since it will only be propagated to the PV once callParamCallbacks is called.

So any logic implementing rollback before that call would be enough.

Sorry, I think I wasn't clear enough.

What I'm taking about a potential flickering is if we attempted to implement this once for all parameters at the end of writeInt32. I say it will have this flickering side-effect because we do have a callParamCallbacks() before we reach such place where the rollback would happen:

ADAndor/andorApp/src/andorCCD.cpp

Lines 772 to 797 in e3b7fc8

/* Do callbacks so higher layers see any changes */

callParamCallbacks();

/* Send a signal to the poller task which will make it do a poll, and switch to the fast poll rate */

epicsEventSignal(statusEvent);

if (mAcquiringData) {

asynPrint(this->pasynUserSelf, ASYN_TRACE_FLOW,

"%s:%s:, Sending dataEvent to dataTask ...\n",

driverName, functionName);

// Also signal the data readout thread

epicsEventSignal(dataEvent);

}

if (status) {

asynPrint(pasynUser, ASYN_TRACE_ERROR,

"%s:%s: error, status=%d function=%d, value=%d\n",

driverName, functionName, status, function, value);

} else {

asynPrint(pasynUser, ASYN_TRACEIO_DRIVER,

"%s:%s: function=%d, value=%d\n",

driverName, functionName, function, value);

// For a successful write, clear the error message.

setStringParam(AndorMessage, " ");

}

return status;

Of course, we could add another if (status) code block to implement such rollback before a callParamCallbacks, but that seems to lead to code to the spaghetti zone. Thus, all I'm arguing here is that rolling back inside function == AndorFanMode seems the best option for now, since (as you noted) it won't produce the flickering effect.

SourabhHalli added 4 commits April 13, 2026 13:37

adding fan controls

bc6913f

adding pvs to template

68b9717

fix for error setting param

2b4c1b6

renaming PVs

e353ec2

jwlodek reviewed May 28, 2026

View reviewed changes

Comment thread andorApp/Db/andorCCD.template Outdated

Comment thread andorApp/src/andorCCD.cpp Outdated

Comment thread andorApp/src/andorCCD.cpp Outdated

Comment thread andorApp/src/andorCCD.h

Comment thread docs/ADAndor/andorDoc.rst Outdated

removing fanmode rbv

962902a

SourabhHalli requested a review from jwlodek May 28, 2026 15:44

henriquesimoes suggested changes Jun 11, 2026

View reviewed changes

	// Set in param lib so the user sees a readback straight away. Save a backup in case of errors.
	getIntegerParam(function, &oldValue);
	status = setIntegerParam(function, value);

	/* Do callbacks so higher layers see any changes */
	callParamCallbacks();

	/* Send a signal to the poller task which will make it do a poll, and switch to the fast poll rate */
	epicsEventSignal(statusEvent);

	if (mAcquiringData) {
	asynPrint(this->pasynUserSelf, ASYN_TRACE_FLOW,
	"%s:%s:, Sending dataEvent to dataTask ...\n",
	driverName, functionName);
	// Also signal the data readout thread
	epicsEventSignal(dataEvent);
	}

	if (status) {
	asynPrint(pasynUser, ASYN_TRACE_ERROR,
	"%s:%s: error, status=%d function=%d, value=%d\n",
	driverName, functionName, status, function, value);
	} else {
	asynPrint(pasynUser, ASYN_TRACEIO_DRIVER,
	"%s:%s: function=%d, value=%d\n",
	driverName, functionName, function, value);
	// For a successful write, clear the error message.
	setStringParam(AndorMessage, " ");
	}
	return status;

Conversation

SourabhHalli commented May 28, 2026

Uh oh!

jwlodek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SourabhHalli commented May 28, 2026

Uh oh!

henriquesimoes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henriquesimoes Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

henriquesimoes Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

henriquesimoes Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

henriquesimoes Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

henriquesimoes Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

ericonr Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

henriquesimoes Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

henriquesimoes left a comment •

edited

Loading

henriquesimoes Jun 12, 2026 •

edited

Loading