Yokogawa GS200: Exclude/Include snapshot depending on mode #1699
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1699 +/- ##
==========================================
- Coverage 67.26% 67.25% -0.01%
==========================================
Files 145 145
Lines 17946 17946
==========================================
- Hits 12071 12070 -1
- Misses 5875 5876 +1 |
lakhotiaharshit
changed the title
| @@ -352,6 +357,21 @@ def _ramp_source(self, ramp_to: float, step: float, delay: float) -> None: | |||
| self.output_level.step = saved_step | |||
| self.output_level.inter_delay = saved_inter_delay | |||
|
|
|||
| def _exclude_snapshot(self, mode: str) -> bool: | |||
There was a problem hiding this comment.
it seems to me that this method is not really needed if it's only used at init of the instrument. would you agree?
There was a problem hiding this comment.
I think so. The alternative would be to extend snapshot_exclude from a bool to a function executed at snapshot time
There was a problem hiding this comment.
@astafan8 I have removed the _exclude_snapshot method and excluded current and current_range from the snapshot at init.
|
I would not be against adding a comment above the changes to the snapshot explaining why |
| else: | ||
| self.range = self.current_range | ||
| self.output_level = self.current | ||
| self.voltage_range.snapshot_exclude = True | ||
| self.voltage.snapshot_exclude = True | ||
| self.current_range.snapshot_exclude = False |
There was a problem hiding this comment.
@lakhotiaharshit I have a suspicion that setting the snapshot to True, will require getting the parameter, or at least filling the parameter cache with a known value.
There was a problem hiding this comment.
Could you check up on this?
There was a problem hiding this comment.
@Dominik-Vogel I don't think that require is the correct word here.
setting the snapshot to True, will require getting the parameter, or at least filling the parameter cache with a known value
Did you mean to say that when the parameter gets "switched" from .snapshot_exclude = False to .snapshot_exclude = True (because the instrument mode is switched between voltage/current modes), it's probably useful that the value of that parameter gets updated immediately such that value is "valuable" for use, for example, when snapshot(..) is requested?
i also think that this driver generally needs improvement for proper representation of the fact that it can operate in these two modes. for example, why it is the case that when parameter is in current mode the user still has the voltage parameter available on the instrument and it's get/set don't throw errors saying "you're not in a voltage mode hence you can use the voltage parameter". So i think that this particular PR just quick-fixed the snapshotting of the instrument, and that's it.
There was a problem hiding this comment.
Mikhail, I think we do agree in general. Let me though elaborate a little on why I used the stronger term require: As far as I understand we do have the policy, that the station snapshot contains the current state of the station. Or maybe even: the station snapshot contains the current state of the station at any time. For this reason we do read all the parameter values on adding an instrument to the station. Now there are exceptions, marked with snapshot_exclude to cover the case of parameters that are not definable at the given time.
Therefore I think that when changing the validity of parameters should update them in the snapshot automatically.
(I just commented on this PR because this fix showed the problem more clearly, I have no critique for the PR itself, but just wanted to get Harshits opinion, who probably has thought about it a lot more that I have)
Yokogawa GS200 can be a voltage source or current source. When in one mode the parameters of another mode can be excluded from the snapshot.
Things to do: