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

Support for query() in transactions #99

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

czee
Copy link
Contributor

@czee czee commented Sep 16, 2019

It is possible to mix write() and query() calls in transactions. Only tested with Rigol instruments (DS1054, DG811, DP711).

As seen in the updated documentation:

Note that all query() calls need to return a string for the transaction. This is important when using a Facet since it is possible to make a Facet return an object.

Some instruments (Rigol DP711) do not support multiple commands for which transactions will not work as intended.

@@ -452,34 +452,40 @@ def write(self, message, *args, **kwds):

>>> inst.write('source{}:value {}', channel, value)
"""
full_message = message.format(*args, **kwds)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps this could be refactored in some way to avoid duplication with the code below in query()

@natezb
Copy link
Contributor

natezb commented Oct 2, 2019

I don't think it makes sense to change the behavior of query() within transactions because normally you'd expect a query to return a result (not None). This is especially true when a transaction-unaware function or method is called within a transaction, which makes it hard to reason about composing functions. For example

class MyDevice(VisaMixin):
    def maybe_do_something(self):
        if self.query(':state?') == 1:
            do_something()

    def do_more_stuff(self):
        with self.transaction():
            self.maybe_do_something()  # This call behaves unexpectedly

If you find it useful to chain queries together, then maybe we should add an explicit way to do it? I'd be curious to see what you think.

@czee
Copy link
Contributor Author

czee commented Oct 2, 2019

I'm not sure how you would ensure that query() returns something within a transaction since you have to string the whole message together first and then exit the transaction to flush the queue. This may require some decorator magic. Currently I use the functionality like this:

    with self.oscilloscope.transaction():
        self.oscilloscope.ppulses_min # query
        self.oscilloscope.ppulses_max # query 
        self.oscilloscope.scale1 = self.args.vertical_scale # write
        self.oscilloscope.timebase_main_scale = self.args.timebase_main_scale # write
    self.ppulses_min, self.ppulses_max = self.oscilloscope.response

As far as I can tell the existing functionality for query() calls in transactions is only to flush the message queue. In this change transaction query() calls will be returning data in the order in which they are called.

@czee
Copy link
Contributor Author

czee commented Oct 2, 2019

I think you would need something to track the variable after it's already inside the transaction. Not sure if that's possible:

with self.oscilloscope.transaction():
    myquery_response = self.oscilloscope.ppulses_min # query

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