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

Multipatch dev #23

Merged
merged 25 commits into from
Mar 1, 2017
Merged

Multipatch dev #23

merged 25 commits into from
Mar 1, 2017

Conversation

JFPerkins
Copy link

@JFPerkins JFPerkins commented Jan 17, 2017

0MQ interactions with MIES

Polling for test pulse data.
Live updating for test pulses.
No more need for any ActiveX.
JSON formatting for pipette history.

fixes #1

Correct address string.
Default params to empty list.
Return values with "result" key instead of "return".
Setting pipette pressure modes no longer clicks button
MIES subclasses QObject to allow Timers and signaling.
Stubs for processUpdate and resetData.
@campagnola
Copy link

@JFPerkins, is this ready for review?

Copy link

@campagnola campagnola left a comment

Choose a reason for hiding this comment

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

This looks awesome!



def __reload__(old):
MIES._bridge = old['MIESBridge']._bridge

Choose a reason for hiding this comment

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

should be MIES._bridge = old['MIES']._bridge

self._pollTimer.timeout.connect(self._checkRecv)
self._pollTimer.start(100)

def __call__(self, cmd, *args, **kwds):

Choose a reason for hiding this comment

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

Should we do something with kwds ?

Copy link
Author

Choose a reason for hiding this comment

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

So my intention with kwds was to utilize them to handle the optional arguments if that ever gets supported on the Igor side (it currently isn't). For now they are ignored, but other options would be to remove them or to raise an error on them. Thoughts?

Choose a reason for hiding this comment

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

Let's just remove it for now.

return future

def _checkRecv(self):
socks = self._poller.poll(100)

Choose a reason for hiding this comment

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

Can we do poll(0) here? It would be better to avoid blocking the event loop.

Choose a reason for hiding this comment

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

Alternatively, I think we could set the receive timeout to 0 and ditch the poller altogether?

Copy link
Author

Choose a reason for hiding this comment

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

I'll test with receive timeout set to 0 and if that looks like it is still functional we'll go with that, since I don't really like having the poller either.

messageID = reply.get("messageID", None)
reply = self.parseReply(reply)
future = self._unresolvedFutures.pop(messageID, None)
if future:

Choose a reason for hiding this comment

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

When would we expect not future ? Is it safe to just discard such messages?

reply = self.parseReply(reply)
future = self._unresolvedFutures.pop(messageID, None)
if future:
future.set_result(reply)

Choose a reason for hiding this comment

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

I think there should also be a future.set_exception somewhere in here..

return self.setCtrl("setvar_DataAcq_SSPressure", pressure)

def setApproach(self, hs):
windowName = '"{}"'.format(self.windowName)

Choose a reason for hiding this comment

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

Why are you adding quotes here? This seems to be causing an error in mies:

Assertion FAILED in function IsControlDisabled(...) MIES_GuiUtilities.ipf:862.
Message: Non-existing control or window "ITC1600_Dev_0"


def quit(self):
self._exiting = True
if self._future:

Choose a reason for hiding this comment

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

This line causes AttributeError: 'MIES' object has no attribute '_future'


def setSeal(self):
windowName = '"{}"'.format(self.windowName)
return self.igor("P_MethodSeal", windowName, hs)

Choose a reason for hiding this comment

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

NameError: global name 'hs' is not defined

@campagnola
Copy link

Got an error:

TypeError: 'NoneType' object has no attribute '__getitem__'
  File "acq4\util\igorpro.py", line 250, in _checkRecv
    reply = self.parseReply(reply)
  File "acq4\util\igorpro.py", line 284, in parseReply
    return self.parseWave(val)
  File "acq4\util\igorpro.py", line 289, in parseWave
    dtype = self._types.get(jsonWave["type"], np.float)

@campagnola
Copy link

Lingering issues:

  • Seal button seems to have no effect
  • Plots are too crowded due to plot labels

@campagnola
Copy link

In the multipatch log file, event times are being stored as string rather than float.

@campagnola
Copy link

FYI: We are waiting on a MIES communication bugfix; hopefully will test this soon.

Meanwhile, I noticed changes to NIDAQmx_headers_win32.cache that should not be included (it's a big / automatically generated file). Can you rebase with those changes filtered out?

@JFPerkins
Copy link
Author

Yeah looks like that was accidentally committed back in October, will fix.

@JFPerkins
Copy link
Author

Looks like you had merged some changes in after that file was tracked, so the rebase was a little bit messy. I continued by accepting the changes on your merges compared to head but now it looks like there are conflicts.

@campagnola
Copy link

HRM. I'll have a peek.

@campagnola
Copy link

If you use rebase, don't change the base branch--we don't actually need to rebase on top of the latest multipatch, just rewrite the history without the file in question. It might be easier to use filter-branch; see https://help.github.com/articles/removing-sensitive-data-from-a-repository/

Add comma to end of each logged event for easy json loading.
Grab initial pipette states.
Grab initial microscope surface depth.
Event time is recorded as a float.
Removed titles from test pulse plots to free up space.
Catch situation where wave is returned as null.
@scseeman
Copy link

Hey Jed, tested out this PR today with 64-bit Igor. The only major problem I had was an error that was generated when ACQ4 would prompt MIES to interact with the MCCs
TypeError:'NoneType' object has no attribute 'getitem'
File "acq4\util\mies.py", line 65, in processUpdate
data = res[...,0] # dimension hack when return value suddenly changed

@JFPerkins
Copy link
Author

Looks like this was a problem with a return type of wave but an empty value. Fixed.

@scseeman
Copy link

That sounds right, it was also occurring when MIES gave the error:
TP Cycle Count is zero...returning null

@campagnola
Copy link

Tests look good; let's do this!

@campagnola campagnola merged commit 29eebc3 into aiephys:multipatch Mar 1, 2017
@campagnola
Copy link

Thanks Jed!

davoudian added a commit to davoudian/acq4 that referenced this pull request Mar 2, 2017
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.

3 participants