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

driver/make Decadac verbosely fail for out-of-range voltages #857

Merged

Conversation

WilliamHPNielsen
Copy link
Contributor

It is very dangerous to have instrument drivers silently fail you when you ask them to do stupid things. If I ask the Decadac to output 1000 V on one channel, it is highly unlikely that I actually meant "go to the end of your range ASAP", and rather, I should be informed that I am trying to do something wrong.

This PR protects the Decadac against "out of range attacks". I hope that I found the right place in the driver to implement the safety precaution.

Changes proposed in this pull request:

  • Make driver raise a ValueError if asked to set an out-of-range voltage.

@jenshnielsen @spauka

@jenshnielsen
Copy link
Collaborator

👍 is there still any chance for a +- 1 over/underflow that we need to catch?

@WilliamHPNielsen
Copy link
Contributor Author

Hmm, good question. I guess it is always better to be on the safe side.

@@ -26,12 +26,14 @@ def _dac_v_to_code(self, volt):
based on the minimum/maximum values of a given channel.
Midrange is 32768.
"""
if volt < self.min_val or volt >= self.max_val:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this >= on purpose? Without looking at the implementation, as a user I would interpret max_val as the maximally allowed value....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is on purpose, but I agree that it is not too fitting.

The thing is that min_val maps to 0 and max_val maps to 2**16. So that's one value too many. But I think that current technology can solve even this.

@jenshnielsen
Copy link
Collaborator

I would like to merge this but can we add the checks back to ensure that off by one does not give an overflow?

@WilliamHPNielsen
Copy link
Contributor Author

@jenshnielsen Yes, I'll do that today.

@WilliamHPNielsen
Copy link
Contributor Author

There we go. This issue is really a good case for having a simulated instrument so that we may have a test for such safety measures.

@jenshnielsen jenshnielsen merged commit c5d3352 into microsoft:master Dec 4, 2017
giulioungaretti pushed a commit that referenced this pull request Dec 4, 2017
Author: William H.P. Nielsen <whpn@mailbox.org>

    driver/make Decadac verbosely fail for out-of-range voltages (#857)
@WilliamHPNielsen
Copy link
Contributor Author

If I understand #904 correctly, the code we just merged is wrong!

@jenshnielsen
Copy link
Collaborator

No more than before I would say?

@WilliamHPNielsen
Copy link
Contributor Author

No, not more than before, but still dangerous.

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.

None yet

3 participants