Skip to content

Fix some issues with connecting processors before starting acquisition#286

Merged
aacuevas merged 7 commits intoopen-ephys:developmentfrom
tne-lab:processorgraph-changes
Sep 5, 2019
Merged

Fix some issues with connecting processors before starting acquisition#286
aacuevas merged 7 commits intoopen-ephys:developmentfrom
tne-lab:processorgraph-changes

Conversation

@ethanbb
Copy link
Copy Markdown
Contributor

@ethanbb ethanbb commented Jan 22, 2019

I realized I had this branch from a while ago and wanted to do something with it. I understand if it's not worth it given the signal chain revamp that's going on, though.

Making all the right connections between processors in a signal chain with various mergers and splitters is a tricky problem. The code that does so was also confusingly structured, so I refactored it a bit and then fixed some bugs that I found (although there may still be some remaining):

  • Both paths of a splitter weren't being connected if it had to be revisited because of a merger immediately before it, due to the wasConnected flag. It's OK and actually necessary to explore splitters multiple times if there are multiple different streams coming into them.

  • In situations with multiple splitters in a row with no intervening processors, when exploring the second branch of the downstream splitter, the upstream one would erroneously be switched when moving forward from the downstream splitter's source because it was also in the splitter array. (Fixed by reintroducing activeSplitter.)

  • The source paths of mergers were never being switched, which resulted in incorrect backtracking when a splitter was encountered downstream, causing the second input to not get connected to some downstream nodes.

  • Less important: the wasConnected flag wasn't being used to skip looking downstream of non-splitter processors when possible, causing some redundancy.

@ethanbb
Copy link
Copy Markdown
Contributor Author

ethanbb commented Jun 10, 2019

I've just made a significant addition to this to deal with an edge case I just encountered: destination nodes being connected to multiple sources via mergers in the wrong order. This can result in the channel order of the destination's dataChannelArray not matching the order they actually arrive in the process function - definitely a buggy situation.

This happens when the order of the source tabs (which is the order they are visited in updateConnections) does not match the order in which they are merged leading into some destination. This is as easy as adding a merger and a sink, then adding the second input to the merger before the first. Since the tabs are always created top to bottom, the first tab corresponds to the second merger input and vice versa. The way updateConnections currently works, the ProcessorGraph will put the connections from the first tab before those from the second tab, even though on the Open Ephys metadata/Channel object side the merger topography determines the order. So you get a mismatch.

This solution collects all the connections to be made to each dest node before actually making the connections, sorts them according to the merger topography, and then connects them in the correct order.

@aacuevas aacuevas merged commit e2630b9 into open-ephys:development Sep 5, 2019
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