Skip to content

BF: remove incorrect code on removing eyetracker generated by builder#6526

Merged
TEParsons merged 1 commit intopsychopy:releasefrom
mh105:release-eyetracker-shutdown
Jun 6, 2024
Merged

BF: remove incorrect code on removing eyetracker generated by builder#6526
TEParsons merged 1 commit intopsychopy:releasefrom
mh105:release-eyetracker-shutdown

Conversation

@mh105
Copy link
Contributor

@mh105 mh105 commented Jun 6, 2024

This BF is a bit complicated to explain. Essentially I was trying to figure out why is EyeLink eyetrackers not properly shutdown at the end of an experiment, which led me to these lines of code generated by builder that look like they are meant to shut down an eyetracker.

However, if we look at the removeDevice() method, it relies on an exposed close() method. But for all ioHub devices, they are hosted on separate processes and accessed through ioHubDeviceView instances over a server client. Therefore, calling deviceManager.removeDevice('eyetracker') doesn't actually shut down an eyetracker process (or in fact, any ioHub devices stored in this dictionary). The ioHub eyetracker is only shutdown when the ioServer is stopped as a part of the core.quit() procedures. The terminateHubProcess() method calls _close() on each of the ioHub devices, including EyeTrackerDevice such as for the mousegaze.

I'm proposing to remove these confusing lines when generating script from builder, since they don't actually shut down eyetrackers. My primary problem of not closing EyeLink correctly is actually caused by something related: _close() isn't even implemented in the psychopy-sr-research package, which I'll PR to that repo to fix.

@peircej peircej requested a review from TEParsons June 6, 2024 07:57
@TEParsons
Copy link
Contributor

This seems fine to remove, though as you suggest in your PR to the sr research plugin I think it would be better to implement .close() on the base class so that calling close actually does close it.

@mh105
Copy link
Contributor Author

mh105 commented Jun 6, 2024

I think it would be better to implement .close() on the base class so that calling close actually does close it.

I actually tried that implementation first but realized that it conflicts with how ioHub server client likes to be closed. Basically if _close() is called before the core.quit() routines, there will be a runtime error because as a part of _close(), it removes the ioHubClient from the ioHubDeviceView instance, so by the time ioHubConnection.shutdown() tries to _close() devices, there are calls made to NoneType.

I get the sense that Sol just had a different way of handling closing devices in ioHub. It seems like ioHub devices are not supposed to be turned off individually directly (reflected by the private method _close()), but rather should be handled by making calls to the ioHubConnection instance. This design principle fundamentally differs from how PsychoPy is using DeviceManager that keeps a list of device instances in the .device attribute that can be individually turned on/off. Strangely, ioHubConnection only has a method to add a ioHubDeviceView, but no method to remove a ioHubDeviceView. So I'm not sure ioHub devices currently can be toggled after being added to the server client.

It might be better to let the ioHub server process handle the start/stop of ioHub devices and not try to .removeDevice() on them. If a generic .close() were to be exposed, a lot more changes in ioHub related codes need to be made. For instance, somehow by making calls to the ioHubDeviceView instance, the ioHubConnection needs to be modified. But since ioHubConnection is a manager of multiple ioHubDeviceView kept in ioHubConnection.devices, this could turn out to be difficult.

What I was suggesting in my PR to the sr-research plugin repo is to implement _close() (not close()) in the base EyeTrackerDevice class. But it might also differ from how ioHub likes to do it, since the base class shouldn't assume the availability of .setRecordingState() and .setConnectionState().

@TEParsons TEParsons merged commit 53fdf99 into psychopy:release Jun 6, 2024
@mh105 mh105 deleted the release-eyetracker-shutdown branch June 6, 2024 17:10
@peircej peircej added the 🐞 bug Issue describes a bug (crash or error) or undefined behavior. label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐞 bug Issue describes a bug (crash or error) or undefined behavior.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants