Skip to content
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

Improvements to 344xx driver #1918

Merged
merged 7 commits into from
Feb 11, 2020
Merged

Conversation

spauka
Copy link
Contributor

@spauka spauka commented Feb 10, 2020

This change implements a fix to the 344xx driver that stops the DMM from cycling through all its measurement modes on a snapshot/initialization.

Changes proposed in this pull request:

  • Don't include measurements in a snapshot that would cause the measurement mode to change.
  • Make ask abstract in the AbstractInstrument class to fix a bunch of pylint errors.

@jenshnielsen @WilliamHPNielsen

@codecov
Copy link

codecov bot commented Feb 10, 2020

Codecov Report

Merging #1918 into master will not change coverage.
The diff coverage is 86.95%.

@@           Coverage Diff           @@
##           master    #1918   +/-   ##
=======================================
  Coverage   70.41%   70.41%           
=======================================
  Files         153      153           
  Lines       18949    18949           
=======================================
  Hits        13342    13342           
  Misses       5607     5607

@jenshnielsen
Copy link
Collaborator

@spauka Thanks, I think this looks fine. One question is would it worth the trobuble to switch which parameters are snapshotted depending on the sense mode? The yokogava gs200 driver does something along those lines as far as I remember. In general you don't worry about pylint errors that requires markup inline to fix. It's not a requirement that codacy comes back clean. We specifically only run pylint on the diff of a pr for this reason.

Here it's specifically caused by the way that get and set are automatically constructed from parameters.

@spauka
Copy link
Contributor Author

spauka commented Feb 10, 2020

@jenshnielsen Thanks for checking :) Regarding snapshotting, the previously read value is still stored in the snapshot as part of the cache, however, it is not updated when connecting to the instrument for the first time or when performing a snapshot, i.e.:

In[12]: dmm.volt()
Out[12]: 0.00179960528

In[13]: snap = station.snapshot(update=True)

In[14]: snap["instruments"]["dmm1"]["parameters"]["volt"]["value"]
Out[14]: 0.00179960528

It is not clear that it would be desirable to read a new value anyway, since depending on the aperture/NPLC setting of the DMM, it could be a long running operation.

The logic in the GS200 driver was slightly different, since we wanted to prevent someone from trying to set the current when in voltage mode and vice versa, hence we needed to keep track of the state of the instrument.

However, let me know if there's any changes you would like made, I can implement a cached mode if you like or revert linting fixes? FWIW I think ask probably wants to be an abstractmethod in the class anyway?

@jenshnielsen
Copy link
Collaborator

Hmn yes I don't think it makes sense to have any of these values in the snapshot. But perhaps we should just add a small comment above the first of them explaining why they are excluded for future reference?

@spauka
Copy link
Contributor Author

spauka commented Feb 10, 2020

Agreed and done :)

@jenshnielsen jenshnielsen merged commit dceef8d into microsoft:master Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants