ophyd icon indicating copy to clipboard operation
ophyd copied to clipboard

Ophyd v2 core

Open coretl opened this issue 2 years ago • 12 comments

Add:

  • Device baseclass
  • Signal baseclass
  • EpicsSignal with CA and Sim backends
  • Demo EPICS Device and IOC to drive it
  • Magics for some simple plan stubs
  • Docs and a tutorial
  • Reasonable test coverage

coretl avatar Oct 21 '22 14:10 coretl

What I'd like reviewed most is the docs. Downlowd and unzip https://github.com/bluesky/ophyd/suites/8898144421/artifacts/407113849 then point a browser at it and look at the User Guide v2 tutorials and how-tos.

coretl avatar Oct 21 '22 15:10 coretl

For some reason GitHub won't let me ask @tacaswell @whs92 and @callumforrester for a review...

coretl avatar Oct 21 '22 15:10 coretl

A couple of tests that worked on 3.9 but don't work on 3.8, will look at this next week...

coretl avatar Oct 21 '22 15:10 coretl

Since Channel Access PVs remain part of EPICS 7, change EPICS V3 PVs to EPICS Channel Access (V3) PVs

prjemian avatar Oct 21 '22 19:10 prjemian

I'll give the docs a readthrough, first impressions are very positive

ZLLentz avatar Oct 21 '22 20:10 ZLLentz

Since Channel Access PVs remain part of EPICS 7, change EPICS V3 PVs to EPICS Channel Access (V3) PVs

The top half of the main index that includes this text is actually the existing README of this repo- maybe the README needs a look-over. Not sure if that's in scope for v2 docs.

ZLLentz avatar Oct 21 '22 20:10 ZLLentz

I agree with the general flow of the documentation shown here. Very readable.

We're also being asked to comment on code?

I'm very concerned that the complexity shown in the example class Sensor(Configurable, Readable, Device): will add a barrier to further adoption of Bluesky at APS. How much of this can be hidden behind a much simpler interface, such as without type declarations?

prjemian avatar Oct 21 '22 21:10 prjemian

Since Channel Access PVs remain part of EPICS 7, change EPICS V3 PVs to EPICS Channel Access (V3) PVs

The top half of the main index that includes this text is actually the existing README of this repo- maybe the README needs a look-over. Not sure if that's in scope for v2 docs.

We're planning PVA support too, so I'd prefer to exclude this README from the scope of the review and come back to it when that's done

coretl avatar Oct 24 '22 11:10 coretl

We're also being asked to comment on code?

Yes please, I was aware that people had limited time, so I wanted people to focus on docs and the example user code that was being presented in the docs rather than the "behind-the-scenes" implementation of EpicsSignal for instance, but feel free to comment on anything in the PR.

I'm very concerned that the complexity shown in the example class Sensor(Configurable, Readable, Device): will add a barrier to further adoption of Bluesky at APS. How much of this can be hidden behind a much simpler interface, such as without type declarations?

I've made the Sensor demo Device use the same baseclass as the Mover device that gets rid of the boilerplate read, describe etc. methods. I didn't use it originally as I wanted to build up to it, but I can work the other way (start with the simple class using the helper mix-in, then show how you can implement those methods yourself in a more advanced How-To).

@prjemian please can you comment on the specific lines of ophyd/v2/epicsdemo/__init__.py that you'd like to hide away? Is it the connect() method? Or the explicit types in the EpicsSignal constructor? Or was it the Readable baseclass that has now gone?

coretl avatar Oct 24 '22 11:10 coretl

Converting to Draft until I've actioned all the comments...

coretl avatar Oct 24 '22 11:10 coretl

I'd like to discuss these changes in the community call on Thursday, can people make it to that?

coretl avatar Oct 24 '22 11:10 coretl

@ZLLentz and @prjemian I've simplified the process of making a Device by introducing a SimpleDevice helper class that does a lot of the boilerplate code for you. I also discussed magics with @danielballan and @tacaswell and I think we need a wider discussion on this. Dan was going to try and find a good time. In the spirit of "not inventing a third thing for users to learn", I've converted the tutorial to only use plans through the RunEngine (with a few less characters courtesy of bluesky.utils.register_transform)

Please could you have another look at the tutorial and how-to on the docs and see if this is any better? https://github.com/bluesky/ophyd/suites/8968599012/artifacts/412343865

coretl avatar Oct 26 '22 10:10 coretl