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

Update p38 to use device_factory #921

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

Update p38 to use device_factory #921

wants to merge 3 commits into from

Conversation

DiamondJoseph
Copy link
Contributor

Updates p38 beamline definition to use device_factory

Instructions to reviewer on how to test:

  1. Check p38 beamline can still connect with dodal connect test

Checks for reviewer

  • Would the PR title make sense to a scientist on a set of release notes
  • If a new device has been added does it follow the standards
  • If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
  • Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.68%. Comparing base (ffdd9e8) to head (f74c791).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #921   +/-   ##
=======================================
  Coverage   97.68%   97.68%           
=======================================
  Files         160      160           
  Lines        6600     6617   +17     
=======================================
+ Hits         6447     6464   +17     
  Misses        153      153           

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

@stan-dot
Copy link
Contributor

dodal connect p38 fails but so it does on main, so it's a net improvement

Copy link
Contributor

@stan-dot stan-dot left a comment

Choose a reason for hiding this comment

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

better than main even if does not connect

@DiamondJoseph
Copy link
Contributor Author

DiamondJoseph commented Nov 26, 2024

Reveals #925 when trying to connect- connects to all non-sim devices

high_pressure_xray_cell: NotConnected:
    all_valves_control: NotConnected:
        fast_valve_control: NotConnected:
            5: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V5:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest
            6: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V6:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest
        valve_control: NotConnected:
            1: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V1:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest
            3: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V3:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest
    pressure_transducers: NotConnected:
        3: NotConnected:
            slow_beckhoff_voltage_readout: NotConnected: ca://BL38P-EA-ADC-03:CH1
ppump: NotConnected:
    direction: TypeError: Differing datatypes: BL38P-EA-PUMP-01:INFO:DIR has 0, BL38P-EA-PUMP-01:SET:DIR has 3
    state: TypeError: Differing choices: BL38P-EA-PUMP-01:INFO:RUN has ('STOPped', 'STARTed'), BL38P-EA-PUMP-01:SET:RUN has ('STOP', 'START')

@stan-dot @barnettwilliam can I leave this with you to resolve the ppump and hpxc?

d3, d12, i0 I am investigating, as they also do not connect

e: i0, it are both disconnected from the synoptic screen on p38: I can only assume the IOC has not been started up since power loss.

@DiamondJoseph DiamondJoseph marked this pull request as draft November 26, 2024 16:06
@stan-dot
Copy link
Contributor

ppump
idk where it is tbh

@stan-dot stan-dot mentioned this pull request Nov 27, 2024
4 tasks
@stan-dot
Copy link
Contributor

fixing this now

high_pressure_xray_cell: NotConnected:
    all_valves_control: NotConnected:
        fast_valve_control: NotConnected:
            5: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V5:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest
            6: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V6:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest
        valve_control: NotConnected:
            1: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V1:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest
            3: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V3:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest
    pressure_transducers: NotConnected:
        3: NotConnected:
            slow_beckhoff_voltage_readout: NotConnected: ca://BL38P-EA-ADC-03:CH1

@stan-dot
Copy link
Contributor

slits return not connected at the moment, even though they should be mocked

@stan-dot
Copy link
Contributor

I presume that the mock logic is not working for the slits

@stan-dot stan-dot marked this pull request as ready for review November 27, 2024 14:39
@stan-dot stan-dot requested a review from a team as a code owner December 11, 2024 11:49
Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

@DiamondJoseph and @stan-dot can you recommend a machine to run the system tests from? I need a RHEL8 one that can access the P38 PVs

src/dodal/devices/pressure_jump_cell.py Show resolved Hide resolved
Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

System tests are currently failing for me

logs.txt

@stan-dot
Copy link
Contributor

@callumforrester please include such long logs as a file not as a comment string

@callumforrester
Copy link
Contributor

@stan-dot my bad, edited

@callumforrester
Copy link
Contributor

System tests continuing to fail:
dodal-connect-p38-log.txt

@stan-dot
Copy link
Contributor

I am not sure how to track what is the actual status of the p38 beamline. maybe some things don't connect because they are physically disconnected

that was the case for i20-1

@stan-dot
Copy link
Contributor

discussed the connection issues with @barnettwilliam , some pressure jump device issues waiting for #812 (?)

@stan-dot
Copy link
Contributor

and slits_N where N is from 1 to 6 also are not connected

caget times out:
caget BL38P-MO-FSWT-01:FILTER-101:STATUS_RBV

@stan-dot
Copy link
Contributor

@DiamondJoseph have there been any issues with the mock not working with device_factory? that's what I suspect is happening

@DiamondJoseph
Copy link
Contributor Author

@DiamondJoseph have there been any issues with the mock not working with device_factory? that's what I suspect is happening

That does appear to be the case. I know blueapi respects mock=True, but I thought the dodal connect tests did too. Leave it to me, I'll look at it tomorrow.

@stan-dot
Copy link
Contributor

but I thought the dodal connect tests did too

fantastic... this was one of the edge cases that I got stuck at when doing device_factory back in October before you did your implementation

I was trying to make it work there too, and now we're witnessing a miss

this is an issue for all the 'update-ixx' PRs

@stan-dot stan-dot force-pushed the update-p38 branch 2 times, most recently from 5abf23b to 4053d8b Compare February 3, 2025 11:24
@stan-dot
Copy link
Contributor

stan-dot commented Feb 3, 2025

this is the relevant issue

#925

@barnettwilliam
Copy link
Contributor

Testing in P38 and excluding known errors for the mock devices, the expected devices except the pressure cell (d3, d12, i0m, and ppump) are connecting.

The pressure cell errors are down to the types returned by the OPENSEQ pvs being int and they are enums in dodal so the pressure jump dodal device should be corrected.

high_pressure_xray_cell: NotConnected:
    all_valves_control: NotConnected:
        fast_valve_control: NotConnected:
            5: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V5:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest
            6: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V6:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest
        valve_control: NotConnected:
            1: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V1:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest
            3: NotConnected:
                open: TypeError: BL38P-EA-HPXC-01:V3:OPENSEQ with inferred datatype int cannot be coerced to ValveOpenSeqRequest

@barnettwilliam
Copy link
Contributor

Created #1034 to fix the pressure jump cell type issue.

@stan-dot
Copy link
Contributor

@barnettwilliam after the latest merge we've got a branch conflict, could you please take a look?

@barnettwilliam
Copy link
Contributor

Yes will do

@barnettwilliam
Copy link
Contributor

@stan-dot conflicts are now resolved

@stan-dot
Copy link
Contributor

the non-connecting devices are those that fail due to the bug with device_factory interacting with dodal connect

related ticket (?)
#973

image

@callumforrester I'd say this is bug-for-bug compatible with the main branch and ready to merge

@stan-dot stan-dot dismissed callumforrester’s stale review February 13, 2025 10:15

two months have passed

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.

4 participants