Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MINP for soft motors #211

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tboegi
Copy link
Contributor

@tboegi tboegi commented Dec 8, 2023

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 kmpeters added this to the R7-4 milestone Dec 8, 2023
@kmpeters
Copy link
Member

kmpeters commented Dec 8, 2023

@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 tboegi force-pushed the torsten/231206-add-MINP-for-softmotor branch from a179d57 to b994981 Compare December 8, 2023 21:09
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.
@tboegi tboegi force-pushed the torsten/231206-add-MINP-for-softmotor branch from b994981 to 2dc3101 Compare December 11, 2023 07:44
@tboegi
Copy link
Contributor Author

tboegi commented Dec 11, 2023

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

tboegi commented Dec 11, 2023

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

@kmpeters
Copy link
Member

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

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

@kmpeters
Copy link
Member

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
Copy link
Contributor Author

tboegi commented Dec 15, 2023

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
Copy link
Contributor Author

tboegi commented Dec 15, 2023

@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 !

@tboegi tboegi marked this pull request as draft December 17, 2023 09:56
@kmpeters
Copy link
Member

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
Copy link
Contributor Author

tboegi commented Dec 18, 2023

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
Copy link
Contributor

mp49 commented Dec 18, 2023

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
Copy link
Contributor Author

tboegi commented Dec 18, 2023

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
Copy link
Member

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

@kmpeters
Copy link
Member

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
Copy link
Contributor Author

tboegi commented Dec 19, 2023

@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
Copy link
Member

kmpeters commented Dec 19, 2023

@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:

case MOVE_ABS:
case MOVE_REL:
status = dbPutLink(&mr->out, DBR_DOUBLE, &mr->dval, 1);
break;

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
Copy link
Member

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
Copy link
Contributor Author

tboegi commented Dec 20, 2023

@kmpeters
Kevin, we are slightly getting off track.
I created
#212

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

When implementing the MINP (MSTA input) it was overseen,
that MSTA is defined as 32 bit (where only 15 bits are used).
Correct this: use long instead of short.
Add the MINP field: It is "similar" to e.g. DINP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants