Conversation
20ccd5b to
3df99f3
Compare
2cc6d15 to
f7c7993
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1768 +/- ##
=======================================
Coverage 99.11% 99.11%
=======================================
Files 319 318 -1
Lines 12267 12327 +60
=======================================
+ Hits 12158 12218 +60
Misses 109 109 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/dodal/devices/undulator.py
Outdated
| self.config_server = ConfigServer(url="https://daq-config.diamond.ac.uk") | ||
|
|
There was a problem hiding this comment.
I am not sure if this hardcoded config server is a good solution long term. I understand that it works fine now, but what if for some reason you need to divert your config server to a different endpoint - you would have to change the path here.
It feels like we need some king of an interface or a lookuptable provider which should probably be configured separately.
There was a problem hiding this comment.
Snap! I think my comment at #1773 (comment) is basically the same thing. I think we should define it in the ixx.py files like we do for e.g. a path provider
There was a problem hiding this comment.
yep, agreed - in an even broader perspective I don't think we should need daq-config-server as a dependency - but that's for later development DiamondLightSource/daq-config-server#157
…erver' into mx_bluesky_1494_use_config_server_for_undulator
c50d993 to
21b0c71
Compare
3d41b62 to
4dabfe9
Compare
4dabfe9 to
a32f358
Compare
5146aca to
c3f2952
Compare
| def pitch_energy_table(self) -> BeamlinePitchLookupTable: | ||
| return self.config_client.get_file_contents( | ||
| self._pitch_energy_table_path, desired_return_type=BeamlinePitchLookupTable | ||
| ) |
There was a problem hiding this comment.
Why not just keep the lookup table contents as a field in the device, and then you don't need to keep a reference to the config client, or re-read the config when you access it (which is going to be the same anyway until we rebuild the device)
There was a problem hiding this comment.
Plus, I think this gives potentially nicer failure behaviour in that the failure to reach the config server happens when you construct the device (which happens on baton handover or plan invocation) instead of later on which may potentially be some time afterwards.
src/dodal/devices/focusing_mirror.py
Outdated
|
|
||
| super().__init__(*args, name=name, **kwargs) | ||
|
|
||
| @property |
There was a problem hiding this comment.
Same consideration here really
Fixes DiamondLightSource/mx-bluesky#1494
Required by DiamondLightSource/mx-bluesky#1497
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}