PVPositionerSoftDone should set done False at start of move

[prjemian]

• close PVPositionerSoftDone doesn't update done value #916
• close Replace unit tests marked by the @pytest.mark.local decorator. #810

[prjemian]

Tests pass locally but not in CI here. Setting PR back to draft for now.

[prjemian]

I'd like to merge in the next day (so apstools can be released on Friday). Any objections to merging by the end of business today?

[gfabbris]

It looks good to me. I should be able to test it with the lakeshore that had issues before, but it'd be either late today or tomorrow morning.

[prjemian]

I'll wait until your report tomorrow. Or end of Thursday comes.

[gfabbris]

Tested using a Lakeshore 340. The original problem is fixed. However, I found another issue: if a ramp is used, the device marks the movement as done even though it is not. But I think I know where the problem is, the target signal is not being checked by inposition (this target is used when ramping because the readback value of the setpoint follows the ramp, being slowly changed by EPICS). Looks like all we need is to change:

apstools/apstools/devices/positioner_soft_done.py

Line 202 in 9e8a793

sp = self.setpoint.get()

to:

sp = self.setpoint.get() if self.update_target is False else self.target.get()

(this worked for the lakeshore)

[prjemian]

Trying that solution locally. That will need more investigation. Many unit tests (in apstools/devices/tests/test_positioner_soft_done.py) fail with that change.

[prjemian]

FWIW, here's the output from pytest covering the failed tests:

The first error is a Type Error:

>       inpos = math.isclose(rb, sp, abs_tol=tol)
E       TypeError: must be real number, not str

apstools/devices/positioner_soft_done.py:204: TypeError

so it might be easy to resolve.

[prjemian]

On debugging, the assignment sp = self.setpoint.get() if not self.update_target else self.target.get() must be a bit more nuanced. Most of the cases that fail happen when self.target.get() is "None". (tracking that down next).

One other case fails differently:

FAILED apstools/devices/tests/test_positioner_soft_done.py::test_move_and_stopped_early - AssertionError: p.name='pos'  rb=6.88000 sp=6.88000 tol=0.0001  dt=0.2817s  p.done=Signal(name='pos_done', parent='pos', value=False, timestamp=1...

[gfabbris]

@prjemian: Trying to fix the other PR I ended up screwing up this branch. To fix it, I think it will involve reverting to commit 9a5afb9, then re-merging main into 916-PVPositionerSoftDone-update (commit f6c366d)

[prjemian]

Alternatively, can you create a new branch and copy into it the changes you have?

[prjemian]

Locally, I still have the most recent branch commit. These are the last two commits:

(base) prjemian@arf:~/.../BCDA-APS/apstools$ git log
commit b49aebbdcbf352f72fa96607d71cb01891474f99 (HEAD -> 990-listdevice-RuntimeError, origin/990-listdevice-RuntimeError)
Author: Pete R Jemian <prjemian@gmail.com>
Date:   Mon Aug 26 16:59:13 2024 -0500

    MNT #990

commit 4a152d0894c9395045eaea6ad00b7721163dfc15 (origin/main, origin/HEAD, main)
Merge: 66256f66 ae2bc654
Author: Pete R Jemian <prjemian@users.noreply.github.com>
Date:   Wed Aug 21 14:25:50 2024 -0500

    Merge pull request #1011 from BCDA-APS/1010-nodefaults
    
    Remove Anaconda defaults channel
    
    @MDecarabas Thanks!

[prjemian]

Let's go with these changes, over #1014. Later, we can restore the Py3.8 run in the CI.

[gfabbris]

These changes:

• Keeps setpoint and readback as normal EpicsSignal. But now, changes to the setpoint are sent to the target through a subscription.
• Forces the done signal to be False when the device is moved.
• Changes the default value of use_target to False. There is one disadvantage of using the target signal: if you change the setpoint "manually" in EPICS (and the device moves to the new setpoint), the Bluesky device will not know that it is inposition because target was not updated. Therefore, I recommend to set use_target=True only when EPICS changes the setpoint value while ramping (like in the Lakeshore 340).