Better open loop handling; add a simulator - #34
Open
tboegi wants to merge 73 commits into
Open
Conversation
Update the .ci submodule to the latest release
Remove Ubuntu 18.04, since this is no longer available: https://github.blog/changelog/2022-08-09-github-actions-the-ubuntu-18-04-actions-runner-image-is-being-deprecated-and-will-be-removed-by-12-1-22/
Make the source code compile without warnings. - Initialize stepTarget_ inside the constructor, not the .h files - Comment out not used "static const char *functionName" - Comment out a not used "asynStatus comStatus_" - Remove not needed helper variables in C++ constructors No intended code changes.
Remove all conversion warnings, found by the Windows compiler. When e.g. a double is assigned to an int, cast it.
Clean up the whithe space: No more TAB or trailing whithespace
Make sure that further whitespace damage is detected in ci:
Changes to be committed:
modified: .github/workflows/ci-scripts-build.yml
new file: checkws.sh
The "positioner name" was garbage: strcpy(dst, src) it is. Fix this, and while there, add a memset() and use strncpy() instead. Add more variables to be retrieved and printed, range limit and thresholds. To avoid too much code-duplication, add and use 2 helper functions to simplify the code: reportHelperInteger() and reportHelperDouble()
Improve the poll() function with 3 changes:
The setting of motorStatusProblem_ was wrong:
// This is correct and not changed:
In MCS2Controller::clearErrors()
skip:
setIntegerParam(this->motorStatusProblem_, comStatus ? 1:0);
In MCS2Axis::poll()
// This is changed, see below.
setIntegerParam(pC_->motorStatusProblem_, movementFailed);
// This is wrong: error is cleared, regardless.
setIntegerParam(pC_->motorStatusProblem_, 0);
// And, at the end of poll(), the Problem_ is set to 0,
// unless there was a communication error.
skip:
setIntegerParam(this->motorStatusProblem_, comStatus ? 1:0);
Clean up the code, and use a single call to set motorStatusProblem_:
skip:
setIntegerParam(pC_->motorStatusProblem_, comStatus ? 1:0);
Change even the the handling of movementFailed:
It is not a problem (of the next motion), but rather an indication
that the current/last commanded movement failed.
Use the motorStatusFollowingError_ to indicate that the last movement failed:
setIntegerParam(pC_->motorStatusFollowingError_,
followLimitReached || movementFailed);
Rename Channel State Bits:
Rename the definition of the "channel state bits" to make more clear,
where they belong to.
Match MCS2ProgrammersGuide.pdf, page 71.
Example: ACTIVELY_MOVING -> CH_STATE_ACTIVELY_MOVING
Make the a "#define", which is a better choice to match an integer
anyway.
Add new definition bits, CH_STATE_IS_PHASED..CH_STATE_BRAKE_ENABLED
No intended code changes.
Do not set the problem bit, but rather the motorStatusCommsError_ bit, when the communication fails in poll(). This will set the appropriate alarm on the motorRecord: COMM_INVALID
Add a printout to hold_time in report().
While there, remove code in report() that is not working:
REPORTHELPERCHECKERROR(":TUN:THR:THO?", buf.target_hold_threshold);
Make the HOLD paramater configurable: Add it as an asyn parameter and add a longout record for it. The default is still -1.
Add a new method, that will be caused when polling starts, either when the IOC starts, or when the connection is lost and now re-established. Rigth now, the HOLD parameter is send to the controller. More functions may be added later.
Detect network problems better when there is a query to the controller and an answer is expected, but none is received. It may happen, the the answer is stuck inside the network, to be more exact inside the TCP layer, when there is a temporary network problem. To avoid that this answer is delivered later and treated as the response to the next poll() and everyrhing is mixed up, disconnect the socket (TCP connection). asyn will the re-establish a new one without having old data floating around.
Partly revert the last commit: The TCP/IP stack in the controller is only allowing one TCP connection at a time. Trying to re-establish a new connection while the old one is still not "fully closed" does not work. This means that the "disconnect" from the previous commit may hang the whold communication, if there is a short glitch in the network. And the controller needs a power-cycle to get out of this situation. The new solution will not close the TCP connection, but wait for the controller to do so. However, when polling one axis fails, all axes are put into the disconnected state. Where the initialPoll() will do the re-establishment.
Add logic to make it possible to supply a user with an
"error message text". From the possible errors and problems,
try to find the most important one and display that one.
Note1: There is no record yet.
Note2: The ordering of priorities may need to change, once we
get more experience in what the controller reports when.
Support the "message from driver" record found in the ESS-motor: There is an overall "message text" record, which either displays the message from the driver (this module), if there is something important, or the messages from the generic driver.
Add the readback in picometers: Store it in a new asynParameter A record will be added to this later
When we have pvxs defined in our worktree, enable it. Otherwise, when compiling/linking against EPICS base 7, enable pva.
Split the READBACK asynparamter into 2: - FREADBACK floating point, double, used for an "ai" record - IREADBACK integer 64 bit, used for an "int64" record Add needed code and #ifdefs to be able to compile against older versions of asyn, which do not have the int64 support.
Improve the printing situation when a timeout occurs: Normally the printouts are are on ASYN_TRACEIO_DRIVER level and switched off. When a timeout occurs, this is serious. Print it on ERROR level.
Add support for a probably unusual configuration: The positioner has a sensor, but it should not be used. The aim is to use an external readback to get the position into motorRecord and use openloop movements in the MCS2. Retries are handled by the motorRecord: The fields RDBL, RTRY RMOD and URIP are used. In the first phase of the project we can compare the external readback with the sensor and see how good that works. In the seconds phase the function of the sensor will be lost, it is going to be damaged by radiation and stops working. Add a new asyn parameter to be able to "connect" a record to it, and use a local variable as a copy inside MCS2Axis::move(). This commit does not use the CNEN field from the motorRecord for 2 reasons: The default for the driver is still closed loop. The special case open loop needs to be configured, when needed. The CNEN field and specially the setClosedLoop() method in the generic model 3 driver has been used to power on- and off the motor. Espially the auto-power-on (and off) feature relies on the fact that setClosedLoop() inside the driver enables the power and nothing else. The dilemma of CNEN used for different things has been realized by the community, and my take away from this is to keep CNEN away from closed loop.
Add more values in asynReport: step_freq, step_ampl, diag_clf_max, diag_clf_aver
The current implementation uses velocity (maxVelocity) in move() as the directly as the frequency. This has 2 disadvantages: - The velocity from motorRecord (.VELO) is now longer reflected in EGU/second. - Switching between closed- and open-loop gives a mess: A velocity of 1 mm/sec needs e.g a frequency of 300 Hz, as observed at one positioner. To somewhat improve the situation, add an optional factor to calculate the frequency from the velocity. This can be connected to an ao record. When the new factor is 0.0, it is ignored, making the code backward compatible with existing databases.
Add 2 parameters (no records yet) that allow the driver to issue a "step move" in open loop. It turns out that all open loop step moves are using steps, not picometers. A better handling of open loop movements is wanted: On the used hardware we can find that running +1000 moves the positioner 2.4 mm, and stepping backwards with -1000 moves -3.4 mm. That will be addressed in the next commits.
Running the piezo in open-loop means that we use "(full) steps". A step has a specific size, and the positioner even moves a slightly different distance forward compared to reverse. The the achieved step size may even be dependend on other things, I don't know yet. Anyway, make it possible to configure the step size into the driver, so that the move() command achieves a good movement. Example for records will be added later.
Improve the calculation of the frequency when running open loop and there is are step sizes defined. Re-rorder the code to limit the scope of temporary variables. Remove the parameter vel2clf because that is no longer needed. Fix a bug: stepcnt is an integer to move the positioner a number of steps, so create it as asynParamInt32, as it should be.
The open loop logic of move() has 4 different use cases: 1) move absolute, closed loop 2) move relative, closed loop 3) move absolute, open loop 4) move relative, open loop 3) was not working as it should when stepsizef and stepsizer had been used. Instead of converting the position into steps, it was forwared "as is"
When retry-modus 3 was used (motorRecord.RMOD =3, in position), the open loop move() commands are absolute, not relative. (If this modus makes sense, is a different question). However, in order to keep track of the commanded position, update stepTargetPos_nm_ both for relative and absolute moves.
The variable channel_ is not needed: We can use axisNo_ from asynAxisMotor, which does the same thing.
Adding a folder smaractExApp without adding a Makefile makes `make` fail. Solution: Specify "smarActApp" instead of "*App" as a wildcard.
Add support for "SensorPowerMode" and "SensorDelay", ":CHAN%d:SENS:MOD" and ":CHAN%d:SENS:DEL". The power mode has 3 values: 0 == disabled, 1 == enabled, 2 == power save. When disabled, the controller will report error 260. Since we read the sensor power mode and react on it by setting sensorIsDisabled_ to 1 we can ignore this error. It may happen that the device is configured by a vendor tool outside EPICS. To be able to follow what is inside tha controller: Add the reading of sensor power mode and sensor delay in poll(); Improve report(): - decode STAT in report() - print sensorIsDisabld An observation: Reading the position via ':CHAN0:POS?' runs into a timeout when the sensor is disabled. So don't do that.
Since the numerical range of positions is more than what a 32 bit value can hold, the controller uses 64 bit (signed) integers. However, we don't need the full 64 bit range. For the real existing hardware we can use a double. So cleanup and remove the SMARACT_ASYN_ASYNPARAMINT64 and all code around "ireadback" and keep "freadback" as is.
The documentation says that the range of a "step move" is limited: -100000..-1 or +1..+100000 Moving 0 steps doesn't make sense. Clean up the handling: There is no need for "long long PositionType" or steps_to_go_i Use double instead. Add a range check.
When there is no enabled sensor and a move is done by commanding n steps try to calculate the theoretical position according to the direction and step size forward/backward. Site note: This makes motorPosition jump at the beginning of the movement. Should be better at the end. Add some error handling as well.
When there is a powered-on sensor, read ":CHAN%d:POS:TARG?", both open- and closed- loop. The controller will tell us where the positioner should be.
Remove the stale definition: #define MCS2IReadbackString "IREADBACK"
Move "global" variables inside poll() into local blocks. Rename theoretical, because it is the internal setpoint.
Add a helper variable which is updated AFTER the move:
When running open loop without sensor, the motor position
is calculated according to the step size.
And this value is put into motorPosition before the move.
This may confuse users/3rd party systems.
Update the new PV after the move is done.
Changes to be committed:
modified: smarActApp/src/smarActMCS2MotorDriver.cpp
modified: smarActApp/src/smarActMCS2MotorDriver.h
Inside the driver we use somthing like if (sensorPresent_ && !sensorIsDisabled_ && !openLoop_) This means that a disabled sensor will turn a move() into an open loop movement. Make that visible to the user by setting openloop to 1
…eps_ Remove stepTargetPos_nm_ and stepTargetSteps_, all positions should stored in the asyn parameter motorPosition_ . Rename steps_to_go_f into steps_to_go (it is NOT forward, but floating point) Simplify the open loop handling in move()
While debugging an open loop movement with retries (which had the wrong values for stepsize) it turned out that the bits from PSTATE should be easier to follow. In other words: Print them in the IOC log
Unify the text(s) printed in poll() when one ore more bits change: Printthe all in uppercase
The calculation of the "theoretical motor position" was wrong, both for realative- and absolute- moves. Correct it
It seems as if the sensor is working like an incremental encoder: When it is disabled, it doesn't update. Solution: Update the controller, when we run open loop and the sensor is disabled, with the position that we think we are.
Remove ErrTxt, which is not used, since all information is in MsgTxt
Ever since we have the feature
info(asyn:READBACK,"1")
info(asyn:INITIAL_READBACK,"1")
... there is no need to have 2 asyn parameters for the same thing.
Remove the asyn parameter for "postioner type readback".
Map the readback record towards the "PTYP" asyn parameter.
Add a (commented out) asyn:READBACK for the setPositionerType record.
Changes to be committed:
modified: smarActApp/Db/MCS2_Extra.db
modified: smarActApp/src/smarActMCS2MotorDriver.cpp
modified: smarActApp/src/smarActMCS2MotorDriver.h
Fix a small issue when printing chanState in poll() :he function name was printed twice. Remove even the line number and shorten the whole thing.
Do no longer declare "int" when parsing responses: E.g.: These 2 lines mclf = atoi(pC_->inString_); asynMotorAxis::setIntegerParam(pC_->mclf_, mclf); collapse into one line: asynMotorAxis::setIntegerParam(pC_->mclf_, atoi(pC_->inString_));
Add a demo file with the new features.
Changes to be committed:
modified: smarActApp/Db/Makefile
new file: smarActApp/Db/ess-mcs2.template
Add a bunch of CSS Boy files.
They are a copy of
git@gitlab.esss.lu.se:torstenbogershausen/motorsmaract-css-boy.git
tag v1.0.0
(New changes will be done here, in the smarAct repo)
Changes to be committed:
new file: smarActExApp/op/CSS-Boy/IOC.opi
new file: smarActExApp/op/CSS-Boy/genopis/IOC-BIFROST-SpSl1.mid
new file: smarActExApp/op/CSS-Boy/genopis/IOC.end
new file: smarActExApp/op/CSS-Boy/genopis/IOC.start
new file: smarActExApp/op/CSS-Boy/genopis/Makefile
new file: smarActExApp/op/CSS-Boy/genopis/checkws.sh
new file: smarActExApp/op/CSS-Boy/genopis/e-cnen-vis.mid
new file: smarActExApp/op/CSS-Boy/genopis/e-cnen.mid
new file: smarActExApp/op/CSS-Boy/genopis/e-errtxt.mid
new file: smarActExApp/op/CSS-Boy/genopis/e-foff-vis.mid
new file: smarActExApp/op/CSS-Boy/genopis/e-foff.mid
new file: smarActExApp/op/CSS-Boy/genopis/e-homf-cal-homr.mid
new file: smarActExApp/op/CSS-Boy/genopis/e-openloop.mid
new file: smarActExApp/op/CSS-Boy/genopis/e-retry.mid
new file: smarActExApp/op/CSS-Boy/genopis/e-urip.mid
new file: smarActExApp/op/CSS-Boy/genopis/genExpertPiezo.sh
new file: smarActExApp/op/CSS-Boy/genopis/genExpertWithAuxBits.py
new file: smarActExApp/op/CSS-Boy/genopis/genIOC.sh
new file: smarActExApp/op/CSS-Boy/genopis/genMotor.sh
new file: smarActExApp/op/CSS-Boy/genopis/m-egu-rbv.mid
new file: smarActExApp/op/CSS-Boy/genopis/m-motor.mid
new file: smarActExApp/op/CSS-Boy/genopis/m-msgtxt.mid
new file: smarActExApp/op/CSS-Boy/genopis/motorx-piezo.mid
new file: smarActExApp/op/CSS-Boy/genopis/motorx.end
new file: smarActExApp/op/CSS-Boy/genopis/motorx.start
new file: smarActExApp/op/CSS-Boy/genopis/shiftopi.py
new file: smarActExApp/op/CSS-Boy/genopis/smaractAnalogInput.mid
new file: smarActExApp/op/CSS-Boy/genopis/smaractAnalogOutput.mid
new file: smarActExApp/op/CSS-Boy/genopis/smaractAxisExpert.end
new file: smarActExApp/op/CSS-Boy/genopis/smaractAxisExpert.mid
new file: smarActExApp/op/CSS-Boy/genopis/smaractMotorRecordField.mid
new file: smarActExApp/op/CSS-Boy/genopis/smaractaxisExpert.start
new file: smarActExApp/op/CSS-Boy/genopis/smaractaxisPiezoPSTAT.mid
new file: smarActExApp/op/CSS-Boy/motor-1-piezo.opi
new file: smarActExApp/op/CSS-Boy/motor-2-piezo.opi
new file: smarActExApp/op/CSS-Boy/smaractAxisExpert.opi
Add an(other) directory with IOC example files.
Note that these example does include "open loop"
Changes to be committed:
new file: iocs/smarActIOC/iocBoot/ioc-smaract-ymir-handheld/.gitattributes
new file: iocs/smarActIOC/iocBoot/ioc-smaract-ymir-handheld/.gitignore
new file: iocs/smarActIOC/iocBoot/ioc-smaract-ymir-handheld/run-ioc.sh
new file: iocs/smarActIOC/iocBoot/ioc-smaract-ymir-handheld/st-handheld.cmd
new file: iocs/smarActIOC/iocBoot/ioc-smaract-ymir-handheld/st-simulator.cmd
Rename the PVs to have the test system more compatible with
the production systems:
$(P)$(M)-something is now $(P)$(M)Something
Adapt the CSS Screens.
Changes to be committed:
modified: iocs/smarActIOC/iocBoot/ioc-smaract-ymir-handheld/st-handheld.cmd
modified: smarActApp/Db/ess-mcs2.template
modified: smarActExApp/op/CSS-Boy/genopis/e-homf-cal-homr.mid
modified: smarActExApp/op/CSS-Boy/genopis/e-openloop.mid
modified: smarActExApp/op/CSS-Boy/genopis/genExpertPiezo.sh
modified: smarActExApp/op/CSS-Boy/genopis/smaractaxisPiezoPSTAT.mid
modified: smarActExApp/op/CSS-Boy/smaractAxisExpert.opi
Add a macro to initialize the VAL field for STEPSIZEF and STEPSIZER
Remove superflous "setClosedLoop" when printing "setClosedLoop" changes (which actually switches the stage on and off)
Do not use printf() in home(): use asynPrint() instead
Add a simulator written in rust. But: The simulater is good enough to demonstrate and test the driver. Know issues: Disabling the sensor in the hardware puts the stage into not homed after the next movement. And looses the position, so it must be re-homed with HOMF or HOMR, depending where we are compared to the home mark. Changes to be committed: file: test/smaract-sim-rs/smaract-sim-rs/.gitignore file: test/smaract-sim-rs/smaract-sim-rs/Cargo.lock file: test/smaract-sim-rs/smaract-sim-rs/Cargo.toml file: test/smaract-sim-rs/smaract-sim-rs/Makefile file: test/smaract-sim-rs/smaract-sim-rs/log.txt file: test/smaract-sim-rs/smaract-sim-rs/run.sh file: test/smaract-sim-rs/smaract-sim-rs/src/Makefile file: test/smaract-sim-rs/smaract-sim-rs/src/main.rs file: test/smaract-sim-rs/smaract-sim-rs/src/n2 file: test/smaract-sim-rs/smaract-sim-rs/src/netcommand.rs file: test/smaract-sim-rs/smaract-sim-rs/src/netcommand/mcs2axis.rs
Merge all upstream changes with one exception:
Take smarActApp/src/smarActMCS2MotorDriver.cpp and .h from us.
Conflicts:
.github/workflows/ci-scripts-build-full.yml
.github/workflows/ci-scripts-build.yml
smarActApp/src/smarActMCS2MotorDriver.cpp
smarActApp/src/smarActMCS2MotorDriver.h
smarActApp/src/smarActMCSMotorDriver.cpp
smarActApp/src/smarActMCSMotorDriver.h
smarActApp/src/smarActSCUMotorDriver.cpp
Prepare for the next commit: The position of the positioner is "lost" when the sensor is disabled. In other words: we need to introduce another variable.
When the sensor is disabled, do not update the position reported
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improve the open loop handling:
The step size forward and backward can be configured,
which allows better to keep track of the position when switching open/closed loop
Various other small improvements.
Add a simulator to test this driver