epics-modules / motor

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

Add MINP for soft motors #211

Open tboegi opened 11 months ago

tboegi commented 11 months ago

Make it possible to set bits of the MSTA field for soft motors. Add a new input link, called MINP, beside the existing DINP and RINP

MINP will read the new MSTA bits, but filter out the RA_DONE bit. The RA_DONE bit will be taken from DINP (as before) and the RA_DONE bit inside MINP will be ignored, preserving the RA_DONE coming from DINP

Note that the MINP needs to follow the bit-definition in MSTA. It may update RA_PLUS_LS, RA_MINUS_LS. RA_HOMED or other bits may be set as well, whatever the application needs. MINP may be pointed to the output of a calc or transform record.

kmpeters commented 11 months ago

@tboegi, I switched the master branch to use the sequencer mirror. If you rebase this pull request on the master branch, the CI builds should succeed.

tboegi commented 11 months ago

2023-12-11T07:45:00.0015590Z File "/Users/runner/work/motor/motor/.ci/cue.py", line 14, in 2023-12-11T07:45:00.0016620Z import distutils.util 2023-12-11T07:45:00.0018040Z ModuleNotFoundError: No module named 'distutils' 2023-12-11T07:45:00.0148950Z ##[error]Process completed with exit code 1.

kmpeters commented 11 months ago

I'm very confused. Why did the build of the master branch succeed on OS X:

https://github.com/epics-modules/motor/actions/runs/7144493328

While the build of this branch failed:

https://github.com/epics-modules/motor/actions/runs/7164362343

With the same motor/.ci/cue.py?

kmpeters commented 11 months ago

I might just ignore the CI failure for now. I've seen weird errors on pull requests that weren't problems on the master branch.

tboegi commented 11 months ago

@kmpeters I am confused as well. More digging needed. The code was compiled (and tested) locally running on a Intel-Mac.

kmpeters commented 11 months ago

Pedro Nariyoshi reports that these changes work well in his test setup:

https://epics.anl.gov/tech-talk/2023/msg01901.php

kmpeters commented 11 months ago

I'm testing this pull request with using the following databases in the motorMotorSim example IOC, which implements the soft motor example from the motor documentation and links an mbboDirect record to the MINP field of the soft motor:

SoftMotorExample.db.txt SoftMSTA.db.txt

I'm using the first simulated motor as the "real" motor:

dbLoadRecords("$(TOP)/motorSimApp/Db/SoftMotorExample.db", "P=$(PREFIX),M=m1,SM=sm1")
dbLoadRecords("$(TOP)/motorSimApp/Db/SoftMSTA.db", "P=$(PREFIX),SM=sm1")

I'm able to set the high hardware limit by doing this: caput IOC:sm1:msta.B2 1 I'm able to set the low hardware limit by doing this: caput IOC:sm1:msta.BD 1

I observe the following:

  1. Setting hardware limits via the MINP field causes the soft motor to think the motor is done moving shortly after the move is initiated (the target position gets synced with the RBV field), however the real motor is never stopped.
  2. The soft motor's MSTA field isn't set correctly when the homed bit is set, caput IOC:sm1:msta.BF 1. The IOC:sm1:msta.VAL is 32768, but the soft motor's MSTA field has a value of 0xffff8002.
tboegi commented 11 months ago

About the STOP: I need to check, if we want to have this functionality ?

@kmpeters The homed bit is bit 14, not 15.

caput IOC:sm1:msta.BE 1

14 unsigned int RA_HOMED :1; / Axis has been homed./ 13 unsigned int RA_MINUS_LS :1; / minus limit switch has been hit / 12 unsigned int CNTRL_COMM_ERR :1; / Controller communication error. / 11 unsigned int GAIN_SUPPORT :1; / Motor supports closed-loop position control. / 10 unsigned int RA_MOVING :1; / non-zero velocity present / 9 unsigned int RA_PROBLEM :1; / driver stopped polling / 8 unsigned int EA_PRESENT :1; / encoder is present / 7 unsigned int EA_HOME :1; / encoder home signal on / 6 unsigned int EA_SLIP_STALL :1; / slip/stall detected / 5 unsigned int EA_POSITION :1; / position maintenence enabled / 4 unsigned int EA_SLIP :1; / encoder slip enabled / 3 unsigned int RA_HOME :1; / The home signal is on / 2 unsigned int RA_PLUS_LS :1; / plus limit switch has been hit / 1 unsigned int RA_DONE :1; / a motion is complete / 0 unsigned int RA_DIRECTION :1; / (last) 0=Negative, 1=Positive /

