-
Notifications
You must be signed in to change notification settings - Fork 1
Add top level attributes and commands to simplify API #82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
91267b2 to
68b31e4
Compare
Update to fastcs-odin 0.8.0a1
68b31e4 to
1979b0c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #82 +/- ##
==========================================
- Coverage 84.86% 82.00% -2.87%
==========================================
Files 13 14 +1
Lines 403 450 +47
==========================================
+ Hits 342 369 +27
- Misses 61 81 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shihab-dls
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left a couple of comments that are just enquiries; Will request changes just so that you can rerequest a review when codecov is satisfied.
| self.file_prefix = AttrRW( | ||
| String(), | ||
| io_ref=ConfigFanAttributeIORef([self.FP.file_prefix, self.MW.file_prefix]), | ||
| ) | ||
| self.acquisition_id = AttrRW( | ||
| String(), | ||
| io_ref=ConfigFanAttributeIORef( | ||
| [ | ||
| self.file_prefix, | ||
| self.FP.acquisition_id, | ||
| self.MW.acquisition_id, | ||
| self.EF.acqid, | ||
| ] | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the acquisition_id must always be the same as the FP and MW file_prefixes. With these changes, one could set self.file_prefix after self.acquisition_id with a different name. At the top level, should we not just expose:
| self.file_prefix = AttrRW( | |
| String(), | |
| io_ref=ConfigFanAttributeIORef([self.FP.file_prefix, self.MW.file_prefix]), | |
| ) | |
| self.acquisition_id = AttrRW( | |
| String(), | |
| io_ref=ConfigFanAttributeIORef( | |
| [ | |
| self.file_prefix, | |
| self.FP.acquisition_id, | |
| self.MW.acquisition_id, | |
| self.EF.acqid, | |
| ] | |
| ), | |
| ) | |
| self.acquisition_id = AttrRW( | |
| String(), | |
| io_ref=ConfigFanAttributeIORef( | |
| [ | |
| self.FP.file_prefix, | |
| self.MW.file_prefix, | |
| self.FP.acquisition_id, | |
| self.MW.acquisition_id, | |
| self.EF.acqid, | |
| ] | |
| ), | |
| ) |
such that one should/could only poke one PV consistently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not any real reason to set them differently, but I think some do. We could check that with some DAQ people.
@DominicOram do you have any thoughts on this?
| TimeoutError: If parameters are not synchronised or arm PUT request fails | ||
| """ | ||
| await self.stale_parameters.wait_for_value(False, timeout=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could: Now that we have wait_for_value with a timeout should we expose a DEFAULT_TIMEOUT in FastCS, such that we can ensure observation timeouts are consistent unless they explicitly shouldn't be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure there is a widely useful default - what else would you use that default for?
I am actually not sure if 1 second here is sufficient. Perhaps it should be an Attribute so it can be changed at runtime?
Uh oh!
There was an error while loading. Please reload this page.