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

Improved resource cleanup #226

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

Improved resource cleanup #226

wants to merge 1 commit into from

Conversation

hackfin
Copy link

@hackfin hackfin commented Feb 5, 2023

Under Linux, re-opening the Plotter within a peristent context may not deallocate resources. Allow this via an explicit .close() method.

Hardware tests added:

  • Verify that we can re-open the device without I/O errors (in fact, ValueErrors)
  • Added umockdev_mode config flag to skip safe_write. Allows to run with umockdev-run.

Addendum: I am looking into some class restructuring such that the silhouette can be used by other front ends plus facilitation of derival such that no functionality is broken or compatibility is disrupted. Roughly, this would:

  • Split the Graphtec class into a hierarchy, avoiding product code comparison within member functions
  • Turn Graphtec into a function call that returns the properly detected class
  • Add some threading framework such that large tasks can be run in background (already established with Blender plug in)

Opinions?

For verification, more umockdev ioctl traces would be required and a reference test to be added. I currently have access to a cameo3 only. Appreciating any 'foreign' ioctl files for further I/O verification to start with (test_io.py).

Under Linux, re-opening the Plotter within a peristent context may
not deallocate resources. Allow this via an explicit .close() method.

Hardware tests added:

- Verify that we can re-open the device without I/O errors (in fact,
  ValueErrors)
- Added umockdev_mode config flag to skip safe_write. Allows to run with
  umockdev-run.
@t0b3
Copy link
Collaborator

t0b3 commented Feb 10, 2023

@hackfin thank you for this PR proposing further advancement of this plugin. Can you elaborate more on how to reproduce your issue re-opening the Plotter within a peristent context may not deallocate resources ? i'm curious to understand your userstory. Would this isssue also impact win or mac? thank you for sharing your thoughts...

@hackfin
Copy link
Author

hackfin commented Feb 11, 2023

Re-opening the plotter after closing the device (deletion) obviously frees not all USB resources (claims, probably). Upon reopening, a not so verbose 'ValueError' is raised, eventually. I don't know about Mac and Windows (yet).

The user story is that I'm driving this plugin from other software (like Blender) and reopening files requires some cleanup.

For reproduction with attached hardware, see test scenario: https://github.com/hackfin/inkscape-silhouette/blob/5f5942337b9aef1764bc138ab1b3962f29986ed2/test/hardware/test_io.py#L53

Since this PR broke the tests though, I've probably got some homework to do in order to supply passing umockdev tests for the github actions. Does anyone have input on how to get mocked hardware tested within the github pipeline?

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