Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #912 +/- ##
=======================================
Coverage 96.04% 96.04%
=======================================
Files 136 136
Lines 5639 5645 +6
=======================================
+ Hits 5416 5422 +6
Misses 223 223 ☔ View full report in Codecov by Sentry. |
noemifrisina
requested review from
DominicOram,
d-perl,
olliesilvester and
rtuck99
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks, a couple of comments
src/dodal/beamlines/i24.py
Outdated
| def attenuator( | ||
| wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False | ||
| ) -> AttenuatorBase: | ||
| """Get a basic (without filter wheels) attenuator device for i24, instantiate it |
There was a problem hiding this comment.
Should: This is a bit misleading, there are filter wheels on the I24 attenuator:
.
There was a problem hiding this comment.
Yes, but for the moment there's no filter wheel device (Made a ticket to write one #927 , which will also be needed soon for i19) and I24 doesn't need it at this stage... Beyond making it clearer in the comment that it's only temporary and will be added in the future I'm not sure what could be added here?
src/dodal/devices/attenuator_base.py
Outdated
| class AttenuatorBase(StandardReadable): | ||
| """An attenuator base class with a minimum set of common PVs to phase1 beamlines.""" |
There was a problem hiding this comment.
Should: I think it's worth making clear here that the read transmission will be fractional i.e. 0-1 not a percentage
src/dodal/devices/attenuator_base.py
Outdated
| self._desired_transmission = epics_signal_rw(float, prefix + "T2A:SETVAL1") | ||
| self._use_current_energy = epics_signal_x(prefix + "E2WL:USECURRENTENERGY.PROC") |
There was a problem hiding this comment.
Should: Either this attenuator is only used for reading and we remove these PVs or we want to add the setting too, in which case AttenuatorBase should have a set.
If we're going to do setting as well I actually think you might be able to get away with exactly the same set between the two. In the i24 case:
_filters_in_positioncontains onlyBL24I-OP-ATTN-01:MP1:INPOSandBL24I-OP-ATTN-01:MP2:INPOS_calculated_filter_statesare soft signals that are all high_changeis a soft signal that's a no-op
If we don't do this in this issue can you put this in a new issue please?
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
I think it would be clearer about what's going on if we just reframe it all as read-only vs settable Attenuators. I've done this in #931, let me know what you think. Sorry to jump on it and re-write it but it seemed easier than trying to articulate what I meant
noemifrisina
dismissed
DominicOram’s stale review
Changes addressed, has been r-reviewed since
* Create an AttenuatorBase class and use it * Link issue and clarify comment * Add to docstring for fractional transmission * Add link to issue * Rename BaseAttenuator to ReadOnlyAttenuator (#931) --------- Co-authored-by: Dominic Oram <dominic.oram@diamond.ac.uk>
3 participants
Fixes #889
Creates an AttenuatorBase with a minimum set of common PVs that beamline-specific devices can inherit from.
Used in MX_bluesky#627
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}