Repository navigation
Move the beam centre to target position by nudging the piezos - #186
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
==========================================
+ Coverage 98.90% 99.11% +0.20%
==========================================
Files 36 38 +2
Lines 733 901 +168
==========================================
+ Hits 725 893 +168
Misses 8 8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks, generally looks good, I'm glad it worked! Some comments in code but no real showstoppers
| return (x, y) | ||
|
|
||
|
|
||
| # TODO Create model in daq-config-server |
There was a problem hiding this comment.
Should: Can you add an issue for it in the config server?
| return lut.columns | ||
|
|
||
|
|
||
| def _get_lut_path_and_column_from_name(device_name: str) -> tuple[Path, int]: |
There was a problem hiding this comment.
Should: It feels like this info should live in the device?
There was a problem hiding this comment.
Made a separate issue for this, as eh2 will also want to use this code but they need their own lookup tables first - and I don't much like the idea of temporarily having the device just use the eh1 lut for both hutches. Issues in code.
| def _get_lut_path_and_column_from_name(device_name: str) -> tuple[Path, int]: | ||
| match device_name: | ||
| case "hfm_piezo": | ||
| return (HFM_LUT, 1) |
There was a problem hiding this comment.
Could: If we're only using the one column then I think we should just have the one in the file
There was a problem hiding this comment.
I'm not sure I like the idea of not having both columns, mainly because it's a good indication of the fact that even if slightly each piezo does shift the beam in the other direction too (iirc the vfm more than the hfm)
That being said, I admit I don't like this function just couldn't think of anything else for now...
| return nudge_size | ||
|
|
||
|
|
||
| def _check_position_reached( |
There was a problem hiding this comment.
Could: Does it make more sense to do the pythagorian distance here? In practice probably makes very little difference
There was a problem hiding this comment.
Probably yeah, even if it doesn't change things that much.
| # NOTE. The following plans should in the future become just one (if even needed), but | ||
| # until further testing can be done it would be easier to have two separare entry points | ||
| # for the two piezos. |
There was a problem hiding this comment.
Should: I think we should decide now whether to remove them or make them into one and do it in this PR
There was a problem hiding this comment.
At the very least I would keep one, considering we're going to need to run the test again now that the work on the vfm is done and its lookup table has been updated - it was much easier to debug things one piezo at a time then when running both. And we'll also need it to do the same on eh2 at a later point anyway...
| """Stops the stats plugin at the end of the loop""" | ||
| yield from bps.abs_set( | ||
| beam_centre.stats.enable_callbacks, EnableDisable.DISABLE, group=group | ||
| ) | ||
| yield from bps.abs_set(beam_centre.stats.compute_statistics, False, group=group) | ||
| yield from bps.abs_set(beam_centre.stats.compute_centroid, False, group=group) | ||
| yield from bps.abs_set(beam_centre.stats.compute_profiles, False, group=group) | ||
| yield from bps.abs_set(beam_centre.stats.compute_histogram, False, group=group) |
There was a problem hiding this comment.
Should: The setup for this is in the device itself so it feels like the teardown should be too. I think the most logical way to do this would be to replace what you're doing in trigger with a stage and then put this stuff into unstage?
2 participants
Closes #166
Closes #180
Needs dodal#2227
Add plans to move the beam centre by nudging the piezos in a loop.
Tested on the beamline and gave promising results even with the vfm not in a closed loop. Will need to retest next occasion with beam as vfm is now in a closed loop, but I think it would be better to get this bit merged and make any fixes in a separate branch/issue, as the PR is getting quite big.