epics-modules / motor

APS BCDA synApps module: motor
https://epics-modules.github.io/motor/
20 stars 48 forks source link

fix for profileTimes writing to asynMotorController #188

Closed Sidney28 closed 1 year ago

Sidney28 commented 2 years ago

profileTimes variable is a part of the asynMotorController class, not asynMotorAxis class. So, in previous version of the asynMotorController::writeFloat64Array function, getAxis will return 0 and profileTimes will not be written.

In this version if (function == profileTimeArray_)... is executed first to fix the problem.

MarkRivers commented 2 years ago

profileTimes variable is a part of the asynMotorController class, not asynMotorAxis class. So, in previous version of the asynMotorController::writeFloat64Array function, getAxis will return 0 and profileTimes will not be written.

I don't understand that comment. I just tested writing profileTimes from a Channel Access client and it works fine without this PR.

This is a plot of Prof1:Times when the IOC boots: image

I then create an array of times and write it using the IDL CA client:

IDL> times = findgen(250)/250.
IDL> t = caput('DMC01:Prof1:Times', times)

This is the plot after doing that: image

I don't understand why you say that getAxis will return 0. It should return a pointer to axis 0, because asyn addr=0 is used for both the controller itself and for the first axis.

Do you have an example where it fails, since I clearly have an example where it does not fail.

MarkRivers commented 2 years ago

I realized my test above does not prove that asynMotorController::writeFloat64Array is not getting 0 for getAxis. It just proves that the the waveform record was written to OK.

I then made this change to add debugging to asynMotorController::writeFloat64Array:

corvette:motor/motorApp/MotorSrc>git diff .
diff --git a/motorApp/MotorSrc/asynMotorController.cpp b/motorApp/MotorSrc/asynMotorController.cpp
index e5d069d..fde4e8e 100644
--- a/motorApp/MotorSrc/asynMotorController.cpp
+++ b/motorApp/MotorSrc/asynMotorController.cpp
@@ -440,6 +440,7 @@ asynStatus asynMotorController::writeFloat64Array(asynUser *pasynUser, epicsFloa
   static const char *functionName = "writeFloat64Array";

   pAxis = getAxis(pasynUser);
+printf("%s::%s pAxis=%p\n", driverName, functionName, pAxis);
   if (!pAxis) return asynError;

   if (nElements > maxProfilePoints_) nElements = maxProfilePoints_;

Now when I write to Prof1:Times from IDL I see this message on the IOC:

asynMotorController::writeFloat64Array pAxis=0x2466fb0

That proves that getAxis is not returning 0, it is returning a pointer to the first axis.

So I don't understand what error was motivating this PR.

Sidney28 commented 1 year ago

Thank you for the answer and sorry for my late response.

I faced with this problem, while writing a driver for a custom controller. It has a bus where axes boards have its internal addresses 128 and 129. And I have to add an address of a board in packages, that I send to it. So I used this addresses, when I created an Axes object. I thought that it is a proper way to use asyn address.

As I do not have axis with address 0 (only 128 and 129), a call of getAxis(pasynUser) for putting profile time array returns nullptr. I assume that in your test there was an axis with address 0. That is why it works fine.

In read/write methods in my controller class I firstly check if pasynUser->reason is related to the controller (and if so handle it), then call getAxis(pasynUser) and handle axis related reasons, and if it is not reason of my driver implementation, I call the base class implementation. That is why other parts of driver works fine for me.

I thought that it is a proper way to use asyn address. And I thought if some one will have controller, that has no axis with address 0, calling writeFloat64Array for putting profile time array will fail.

So, if I an not right, I think I should rewrite my driver implementation to fix my problem without changes in asynMotorController.

But if you think that it is possible to have no axis with address 0 (as in my case), then this PR may be useful.

kmpeters commented 1 year ago

I think the correct approach is to rewrite the driver implementation to fix the problem without changes to asynMotorController.