tboegi commented 11 months ago

@kmpeters Thanks for testing

I just realize that the .MSTA field is declared as field(MSTA,DBF_ULONG) { While we see this: void soft_minp_func(struct motorRecord *mr, short newminp)

So I think that the PR needs improvements !

kmpeters commented 11 months ago

About the STOP: I need to check, if we want to have this functionality ?

I don't understand the use-case for the MINP field, if we don't want the actual motors to stop when a soft-motor's hardware limit is activated.

tboegi commented 11 months ago

The original motivation comes from a soft-slit database: 2 physical (hard) motors (the pos- and neg- blade) controlled by 4 logical motors: gap, center, pos, neg. The MINP field would allow to a) forward limit switches: from pos+HLS -> gapHLS, centerHLS using a calc or transform record. b) handle more bits (homed, power on) from hard- to soft-motors.

The STOP function is another nice feature. I will look at it the next days.

mp49 commented 11 months ago

If we add a STOP feature it should definitely be optional. Presumably the underlying motor controller or PLC will handle stopping on a limit switch. Additional software STOPs can cause issues with motion programs implemented under the feet of the motor record.

tboegi commented 11 months ago

Yes, the more I think about it, the more I am tempted to say that a STOP function could and should be implemented in external records, like calc or transform. The MINP should just forward bits. And those can come from other records, or directly from a hard-motor.

kmpeters commented 11 months ago

I'll do some testing with an external stop calcout record.

kmpeters commented 11 months ago

I'll do some testing with an external stop calcout record.

This updated SoftMSTA.db works reasonably well with simulated motors:

record(motor, "$(P)$(SM)")
{
  field(MINP, "$(P)$(SM):msta NPP NMS")
}

record(mbboDirect, "$(P)$(SM):msta")
{
  field(DESC, "User-specified motor status")
  field(DTYP, "Soft Channel")
  field(OMSL, "supervisory")
  #field(OUT,  "TBD")
  field(SHFT, "0")
}

record(calcout, "$(P)$(SM):stopCalc")
{
  field(DESC, "HW limit - stop motor")
  field(INPA, "$(P)$(SM).MSTA CP NMS")
  field(CALC, "(A&(1<<2))||(A&(1<<13))")
  field(OOPT, "When Non-zero")
  field(DOPT, "Use OCAL")
  field(OCAL, "1")
  field(OUT,  "$(P)$(M).STOP PP NMS")
}

Every move that is made while the hardware limit of the soft motor is active is stopped, but the motor is able to move small amounts that would, in theory, allow moving off of the limit switch.

tboegi commented 11 months ago

@kmpeters Do I understand that right: The motorRecord.cc allows to move forward, when the HLS is active ? That feels like a bug to me.

kmpeters commented 11 months ago

@kmpeters Do I understand that right: The motorRecord.cc allows to move forward, when the HLS is active ? That feels like a bug to me.

@tboegi, I believe you understand it correctly. The motor record checks the hardware limit states in the commanded direction of motion for retries, jogging and homing, but not normal moves. This is not likely a problem for real motor controllers, nor is it a problem for simulated motors, because the controllers know there is a limit violation and the move command that is sent should result in an error. The soft-channel device support, however, does not know there is a limit violation and it unconditionally tells the linked motor to move:

https://github.com/epics-modules/motor/blob/f8a3239f0678f8f3d75fd5cf2f9c717dc3db9426/motorApp/SoftMotorSrc/devSoft.cc#L163-L166

I think adding a check for a hardware limit violation and returning an error instead of moving the linked motor in devSoft.cc is a better solution than adding checks for the hardware limit states before moves in motorRecord.cc. I would expect hardware limit enforcement in the motor record to cause unable-to-move-off-of-limit problems, because of the delay polling the motor's status.

kmpeters commented 11 months ago

Implementing hardware-limit enforcement in the soft-channel device support will likely require the direction bit to be set properly, which is also not implemented.

tboegi commented 11 months ago

@kmpeters Kevin, we are slightly getting off track. I created https://github.com/epics-modules/motor/issues/212

I will continue to work on both of these. Stay tuned.