Conversation
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Some comments in code, otherwise good, thank you!
src/dodal/beamlines/i03.py
Outdated
|
|
||
| def sample_shutter( | ||
| wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False | ||
| ) -> Attenuator: |
There was a problem hiding this comment.
Must: Type hint is wrong
There was a problem hiding this comment.
Should: Can we have a docstring on this too?
| acquire: EpicsSignal = Component(EpicsSignal, "Acquire") | ||
| acquire_rbv: EpicsSignalRO = Component(EpicsSignal, "Acquire_RBV") |
There was a problem hiding this comment.
Could: You can use EpicsSignalWithRBV for this
| def do_start(self) -> Status: | ||
| self.erase.put(EraseState.ERASE.value) | ||
| status = self.channel_1.update_arrays.set(AcquireState.DONE.value) | ||
| # status = self.channel_1.update_arrays.set(AcquireState.DONE.value) |
There was a problem hiding this comment.
Should: Remove this rather than comment out
| def do_start(self) -> Status: | ||
| self.erase.put(EraseState.ERASE.value) | ||
| status = self.channel_1.update_arrays.set(AcquireState.DONE.value) | ||
| # status = self.channel_1.update_arrays.set(AcquireState.DONE.value) | ||
| # GDA code suggests this put does not callback until collection finished, for now just hold on to it | ||
| self.acquire_status = self.acquire.set(AcquireState.ACQUIRE.value) | ||
| return status | ||
| return self.acquire.set(AcquireState.ACQUIRE.value) | ||
|
|
||
| def arm(self) -> Status: |
There was a problem hiding this comment.
Should: I think we can just combine these two methods
| self.erase.put(EraseState.ERASE.value) | ||
| status = self.channel_1.update_arrays.set(AcquireState.DONE.value) | ||
| # status = self.channel_1.update_arrays.set(AcquireState.DONE.value) | ||
| # GDA code suggests this put does not callback until collection finished, for now just hold on to it |
There was a problem hiding this comment.
Should: We discovered on the beamline that this comment is wrong
| arm_status = await_value_in_list(self.detector_state, self.detector_busy_states) | ||
| return arm_status | ||
| arm_status &= self.do_start() | ||
| arm_status.wait(1) |
There was a problem hiding this comment.
Could: Would be nice to pull this timeout into a constant
Codecov Report
@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 90.00% 90.11% +0.11%
==========================================
Files 58 58
Lines 2010 2004 -6
==========================================
- Hits 1809 1806 -3
+ Misses 201 198 -3
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
2 participants
Fixes DiamondLightSource/hyperion#779
To test:
Check new PV names
Check and run new tests