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

Added support for the Thorlabs Temperature Controller #56

Merged
merged 3 commits into from Jan 24, 2016
Merged

Added support for the Thorlabs Temperature Controller #56

merged 3 commits into from Jan 24, 2016

Conversation

CatherineH
Copy link
Contributor

No description provided.

:return: the converted temperature
:rtype: `quantities.Quantity`
"""
# quantities reports equivalence between degC and degK, so a string comparison is needed
Copy link
Contributor

Choose a reason for hiding this comment

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

I just had to take a look for myself to see if you were right....and yep, pyquantities treats degC as the same as degK/kelvin. Rescaling between the two doesn't change the value...

This should most definitely be brought up with the pyquantities folk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already talked to the pyquantities people earlier today; they said that introducing offsets into scaling is outside the scope of the project.

Since C = K + offset, pyquantities sees C=K. Setting these as not equal would require significant work for them, so it won't be resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Um, what. ಠ_ಠ

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this is not the first time we've had a disagreement with them over how units should be handled, see https://github.com/Galvant/InstrumentKit/blob/dev/instruments/instruments/units.py where @cgranade added some additional units to our implementation.

@scasagrande
Copy link
Contributor

I'm seeing your styling changes from the other PR are also showing up in this PR

@@ -115,7 +123,13 @@ def query(self, cmd, size=-1):
connected instrument.
:rtype: `str`
"""
return self._file.query(cmd, size)
self._file.flush_input()
Copy link
Contributor

Choose a reason for hiding this comment

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

What purpose is self._file.flush_input() call having in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember. Let me take it out and see if it still works.

@scasagrande
Copy link
Contributor

Also merge conflicts should just be around the test files. I just merged my big unit-test-related PR

@CatherineH
Copy link
Contributor Author

I've merged with upstream and removed the flush input line.

@@ -47,3 +47,7 @@ nosetests.xml

#Mr Developer
.mr.developer.cfg

#pycharm generated
>>>>>>> origin
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete

fixed camel words issue with serial manager args, a little pep8 cleanup
@scasagrande
Copy link
Contributor

I like the idea of the prompt and echo support, but I think we should do it a little more robustly. Some of this I've already mentioned to @CatherineH over hangouts, but I'll try to get down my thoughts a little more clearly here.

From what I can tell, echo is essentially the instrument sending back some sort of ACK string any time you send something. Right now the echo implementation is only processing there ACK strings when calling Instrument.query. Instead what I think we should do is look to read and process these ACKs during Instrument.sendcmd. Basically, maybe have a Instrument._ack boolean, where when true, some additional processing is done in Instrument.sendcmd. The processing that I'm imagining is doing an Instrument.read(size=-1) call (since the ACK will be EOS terminated), and then checking that the value returned is as expected.

class Instrument(object):

    def sendcmd(self, cmd):
        self._file.sendcmd(cmd)
        if self._ack_expected(msg) is not None:
            if self.read(-1) is not self._ack_expected(msg): raise Exception

    def _ack_expected(msg):
        return None

Where _ack_expected can be overridden by instruments that have lookup rules.

Something can also be added in for the prompt support too. Looking at the tests in this PR, the format after sending something seems to be like this: ACK\r>\rRESP\r>\r but the tests aren't consistent with the number of \r. Basically for every read with data, another read should be done for the prompt, which should be checked for. Maybe something like this will work:

class Instrument(object):

    def sendcmd(self, cmd):
        self._file.sendcmd(cmd)
        if self._ack_expected(msg) is not None:
            if self.read(-1) is not self._ack_expected(msg): raise Exception

    def read(self, size = -1):
        resp = self._file.read(size)
        if self._prompt is not None:
            if self._file.read(-1) is not self._prompt: raise Exception

    def _ack_expected(msg):
        return None

@scasagrande
Copy link
Contributor

Going to ping @cgranade in the event he has time and an idea

@scasagrande scasagrande merged commit 86b7262 into instrumentkit:dev Jan 24, 2016
scasagrande added a commit that referenced this pull request Jan 24, 2016
@CatherineH
Copy link
Contributor Author

yay!

@scasagrande
Copy link
Contributor

lol love it

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

2 participants