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

Remove obsolete PV PoolMaxBuffers #1173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xiaoqiangwang
Copy link

This PV was removed from ADCore since 3.3.1, see
areaDetector/ADCore@47c70bf

This PV was removed from ADCore since 3.3.1, see
areaDetector/ADCore@47c70bf
@tacaswell
Copy link
Contributor

This is not the only place we we have this PV set (it is also on the plugins).

We have support for AD versions back to 1.9 iirc. If we are going to start pushing up the minimum version of AD we support then there is a whole bunch more that can be pulled out.

@xiaoqiangwang
Copy link
Author

xiaoqiangwang commented Jan 10, 2024

This PV is an information to the configured buffer limit. Removing it is unlikely to break application code. But keeping it does break when connecting to areaDetector 3.4+ IOCs.

#1159 addresses a similar issue with areaDetector file plugin.

@tacaswell
Copy link
Contributor

I think we either need to do something like we do with the plugins and have generations of these classes to match the AD versions or put a floor on supported AD versions and simplify a lot of the code.

We are using newer IOCs at NSLS-II. I see a few local patches like this and I suspect in other cases we are setting it to not be in the configuration so we just happen to never connect.

@xiaoqiangwang
Copy link
Author

I suspect in other cases we are setting it to not be in the configuration so we just happen to never connect.

Exactly, we find out such broken links only with,

wait_for_connection(all_signals=True)

@prjemian
Copy link
Contributor

@prjemian
Copy link
Contributor

We've been using the updates in apstools for AreaDetector versions as high as 3.12 (current).

@prjemian
Copy link
Contributor

This is not the only place we we have this PV set (it is also on the plugins).

AFAIK, the V34 area detector plugins in ophyd are working up to the current AD tag {3.12.1).

Instead of this PR (or as branch to this one), I propose hoisting these classes from apstools to ophyd:

  • apstools.devices.area_detector_support.CamMixin_V3_1_1
  • apstools.devices.area_detector_support.CamMixin_V34
  • apstools.devices.area_detector_support.SingleTrigger_V34

@xiaoqiangwang
Copy link
Author

  • apstools.devices.area_detector_support.CamMixin_V3_1_1
  • apstools.devices.area_detector_support.CamMixin_V34
  • apstools.devices.area_detector_support.SingleTrigger_V34

After a period of idling I come back to implement the versioned CamBase, but realised one problem. i.e. Unlike NDPlugins, a special camera class e.g. SimDetectorCam has its own version and at the same time derives from a range of CamBase_Vxx.
The number of classes (SimDetectorCam_BaseVxx_Vxx) would be the multiplication of both. This becomes quickly intractable.

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