Skip to content

Restructure OAV device organisation and naming (Artemis554)#25

Merged
d-perl merged 20 commits intomainfrom
artemis554
May 12, 2023
Merged

Restructure OAV device organisation and naming (Artemis554)#25
d-perl merged 20 commits intomainfrom
artemis554

Conversation

@d-perl
Copy link
Contributor

@d-perl d-perl commented Mar 16, 2023

Goes with changes in DiamondLightSource/hyperion#584

d-perl added 4 commits March 14, 2023 12:30
but keep derivative devices named snapshot
- move mjpg and mxsc to areadetector plugins
- separate out oav utils
- update i03
@DominicOram
Copy link
Contributor

Can you make the PR title a bit more descriptive, dodal will use it for release note generation so should probably use something that makes sense to general DLS devs

@d-perl d-perl changed the title Artemis554 Restructure OAV device organisation and naming (Artemis554) Mar 16, 2023
Copy link
Contributor

@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.

Changes here look mostly good. Most comments are on Artemis PR. Do we still want the component in the OAV to be called snapshot, after renaming it to MJPG?

Comment on lines +9 to +12
input_plugin_pv: EpicsSignal = Component(EpicsSignal, "NDArrayPort")
enable_callbacks_pv: EpicsSignal = Component(EpicsSignal, "EnableCallbacks")
min_callback_time_pv: EpicsSignal = Component(EpicsSignal, "MinCallbackTime")
blocking_callbacks_pv: EpicsSignal = Component(EpicsSignal, "BlockingCallbacks")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could: I think the word PV is a bit redundant here. We know they're all PVs

@d-perl
Copy link
Contributor Author

d-perl commented Mar 27, 2023

I left is as snapshot on purpose, think it makes sense this way. but happy to change it if you don't like it

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #25 (bbf3b6b) into main (cdf9698) will increase coverage by 1.01%.
The diff coverage is 96.24%.

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   83.77%   84.79%   +1.01%     
==========================================
  Files          51       52       +1     
  Lines        1609     1644      +35     
==========================================
+ Hits         1348     1394      +46     
+ Misses        261      250      -11     
Impacted Files Coverage Δ
src/dodal/i03.py 91.02% <75.00%> (-0.87%) ⬇️
src/dodal/devices/oav/oav_parameters.py 95.23% <94.02%> (-2.17%) ⬇️
src/dodal/devices/areadetector/plugins/MJPG.py 97.05% <100.00%> (ø)
src/dodal/devices/areadetector/plugins/MXSC.py 100.00% <100.00%> (ø)
src/dodal/devices/detector_motion.py 100.00% <100.00%> (+100.00%) ⬆️
src/dodal/devices/oav/grid_overlay.py 100.00% <100.00%> (ø)
src/dodal/devices/oav/oav_detector.py 96.15% <100.00%> (-2.29%) ⬇️
src/dodal/devices/oav/utils.py 100.00% <100.00%> (ø)
src/dodal/utils.py 98.30% <100.00%> (+0.02%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@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.

Thanks, mostly looks good, couple of questions in code

@@ -13,7 +13,7 @@ class Snapshot(Device):
x_size_pv: EpicsSignalRO = Component(EpicsSignalRO, "ArraySize1_RBV")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: We can remove this file entirely now in favour of MJPG, correct?

Copy link
Contributor

@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.

Great, thank you!

@d-perl d-perl merged commit 618a7ac into main May 12, 2023
@d-perl d-perl deleted the artemis554 branch May 12, 2023 09:44
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.

2 participants