search

labscript-suite / blacs

𝗯𝗹𝗮𝗰𝘀 supervises the execution of experiments controlled by the 𝘭𝘢𝘣𝘴𝘤𝘳𝘪𝘱𝘵 𝘴𝘶𝘪𝘵𝘦. It manages experiment queuing and hardware-timed execution, and provides manual control over devices between shots.
http://labscriptsuite.org
Other
4 stars 48 forks source link

Fixed issue where blacs couldn't close with certain tabs #87

Closed zakv closed 2 years ago

zakv commented 3 years ago

Fixes #86 using Chris's suggestion there.

In short, the issue was that blacs failed to close when a PluginTab was used because that class doesn't have shutdown_workers() and finalise_close_tab() methods. This PR works around that by having blacs only call those methods on tabs that actually have them.

dihm commented 2 years ago

@zakv @chrisjbillington This looks fine to me, but I do have a question. Could we not just add these missing methods as blank passes in the PluginTab class as well? I can certainly imagine their functionality being useful at some point for plugins, and it may also help if we ever get around to merging the PluginTab and DeviceTab classes to have them be more similar. Thoughts?

I haven't thought super hard about it, so feel free to tell me that is a bad idea.

zakv commented 2 years ago

Sorry for the slow response, I'm a bit short on time at the moment. I do think your suggestion would be the most reasonable thing to do for the class methods. I'm not as sure what to do with the required attributes state and shutdown_workers_complete though. Maybe there is a reasonable way to handle those if they are added to the PluginTab class. I haven't had a chance to dig through the code again but from my comment here it seemed like they were necessary in the current implementation.

dihm commented 2 years ago

Fair enough. I should have read the bug report thread more carefully. Dealing with missing attributes as well is probably just complicated enough that I'd rather not touch it now, so this is the better method. I'll go ahead and merge it.