Skip to content
This repository has been archived by the owner on Oct 11, 2023. It is now read-only.

Add a zebra device #11

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add a zebra device #11

wants to merge 4 commits into from

Conversation

rosesyrett
Copy link
Collaborator

This PR is intended to include a zebra device inside the suite of ophyd v2 devices. Some of this, especially the logic gates, is adapted from https://github.com/DiamondLightSource/python-artemis, and the other parts of this have been incorporated from my own work on making a v2 Zebra device.

A few things I'm unsure about;

  • in python-artemis, a timeout of 30 seconds was set for some PVs. I would like to recreate this functionality here, although 30 seconds seems a bit excessive.
  • would like to try out the Zebra simulator to ensure everything connects/works as it should before merging.
  • would like to write some tests for this device.

This PR is part of a larger work, https://jira.diamond.ac.uk/browse/DAQ-4527, looking at putting my v2 Zebra device onto here and in dodal (https://github.com/DiamondLightSource/dodal).

@rosesyrett rosesyrett requested a review from coretl March 13, 2023 16:40
@rosesyrett
Copy link
Collaborator Author

@coretl how would I go about setting a connection timeout for a PV with v2? This is something Dom's zebra device currently uses, so would be good to maybe expose this in the class.

@rosesyrett rosesyrett requested a review from DominicOram March 14, 2023 11:26
@rosesyrett
Copy link
Collaborator Author

The timeout problem is being investigated/expanded upon: bluesky/ophyd#1101

Tests are failing, going to have a look as to why. I thought I made my test devices have sim=True so that it wouldn't matter if they were actually connected to real underlying PVs.

Copy link

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments, mostly looks good

src/ophyd_epics_devices/zebra.py Outdated Show resolved Hide resolved
src/ophyd_epics_devices/zebra.py Outdated Show resolved Hide resolved
src/ophyd_epics_devices/zebra.py Outdated Show resolved Hide resolved
tests/test_zebra.py Outdated Show resolved Hide resolved
src/ophyd_epics_devices/zebra.py Outdated Show resolved Hide resolved
Comment on lines 293 to 298
class Zebra(StandardReadable):
def __init__(self, prefix: str, name: str = ""):
self.pc = PositionCompare("")
self.output = ZebraOutputPanel("")
self.logic_gates = LogicGateConfigurer("")
self.sys = System("")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I have a preference for mirroring the structure of the blocks on the Zebra opi screens, but I think this should be a wider discussion.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy for it to mirror the OPI more closely, I think it makes things easier

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have a crack at doing this...

I'm assuming opi screens == Epics screens when you open up a zebra

Copy link
Collaborator

@coretl coretl Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There is a heirarchy implied in both the screen layout and the record names. E.g.

  • PREFIX:PC_ARM could map to zebra.pc.arm: SignalX
  • PREFIX:PC_GATE_START could map to zebra.pc.gate.start: SignalRW[float]
  • PREFIX:DIV2_DIV could map to zebra.div[2].div: SignalRW[int]

There are also bitmap registers that correspond to many blocks:

  • Div1 "Trigger on" is PREFIX:POLARITY:B8 which could map to zebra.div[1].trigger_on: SignalRW[bool]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

structure now (hopefully) mirrors epics screens better

@prjemian
Copy link

prjemian commented Mar 21, 2023 via email

@prjemian
Copy link

prjemian commented Mar 23, 2023 via email

@rosesyrett
Copy link
Collaborator Author

Thanks for all the comments.

At the moment the tests are failing with:

./tests/test_zebra.py::test_setting_direction[Direction.positive] Failed with Error: [undefined]failed on setup with "NotImplementedError: No PV has been set as EpicsSignal.connect has not been called"

I have copied the test format from test_motor, yet I get this error. I'm unsure why this is happening, but I wonder if it's to do with the DeviceCollector not working properly with fixtures? I'm a bit confused. But at this point, this is a problem for another day. Would appreciate any help. Thanks

@coretl
Copy link
Collaborator

coretl commented Mar 24, 2023

I have copied the test format from test_motor, yet I get this error. I'm unsure why this is happening, but I wonder if it's to do with the DeviceCollector not working properly with fixtures? I'm a bit confused. But at this point, this is a problem for another day. Would appreciate any help. Thanks

This is bluesky/ophyd#1093

If you update pyproject.toml to point at ophyd 215bd0a43c1240788e176a67f30b9746321e2023 (current master) then it should work.

@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.52% 🎉

Comparison is base (c5efe8b) 98.60% compared to head (9b244ad) 99.13%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
+ Coverage   98.60%   99.13%   +0.52%     
==========================================
  Files           5        6       +1     
  Lines         359      575     +216     
==========================================
+ Hits          354      570     +216     
  Misses          5        5              
Files Changed Coverage Δ
src/ophyd_epics_devices/zebra.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rosesyrett rosesyrett requested review from coretl and DominicOram April 3, 2023 10:25
Copy link

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better, thanks! Some additional minor suggestions

src/ophyd_epics_devices/zebra.py Outdated Show resolved Hide resolved
src/ophyd_epics_devices/zebra.py Outdated Show resolved Hide resolved
src/ophyd_epics_devices/zebra.py Outdated Show resolved Hide resolved
src/ophyd_epics_devices/zebra.py Outdated Show resolved Hide resolved
@rosesyrett
Copy link
Collaborator Author

Working on this again; I've just rebased and forced push. Got rid of earlier commits as they weren't that clean.

@rosesyrett rosesyrett requested a review from DominicOram August 3, 2023 15:33
@rosesyrett
Copy link
Collaborator Author

One test failing now due to #32 which is currently being investigated.

@rosesyrett rosesyrett changed the title added a zebra device Add a zebra device Aug 3, 2023
Comment on lines +405 to +421
# RE = RunEngine()


# async def somefunc():
# async with DeviceCollector():
# # I think I'd like to do and_screen.and[1].inp[1]...
# # so let's start with inp[1]...

# # and_gates[1].inp[1]

# # want to do, inp[1].use for example...
# setup_cap = Zebra("BL03S-EA-ZEBRA-01")
# return setup_cap


# zebra = asyncio.run(somefunc())
# print("aha")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll get rid of these lines in a future commit... whoops

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants