From 90caeafd33b934603db9b72eff9646c41146fdca Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Sun, 13 Sep 2026 09:41:21 -0500 Subject: [PATCH 01/11] fix(device): release the USB claim when construction fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ChdkDevice opened the transport and then the session with nothing in between, so a session that raised left the interface claimed and the caller holding no object to close it. Retrying enumeration piled up claims on a port until someone unplugged the camera — and unplugging is the one repair that costs a body's position on a copy stand. Opening now gives the claim back if anything past it fails, and leaves nothing in the tracking set for _cleanup_all to find. The rollback catches BaseException rather than Exception, since a KeyboardInterrupt arriving between the claim and the session is the same leak. reconnect had the identical sequence written out a second time and the identical leak, worse for happening mid-session with the device already discarded from tracking. It calls _open now, so there is one way to claim an interface and one rollback to keep right. --- src/pychdk/device.py | 37 +++++++++++++++++++-------- tests/test_device.py | 59 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 11 deletions(-) diff --git a/src/pychdk/device.py b/src/pychdk/device.py index 314c2da..6e9df83 100644 --- a/src/pychdk/device.py +++ b/src/pychdk/device.py @@ -157,13 +157,31 @@ def __init__(self, device_info, _usb_device=None): self._open() def _open(self): + """Claim the interface and open a session, or claim nothing. + + The transport is claimed before the session can be opened, and + until the device is tracked there is nothing for the caller to + close: a constructor that raised here left the interface + claimed with no object to release it, so a host retrying + enumeration piled up claims on a port until the camera was + unplugged. Anything that fails past the claim gives it back. + """ self._transport.open() - self._session.open() - self._connected = True - _open_devices.add(self) - # Re-register so our cleanup runs before any pyusb finalizers - # that were registered during device creation (atexit is LIFO). - atexit.register(_cleanup_all) + try: + self._session.open() + self._connected = True + _open_devices.add(self) + # Re-register so our cleanup runs before any pyusb finalizers + # that were registered during device creation (atexit is LIFO). + atexit.register(_cleanup_all) + except BaseException: + self._connected = False + _open_devices.discard(self) + try: + self._transport.close() + except Exception: + pass + raise @property def is_connected(self): @@ -431,10 +449,9 @@ def reconnect(self, wait=2.0): except Exception: pass time.sleep(wait) - self._transport.open() - self._session.open() - self._connected = True - _open_devices.add(self) + # Same claim, same rollback: a reopen that fails mid-session + # leaks exactly as a failed construction did. + self._open() def close(self): """Close the connection to the camera.""" diff --git a/tests/test_device.py b/tests/test_device.py index a13053a..97c9e98 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -14,7 +14,12 @@ ScriptErrorType, ScriptMessage, ) -from pychdk.device import ChdkDevice, list_devices, DeviceInfo +from pychdk.device import ( + ChdkDevice, + list_devices, + DeviceInfo, + _open_devices, +) class TestListDevices: @@ -39,6 +44,58 @@ def test_empty_when_no_cameras(self, mock_find): assert list_devices() == [] +class TestConstructionIsExceptionSafe: + """A claim taken during construction must not outlive the failure.""" + + def _info(self): + return DeviceInfo( + vendor_id=0x04A9, product_id=0x1234, + bus_num=1, device_num=5, serial_num="ABC", + ) + + def test_a_failed_session_releases_the_transport(self): + tracked_before = len(_open_devices) + with patch("pychdk.device.PTPDevice") as MockTransport, \ + patch("pychdk.device.PTPSession") as MockSession, \ + patch("pychdk.device.ChdkPTP"): + MockSession.return_value.open.side_effect = RuntimeError( + "session refused", + ) + with pytest.raises(RuntimeError, match="session refused"): + ChdkDevice(self._info(), _usb_device=MagicMock()) + transport = MockTransport.return_value + transport.open.assert_called_once() + # One open, one close: the claim does not survive the raise. + transport.close.assert_called_once() + assert len(_open_devices) == tracked_before + + def test_a_failed_construction_tracks_nothing(self): + tracked_before = len(_open_devices) + with patch("pychdk.device.PTPDevice"), \ + patch("pychdk.device.PTPSession") as MockSession, \ + patch("pychdk.device.ChdkPTP"): + MockSession.return_value.open.side_effect = RuntimeError("nope") + with pytest.raises(RuntimeError): + ChdkDevice(self._info(), _usb_device=MagicMock()) + # Nothing for _cleanup_all to find, and no half-built device. + assert len(_open_devices) == tracked_before + + def test_a_failed_reconnect_also_releases_the_transport(self): + with patch("pychdk.device.PTPDevice") as MockTransport, \ + patch("pychdk.device.PTPSession") as MockSession, \ + patch("pychdk.device.ChdkPTP"): + dev = ChdkDevice(self._info(), _usb_device=MagicMock()) + transport = MockTransport.return_value + transport.close.reset_mock() + MockSession.return_value.open.side_effect = RuntimeError("gone") + with pytest.raises(RuntimeError, match="gone"): + dev.reconnect(wait=0) + # Closed once on the way down, once releasing the failed open. + assert transport.close.call_count == 2 + assert dev not in _open_devices + assert not dev.is_connected + + class TestChdkDevice: def _make_device(self): info = DeviceInfo( From 3e13c8e61c37be52320262e01642aa110fd6c5c5 Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Sun, 13 Sep 2026 09:43:11 -0500 Subject: [PATCH 02/11] feat(capture): report how many chunks a capture arrived in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remote_capture_get_data assembled chunks and returned only the bytes, so a host had no way to say whether a still came over in one piece or forty. That is one of the things the bench exists to find out, and the assembly loop is the only place that knows it. The count is left on the protocol object as last_capture_chunks and read through a property of the same name on ChdkDevice, rather than returned. shoot() already returns bytes or None depending on four of its arguments, and MultiCam.shoot promises a list of pictures, one per camera — widening either return would change what every existing caller receives to carry a number most of them will not read. Per device is also the right granularity: after a MultiCam shot each camera's figure is on its own entry in MultiCam.cameras. It resets when a capture begins and rises as chunks land, so a capture that failed half way still says how far it got, which is more use at a bench than a number that only appears on success. --- src/pychdk/chdk.py | 19 ++++++++++++++++++ src/pychdk/device.py | 12 ++++++++++++ tests/test_chdk.py | 46 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_device.py | 38 ++++++++++++++++++++++++++++++++++++ 4 files changed, 115 insertions(+) diff --git a/src/pychdk/chdk.py b/src/pychdk/chdk.py index a6ae810..ec09142 100644 --- a/src/pychdk/chdk.py +++ b/src/pychdk/chdk.py @@ -156,6 +156,20 @@ class ChdkPTP: def __init__(self, session): self._session = session + self._last_capture_chunks = 0 + + @property + def last_capture_chunks(self): + """How many chunks the last remote capture arrived in. + + Reset when a capture starts and incremented as each chunk + lands, so it is readable — and still true — after a capture + that failed part way through. A still that arrives in one + chunk and one that arrives in forty say different things + about the wire, and there is one bench session to find out + which of them a real camera does. + """ + return self._last_capture_chunks def get_version(self): """Get CHDK PTP protocol version. @@ -404,6 +418,9 @@ def remote_capture_get_data(self, format_flag): Args: format_flag: Which format to download (JPEG=1, RAW=2, DNG_HDR=4). + The chunk count is left in last_capture_chunks rather than + returned, so the signature callers depend on is unchanged. + Returns: Image data as bytes. @@ -412,8 +429,10 @@ def remote_capture_get_data(self, format_flag): """ image = bytearray() cursor = 0 + self._last_capture_chunks = 0 for _ in range(MAX_CAPTURE_CHUNKS): chunk, more, position = self.remote_capture_get_chunk(format_flag) + self._last_capture_chunks += 1 if position >= 0: cursor = position end = cursor + len(chunk) diff --git a/src/pychdk/device.py b/src/pychdk/device.py index 6e9df83..0e041db 100644 --- a/src/pychdk/device.py +++ b/src/pychdk/device.py @@ -187,6 +187,18 @@ def _open(self): def is_connected(self): return self._connected + @property + def last_capture_chunks(self): + """How many chunks the last streamed capture arrived in. + + Read after shoot(stream=True) rather than returned by it: the + return value is the picture, and MultiCam.shoot promises a list + of those, one per camera. The count lives per device, so after + a MultiCam shot each camera's own figure is on its entry in + MultiCam.cameras. + """ + return self._chdk.last_capture_chunks + def switch_mode(self, mode): """Switch camera to 'record' or 'play' mode. diff --git a/tests/test_chdk.py b/tests/test_chdk.py index b0346ce..f65ed52 100644 --- a/tests/test_chdk.py +++ b/tests/test_chdk.py @@ -379,6 +379,52 @@ def test_camera_that_never_clears_more_raises(self): chdk.remote_capture_get_data(1) +class TestCaptureChunkCount: + """How many chunks a still arrived in is a bench observation.""" + + def _make_chdk(self): + mock_session = MagicMock() + return ChdkPTP(mock_session), mock_session + + def test_it_starts_at_zero(self): + chdk, _ = self._make_chdk() + assert chdk.last_capture_chunks == 0 + + def test_it_counts_the_chunks_of_a_capture(self): + chdk, session = self._make_chdk() + session.transaction.side_effect = [ + ([4, 1, 0xFFFFFFFF], b"AAAA"), + ([4, 1, 0xFFFFFFFF], b"BBBB"), + ([4, 0, 0xFFFFFFFF], b"CCCC"), + ] + chdk.remote_capture_get_data(1) + assert chdk.last_capture_chunks == 3 + + def test_a_later_capture_does_not_inherit_the_count(self): + chdk, session = self._make_chdk() + session.transaction.side_effect = [ + ([4, 1, 0xFFFFFFFF], b"AAAA"), + ([4, 0, 0xFFFFFFFF], b"BBBB"), + ] + chdk.remote_capture_get_data(1) + assert chdk.last_capture_chunks == 2 + session.transaction.side_effect = [([4, 0, 0xFFFFFFFF], b"ZZZZ")] + chdk.remote_capture_get_data(1) + assert chdk.last_capture_chunks == 1 + + def test_the_count_survives_a_capture_that_failed_part_way(self): + chdk, session = self._make_chdk() + session.transaction.side_effect = [ + ([4, 1, 0xFFFFFFFF], b"AAAA"), + ([4, 1, 0xFFFFFFFF], b"BBBB"), + RuntimeError("cable"), + ] + with pytest.raises(RuntimeError, match="cable"): + chdk.remote_capture_get_data(1) + # Two arrived before it broke, which is worth knowing. + assert chdk.last_capture_chunks == 2 + + class TestRemoteCaptureGetChunk: def _make_chdk(self): mock_session = MagicMock() diff --git a/tests/test_device.py b/tests/test_device.py index 97c9e98..c84036a 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -44,6 +44,44 @@ def test_empty_when_no_cameras(self, mock_find): assert list_devices() == [] +class TestCaptureChunkCountThroughShoot: + """Callers use shoot(), so the count has to be reachable from there.""" + + def _device_with_a_real_protocol(self): + info = DeviceInfo( + vendor_id=0x04A9, product_id=0x1234, + bus_num=1, device_num=5, serial_num="ABC", + ) + mock_session = MagicMock() + with patch("pychdk.device.PTPDevice"), \ + patch("pychdk.device.PTPSession", return_value=mock_session): + dev = ChdkDevice(info, _usb_device=MagicMock()) + return dev, mock_session + + def test_shoot_reports_how_many_chunks_arrived(self): + dev, session = self._device_with_a_real_protocol() + session.transaction.side_effect = [ + ([7, 0], b""), # execute_script, id 7 + ([0x01], b""), # ready, JPEG + ([4, 1, 0xFFFFFFFF], b"AAAA"), # chunk 1 + ([4, 0, 0xFFFFFFFF], b"BBBB"), # chunk 2, the last + ([0], b""), # drain: nothing waiting + ] + assert dev.shoot(stream=True) == b"AAAABBBB" + assert dev.last_capture_chunks == 2 + + def test_a_one_chunk_still_reports_one(self): + dev, session = self._device_with_a_real_protocol() + session.transaction.side_effect = [ + ([7, 0], b""), + ([0x01], b""), + ([4, 0, 0xFFFFFFFF], b"JPEG"), + ([0], b""), + ] + assert dev.shoot(stream=True) == b"JPEG" + assert dev.last_capture_chunks == 1 + + class TestConstructionIsExceptionSafe: """A claim taken during construction must not outlive the failure.""" From 01f2f8893d6df9dbcb1aef992c7665d9bcb0ab7d Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Sun, 13 Sep 2026 09:43:57 -0500 Subject: [PATCH 03/11] chore(release): 0.1.2 Cuts the release carrying the USB claim released on a failed construction and the chunk count a streamed capture arrived in, both found reviewing the backend that pins v0.1.1. --- pyproject.toml | 2 +- src/pychdk/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 6130ffd..6362a34 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "pychdk" -version = "0.1.1" +version = "0.1.2" description = "Pure Python CHDK PTP camera control" requires-python = ">=3.11" dependencies = [ diff --git a/src/pychdk/__init__.py b/src/pychdk/__init__.py index 2e758c2..ad0d715 100644 --- a/src/pychdk/__init__.py +++ b/src/pychdk/__init__.py @@ -1,7 +1,7 @@ """Pure Python CHDK PTP camera control.""" import importlib -__version__ = "0.1.1" +__version__ = "0.1.2" __all__ = [ "ChdkDevice", "list_devices", "DeviceInfo", "install_signal_handlers", From be21b9b7f0ea495a4c45d6ed102ccbe20d279ea9 Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Sun, 13 Sep 2026 09:50:00 -0500 Subject: [PATCH 04/11] fix(usb): release a claimed interface however far opening got PTPDevice.open claims the interface and then goes looking for endpoints, but only sets _is_open once everything has succeeded, and close() keyed on that flag. A camera whose endpoints could not be found therefore held a claim the transport did not believe it had: close() returned immediately, and calling it explicitly changed nothing. One claim, no release, until someone unplugged the camera. Ownership is now tracked from the moment the claim succeeds, and close() gives back whatever is actually held rather than whatever opening finished. A device that never got as far as claiming still closes to nothing, and a second close does not release twice. The device-level rollback had the matching gap: transport.open sat outside the handler, so a transport that failed this way was never closed by its caller either. It is inside now, which is what makes the rollback a release rather than a no-op. Both paths into it, construction and reconnect, go through the same code. --- src/pychdk/device.py | 9 +++--- src/pychdk/usb_transport.py | 27 ++++++++++++++---- tests/test_device.py | 15 ++++++++++ tests/test_usb_transport.py | 55 +++++++++++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 10 deletions(-) diff --git a/src/pychdk/device.py b/src/pychdk/device.py index 0e041db..66a2ee2 100644 --- a/src/pychdk/device.py +++ b/src/pychdk/device.py @@ -159,15 +159,16 @@ def __init__(self, device_info, _usb_device=None): def _open(self): """Claim the interface and open a session, or claim nothing. - The transport is claimed before the session can be opened, and - until the device is tracked there is nothing for the caller to + Until the device is tracked there is nothing for the caller to close: a constructor that raised here left the interface claimed with no object to release it, so a host retrying enumeration piled up claims on a port until the camera was - unplugged. Anything that fails past the claim gives it back. + unplugged. Anything that fails past the claim gives it back — + including a failure inside the transport's own open, which can + hold a claim and still raise. """ - self._transport.open() try: + self._transport.open() self._session.open() self._connected = True _open_devices.add(self) diff --git a/src/pychdk/usb_transport.py b/src/pychdk/usb_transport.py index c91d76b..e54b7f6 100644 --- a/src/pychdk/usb_transport.py +++ b/src/pychdk/usb_transport.py @@ -90,6 +90,11 @@ def __init__(self, usb_device): self._ep_out = None self._ep_int = None self._intf_num = None + # The interface we actually hold, set the moment the claim + # succeeds rather than when opening finishes. Everything + # between the claim and the end of open() can fail, and what + # is held has to be releasable in between. + self._claimed_intf = None self._is_open = False @property @@ -154,6 +159,7 @@ def open(self): pass usb.util.claim_interface(self._dev, self._intf_num) + self._claimed_intf = self._intf_num # Find endpoints intf = cfg[(self._intf_num, 0)] @@ -181,13 +187,22 @@ def open(self): self._dev._finalize_called = True def close(self): - """Release the USB interface and dispose of device resources.""" - if not self._is_open: + """Release whatever is held, however far open() got. + + Opening claims the interface and then goes looking for + endpoints, so a device can own a claim while still failing to + open. Keying this on _is_open made close() a no-op in exactly + that case, and the claim was then held until the camera was + unplugged. It keys on the claim instead. + """ + if self._claimed_intf is None and not self._is_open: return - try: - usb.util.release_interface(self._dev, self._intf_num) - except usb.core.USBError: - pass + if self._claimed_intf is not None: + try: + usb.util.release_interface(self._dev, self._claimed_intf) + except usb.core.USBError: + pass + self._claimed_intf = None try: usb.util.dispose_resources(self._dev) except usb.core.USBError: diff --git a/tests/test_device.py b/tests/test_device.py index c84036a..2afe33b 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -107,6 +107,21 @@ def test_a_failed_session_releases_the_transport(self): transport.close.assert_called_once() assert len(_open_devices) == tracked_before + def test_a_failed_transport_open_is_also_released(self): + tracked_before = len(_open_devices) + with patch("pychdk.device.PTPDevice") as MockTransport, \ + patch("pychdk.device.PTPSession"), \ + patch("pychdk.device.ChdkPTP"): + # A transport that claims the interface and then fails + # finding endpoints raises out of open() itself. + MockTransport.return_value.open.side_effect = RuntimeError( + "Could not find bulk endpoints on PTP device", + ) + with pytest.raises(RuntimeError, match="bulk endpoints"): + ChdkDevice(self._info(), _usb_device=MagicMock()) + MockTransport.return_value.close.assert_called_once() + assert len(_open_devices) == tracked_before + def test_a_failed_construction_tracks_nothing(self): tracked_before = len(_open_devices) with patch("pychdk.device.PTPDevice"), \ diff --git a/tests/test_usb_transport.py b/tests/test_usb_transport.py index 022e9eb..d3725c3 100644 --- a/tests/test_usb_transport.py +++ b/tests/test_usb_transport.py @@ -55,6 +55,28 @@ def _make_mock_usb_device(vendor_id=0x04A9, product_id=0xABCD, return dev +def _make_mock_usb_device_without_bulk_endpoints(): + """A PTP interface that claims fine and then has nothing to talk on.""" + ep_int = _make_mock_endpoint(0x83) + ep_int.bmAttributes = 0x03 # interrupt only + + interface = MagicMock() + interface.bInterfaceClass = 6 + interface.bInterfaceSubClass = 1 + interface.bInterfaceProtocol = 1 + interface.bInterfaceNumber = 0 + interface.__iter__ = lambda self: iter([ep_int]) + + config = MagicMock() + config.__iter__ = lambda self: iter([interface]) + config.__getitem__ = lambda self, key: interface + + dev = MagicMock() + dev.__iter__ = lambda self: iter([config]) + dev.__getitem__ = lambda self, i: config + return dev + + class TestFindPTPDevices: @patch("pychdk.usb_transport.usb.core.find") def test_finds_canon_ptp_devices(self, mock_find): @@ -84,6 +106,39 @@ def test_close_releases_interface(self): ptp.close() assert not ptp._is_open + @patch("pychdk.usb_transport.usb.util.release_interface") + @patch("pychdk.usb_transport.usb.util.claim_interface") + def test_a_claim_is_released_when_endpoint_discovery_fails( + self, mock_claim, mock_release, + ): + mock_dev = _make_mock_usb_device_without_bulk_endpoints() + ptp = PTPDevice(mock_dev) + with pytest.raises(RuntimeError, match="bulk endpoints"): + ptp.open() + # The interface was taken before discovery failed. + mock_claim.assert_called_once_with(mock_dev, 0) + ptp.close() + # So closing has to give it back, however far open() got. + mock_release.assert_called_once_with(mock_dev, 0) + + @patch("pychdk.usb_transport.usb.util.release_interface") + def test_closing_a_device_that_never_opened_releases_nothing( + self, mock_release, + ): + ptp = PTPDevice(_make_mock_usb_device()) + ptp.close() + mock_release.assert_not_called() + + @patch("pychdk.usb_transport.usb.util.release_interface") + @patch("pychdk.usb_transport.usb.util.claim_interface") + def test_a_claim_is_given_back_only_once(self, mock_claim, mock_release): + mock_dev = _make_mock_usb_device() + ptp = PTPDevice(mock_dev) + ptp.open() + ptp.close() + ptp.close() + mock_release.assert_called_once_with(mock_dev, 0) + def test_bulk_write(self): mock_dev = _make_mock_usb_device() ptp = PTPDevice(mock_dev) From 03345e10f4de894fe025627f682cd572ff229b79 Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Sun, 13 Sep 2026 09:51:28 -0500 Subject: [PATCH 05/11] fix(capture): zero the chunk count at the attempt, not at the download The count reset inside remote_capture_get_data, which only runs once a capture is ready to download. So a capture refused outright, or one that never became ready, or one that timed out, left the previous capture's number standing. After a two-chunk still, a capture where no chunk arrived at all still said two. That is the failure the count was added to prevent, made worse: a number that is simply missing gets investigated, and a number that is wrong gets believed. At a bench it would have sent someone looking for a transfer problem in a capture that never transferred anything. _shoot_streaming zeroes it on entry, before the DNG refusal, so every streamed attempt starts from nothing and zero means no chunk arrived. remote_capture_get_data keeps its own reset for a caller using it directly. Both docstrings now say that a reader in another thread sees a partial count while a capture is in flight, which matters because MultiCam shoots on a pool. --- src/pychdk/chdk.py | 27 ++++++++++++++++----- src/pychdk/device.py | 12 ++++++++++ tests/test_device.py | 57 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 6 deletions(-) diff --git a/src/pychdk/chdk.py b/src/pychdk/chdk.py index ec09142..9d46d45 100644 --- a/src/pychdk/chdk.py +++ b/src/pychdk/chdk.py @@ -162,15 +162,30 @@ def __init__(self, session): def last_capture_chunks(self): """How many chunks the last remote capture arrived in. - Reset when a capture starts and incremented as each chunk - lands, so it is readable — and still true — after a capture - that failed part way through. A still that arrives in one - chunk and one that arrives in forty say different things - about the wire, and there is one bench session to find out - which of them a real camera does. + Zeroed when a capture is attempted and incremented as each + chunk lands, so it is readable — and still true — after a + capture that failed part way through, and reads zero after one + that never got a chunk at all. A still that arrives in one + chunk and one that arrives in forty say different things about + the wire, and there is one bench session to find out which of + them a real camera does. + + Read it from the thread that ran the capture, or after that + thread has finished: a reader watching from elsewhere while a + capture is in flight sees a partial count, since it rises as + the chunks arrive. """ return self._last_capture_chunks + def reset_capture_chunks(self): + """Zero the chunk count at the start of a capture attempt. + + Called by the caller that begins a capture, because a capture + can fail before any download is attempted and must not go on + reporting the previous capture's chunks. + """ + self._last_capture_chunks = 0 + def get_version(self): """Get CHDK PTP protocol version. diff --git a/src/pychdk/device.py b/src/pychdk/device.py index 66a2ee2..506a838 100644 --- a/src/pychdk/device.py +++ b/src/pychdk/device.py @@ -197,6 +197,11 @@ def last_capture_chunks(self): of those, one per camera. The count lives per device, so after a MultiCam shot each camera's own figure is on its entry in MultiCam.cameras. + + Zero means no chunk arrived, not that no capture was tried. + Read it from the thread that took the shot, or once that thread + has finished: MultiCam shoots on a pool, and a reader looking + at another worker's device mid-capture sees a partial count. """ return self._chdk.last_capture_chunks @@ -270,6 +275,11 @@ def shoot(self, shutter_speed=None, market_iso=None, dng=False, def _shoot_streaming(self, setup_parts, dng): """Capture using remote capture (PTP commands 13/14). + The chunk count is zeroed here, at the attempt, rather than + where the download begins: a capture refused, or one that never + becomes ready, would otherwise keep reporting the chunks of the + capture before it. + Setup and shutter go out as one script, because a second script kills the first unless NOKILL is set ("if script is running return error instead of killing", core/ptp.h) — so a separate @@ -297,6 +307,8 @@ def _shoot_streaming(self, setup_parts, dng): file. This method downloads one format, so it cannot, and _shoot_standard does not request a DNG either. """ + self._chdk.reset_capture_chunks() + if dng: raise NotImplementedError( "DNG capture is not implemented. Streaming would need the " diff --git a/tests/test_device.py b/tests/test_device.py index 2afe33b..695e8f5 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -70,6 +70,63 @@ def test_shoot_reports_how_many_chunks_arrived(self): assert dev.shoot(stream=True) == b"AAAABBBB" assert dev.last_capture_chunks == 2 + def test_a_capture_that_never_downloads_reports_nothing(self): + dev, session = self._device_with_a_real_protocol() + session.transaction.side_effect = [ + ([7, 0], b""), # first capture: two chunks + ([0x01], b""), + ([4, 1, 0xFFFFFFFF], b"AAAA"), + ([4, 0, 0xFFFFFFFF], b"BBBB"), + ([0], b""), + ] + assert dev.shoot(stream=True) == b"AAAABBBB" + assert dev.last_capture_chunks == 2 + + session.transaction.side_effect = [ + ([8, 0], b""), # second: script starts + ([0], b""), # nothing ready + ([0], b""), # and the script has ended + ] + with pytest.raises(RuntimeError, match="without producing a capture"): + dev.shoot(stream=True) + # No chunk arrived, so reporting two would be a lie a bench + # reader would believe. + assert dev.last_capture_chunks == 0 + + def test_a_refused_capture_reports_nothing(self): + dev, session = self._device_with_a_real_protocol() + session.transaction.side_effect = [ + ([7, 0], b""), + ([0x01], b""), + ([4, 0, 0xFFFFFFFF], b"JPEG"), + ([0], b""), + ] + assert dev.shoot(stream=True) == b"JPEG" + assert dev.last_capture_chunks == 1 + + with pytest.raises(NotImplementedError): + dev.shoot(dng=True, stream=True) + assert dev.last_capture_chunks == 0 + + def test_a_timed_out_capture_reports_nothing(self, monkeypatch): + dev, session = self._device_with_a_real_protocol() + session.transaction.side_effect = [ + ([7, 0], b""), + ([0x01], b""), + ([4, 0, 0xFFFFFFFF], b"JPEG"), + ([0], b""), + ] + assert dev.shoot(stream=True) == b"JPEG" + assert dev.last_capture_chunks == 1 + + # A camera that stays busy and never becomes ready. + session.transaction.side_effect = None + session.transaction.return_value = ([0], b"") + monkeypatch.setattr("pychdk.device.CAPTURE_INIT_GRACE", 0.0) + with pytest.raises(RuntimeError): + dev.shoot(stream=True) + assert dev.last_capture_chunks == 0 + def test_a_one_chunk_still_reports_one(self): dev, session = self._device_with_a_real_protocol() session.transaction.side_effect = [ From b4c2cd9a99b7bf0767c90047f4eda7d89d066d5c Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Sun, 13 Sep 2026 09:58:40 -0500 Subject: [PATCH 06/11] test(capture): name the capture failure test after what it checks, and test the deadline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_a_timed_out_capture_reports_nothing never reached a deadline. Its status response said no script was running, so the second capture ended on the script-ended path after three transactions and no elapsed time, raising RuntimeError where a timeout raises TimeoutError. It proved the counter resets on a failed capture, which is worth having, but its name was evidence for a path it never took — and its comment said the camera stayed busy while its mock said the opposite. Renamed for the path it actually exercises, and the deadline now has a test of its own. The clock is driven rather than waited on: a fake stands in for the time module inside the device, walking a scripted monotonic sequence and counting sleeps without taking them, so thirty seconds of deadline cost nothing. It asserts TimeoutError specifically, which the old setup could not have raised, and that the loop went round before expiring rather than falling straight out of it. Also records what close() does about concurrent callers, read out of pyusb rather than assumed: _ResourceManager holds a threading.RLock, managed_claim_interface and managed_release_interface are both @synchronized against it, and release calls the backend only for an interface still in its claimed set, removing it in a finally. So concurrent closes serialise and a second release does nothing, and we rely on that rather than adding a lock here. The docstring cites the file and both functions so the next reader can confirm it in a minute. --- src/pychdk/device.py | 7 +++- src/pychdk/usb_transport.py | 14 ++++++++ tests/test_device.py | 69 +++++++++++++++++++++++++++++++++++-- 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/src/pychdk/device.py b/src/pychdk/device.py index 506a838..c6fb54c 100644 --- a/src/pychdk/device.py +++ b/src/pychdk/device.py @@ -479,7 +479,12 @@ def reconnect(self, wait=2.0): self._open() def close(self): - """Close the connection to the camera.""" + """Close the connection to the camera. + + Safe to call more than once, and safe against a concurrent + closer on the same device. PTPDevice.close says why, and where + in pyusb to check it. + """ self._connected = False _open_devices.discard(self) try: diff --git a/src/pychdk/usb_transport.py b/src/pychdk/usb_transport.py index e54b7f6..3d577c0 100644 --- a/src/pychdk/usb_transport.py +++ b/src/pychdk/usb_transport.py @@ -194,6 +194,20 @@ def close(self): open. Keying this on _is_open made close() a no-op in exactly that case, and the claim was then held until the camera was unplugged. It keys on the claim instead. + + Repeated closes are safe in sequence, and two threads calling + close at once are safe as well, so there is no lock of our own + here. pyusb serialises claiming and releasing on a reentrant + lock it holds itself, and releases only an interface it still + records as claimed: in usb/core.py, _ResourceManager keeps a + threading.RLock, managed_claim_interface and + managed_release_interface are both decorated @synchronized + against it, and the release calls the backend only when the + interface is in its claimed set, removing it in a finally — so + a second release for the same interface does nothing, and a + repeated claim does not double-claim for the same reason. + Checked against the installed pyusb (1.3.1) rather than + assumed; worth a re-read if that version moves. """ if self._claimed_intf is None and not self._is_open: return diff --git a/tests/test_device.py b/tests/test_device.py index 695e8f5..dd8e56d 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -8,6 +8,7 @@ import pychdk from pychdk import device from pychdk.chdk import ( + ChdkCommand, MessageType, REMOTE_CAP_NOTSET, ScriptDataType, @@ -44,6 +45,27 @@ def test_empty_when_no_cameras(self, mock_find): assert list_devices() == [] +class _FakeClock: + """Stands in for the time module inside pychdk.device. + + monotonic() walks a scripted sequence and then holds its last + value, so a thirty-second deadline can be reached in no time at + all; sleep() records what was asked for without waiting. + """ + + def __init__(self, readings): + self._readings = list(readings) + self.slept = 0.0 + + def monotonic(self): + if len(self._readings) > 1: + return self._readings.pop(0) + return self._readings[0] + + def sleep(self, seconds): + self.slept += seconds + + class TestCaptureChunkCountThroughShoot: """Callers use shoot(), so the count has to be reachable from there.""" @@ -108,7 +130,9 @@ def test_a_refused_capture_reports_nothing(self): dev.shoot(dng=True, stream=True) assert dev.last_capture_chunks == 0 - def test_a_timed_out_capture_reports_nothing(self, monkeypatch): + def test_a_second_capture_that_ends_without_data_reports_nothing( + self, monkeypatch, + ): dev, session = self._device_with_a_real_protocol() session.transaction.side_effect = [ ([7, 0], b""), @@ -119,13 +143,52 @@ def test_a_timed_out_capture_reports_nothing(self, monkeypatch): assert dev.shoot(stream=True) == b"JPEG" assert dev.last_capture_chunks == 1 - # A camera that stays busy and never becomes ready. + # Nothing ready and the script already finished: this ends on + # the script-ended path, not at the deadline. See + # test_a_capture_that_runs_out_its_deadline_reports_nothing. session.transaction.side_effect = None session.transaction.return_value = ([0], b"") monkeypatch.setattr("pychdk.device.CAPTURE_INIT_GRACE", 0.0) - with pytest.raises(RuntimeError): + with pytest.raises(RuntimeError, match="without producing a capture"): + dev.shoot(stream=True) + assert dev.last_capture_chunks == 0 + + def test_a_capture_that_runs_out_its_deadline_reports_nothing( + self, monkeypatch, + ): + dev, session = self._device_with_a_real_protocol() + session.transaction.side_effect = [ + ([7, 0], b""), + ([0x01], b""), + ([4, 0, 0xFFFFFFFF], b"JPEG"), + ([0], b""), + ] + assert dev.shoot(stream=True) == b"JPEG" + assert dev.last_capture_chunks == 1 + + # A camera that stays busy and never becomes ready. The clock is + # driven rather than waited on: the deadline is thirty seconds + # and the suite must not spend them. + def respond(operation, params=None, **kwargs): + command = params[0] + if command == ChdkCommand.EXECUTE_SCRIPT: + return ([8, 0], b"") + if command == ChdkCommand.REMOTE_CAPTURE_IS_READY: + return ([0], b"") # never ready + if command == ChdkCommand.SCRIPT_STATUS: + return ([0b01], b"") # still running, nothing to say + return ([0], b"") + + session.transaction.side_effect = respond + clock = _FakeClock([0.0, 0.0, 0.0, 0.0, 999.0]) + monkeypatch.setattr("pychdk.device.time", clock) + + # TimeoutError, not RuntimeError: only the deadline raises this. + with pytest.raises(TimeoutError, match="did not complete"): dev.shoot(stream=True) assert dev.last_capture_chunks == 0 + # It really went round the loop rather than falling straight out. + assert clock.slept > 0 def test_a_one_chunk_still_reports_one(self): dev, session = self._device_with_a_real_protocol() From 6e06df8af186495bf43bd10de3aee396a123291c Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Sun, 13 Sep 2026 13:09:15 -0500 Subject: [PATCH 07/11] fix(usb): release the claim when opening fails inside a with block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PTPDevice.__enter__ called open() and returned self, and Python does not call __exit__ when __enter__ raises. So a transport that claimed the interface and then failed finding endpoints held it with nothing outside the block able to give it back — the same leak as a failed construction, in the one path neither that fix nor the reconnect fix reached. __enter__ closes before re-raising. close() already releases whatever is actually held however far open() got, so this is the rollback the other two callers have, in the third place that needed it. Three call sites and one fault between them says the shape is wrong rather than the callers: open() can take a claim and then raise, and each caller is separately responsible for noticing. The fix that ends it is to give open() the guarantee instead — claim released before it propagates, so that a raise means nothing is held — which would make all three rollbacks redundant rather than mandatory. Not at the end of a release, but it is the change that stops a fourth. --- src/pychdk/usb_transport.py | 18 +++++++++++++++++- tests/test_usb_transport.py | 16 ++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/pychdk/usb_transport.py b/src/pychdk/usb_transport.py index 3d577c0..2e3e460 100644 --- a/src/pychdk/usb_transport.py +++ b/src/pychdk/usb_transport.py @@ -234,7 +234,23 @@ def bulk_read(self, size=None, timeout=DEFAULT_TIMEOUT): return bytes(self._ep_in.read(size, timeout=timeout)) def __enter__(self): - self.open() + """Open on the way in, giving the claim back if opening fails. + + Python does not call __exit__ when __enter__ raises, so nothing + outside the with block can release the interface: the rollback + has to be here. This is the third place the same fault turned + up — construction, reconnect, and now here — because open() + can claim and then raise, and every caller is left to remember + that separately. + """ + try: + self.open() + except BaseException: + try: + self.close() + except Exception: + pass + raise return self def __exit__(self, *args): diff --git a/tests/test_usb_transport.py b/tests/test_usb_transport.py index d3725c3..fe02dae 100644 --- a/tests/test_usb_transport.py +++ b/tests/test_usb_transport.py @@ -139,6 +139,22 @@ def test_a_claim_is_given_back_only_once(self, mock_claim, mock_release): ptp.close() mock_release.assert_called_once_with(mock_dev, 0) + @patch("pychdk.usb_transport.usb.util.release_interface") + @patch("pychdk.usb_transport.usb.util.claim_interface") + def test_a_failed_open_in_a_with_block_claims_nothing( + self, mock_claim, mock_release, + ): + mock_dev = _make_mock_usb_device_without_bulk_endpoints() + ptp = PTPDevice(mock_dev) + # __exit__ never runs when __enter__ raises, so nothing outside + # the block can give the interface back. + with pytest.raises(RuntimeError, match="bulk endpoints"): + with ptp: + pass + mock_claim.assert_called_once_with(mock_dev, 0) + mock_release.assert_called_once_with(mock_dev, 0) + assert ptp._claimed_intf is None + def test_bulk_write(self): mock_dev = _make_mock_usb_device() ptp = PTPDevice(mock_dev) From 1e8c06012acb94643309454338ccd11faa01a1f3 Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Sun, 13 Sep 2026 13:11:40 -0500 Subject: [PATCH 08/11] fix(usb): make a failed open hold nothing, rather than asking callers to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Releasing a claim that open() took and then lost was a convention, and three call sites independently failed to honour it — construction, reconnect, and the context manager — each found separately, each fixed separately. That is what a convention does once there is more than one caller: the next one added will not know about it either. open() now carries the guarantee itself. Everything after the claim runs inside a handler that gives the interface back before the exception propagates, so "open raised" means "nothing is held" as a property of the function that does the claiming, not as something four callers have to remember. The existing rollbacks stay: they are harmless now rather than load-bearing, and taking them out is its own decision. PTPSession.__enter__ has the same shape and deliberately no rollback, which is now written down where it sits rather than left to be rediscovered: a failed session open holds no operating-system resource, since it raises when the camera refused the session. The one asymmetry is noted there too — a command that reached the camera followed by a failed response read could leave a camera-side session we would not close. --- src/pychdk/ptp.py | 7 ++++ src/pychdk/usb_transport.py | 70 ++++++++++++++++++++++++------------- tests/test_usb_transport.py | 15 ++++++++ 3 files changed, 67 insertions(+), 25 deletions(-) diff --git a/src/pychdk/ptp.py b/src/pychdk/ptp.py index c0389fc..81ff3a8 100644 --- a/src/pychdk/ptp.py +++ b/src/pychdk/ptp.py @@ -213,6 +213,13 @@ def _receive_data(self, tx_id): return data[:total_length - CONTAINER_HEADER_SIZE] def __enter__(self): + # No rollback here, deliberately: unlike PTPDevice.open, a + # failed session open holds no operating-system resource. It + # raises only when the camera refused OPEN_SESSION, so there is + # no session to close and nothing claimed. The one asymmetry, + # should it ever matter: if the command reached the camera and + # the response read failed, the camera may hold a session we + # would not close, because close() returns early on _is_open. self.open() return self diff --git a/src/pychdk/usb_transport.py b/src/pychdk/usb_transport.py index 2e3e460..0233f6b 100644 --- a/src/pychdk/usb_transport.py +++ b/src/pychdk/usb_transport.py @@ -121,7 +121,15 @@ def serial_number(self): return None def open(self): - """Open the PTP device — claim interface and find endpoints.""" + """Open the PTP device — claim interface and find endpoints. + + Either this returns with the interface claimed, or it raises + having claimed nothing. That guarantee lives here rather than + in each caller because it was a convention before, and three + call sites independently failed to honour it: a claim taken + and then lost to an exception is held until the camera is + unplugged, and every caller had to remember that separately. + """ if self._is_open: return @@ -161,30 +169,42 @@ def open(self): usb.util.claim_interface(self._dev, self._intf_num) self._claimed_intf = self._intf_num - # Find endpoints - intf = cfg[(self._intf_num, 0)] - for ep in intf: - attr = ep.bmAttributes & 0x03 # transfer type mask - direction = ep.bEndpointAddress & 0x80 # direction mask - if attr == usb.util.ENDPOINT_TYPE_BULK: - if direction == EP_DIR_IN: - self._ep_in = ep - else: - self._ep_out = ep - elif attr == usb.util.ENDPOINT_TYPE_INTR: - if direction == EP_DIR_IN: - self._ep_int = ep - - if self._ep_in is None or self._ep_out is None: - raise RuntimeError("Could not find bulk endpoints on PTP device") - - self._is_open = True - - # Disable pyusb's weakref.finalize cleanup for this Device. - # During Python shutdown, pyusb's finalizer can call libusb_open - # after the libusb context has been freed, causing a SIGSEGV. - # We handle all USB cleanup ourselves in close(). - self._dev._finalize_called = True + # Everything past the claim runs under the guarantee: if it + # raises, the interface goes back before the exception does. + try: + # Find endpoints + intf = cfg[(self._intf_num, 0)] + for ep in intf: + attr = ep.bmAttributes & 0x03 # transfer type mask + direction = ep.bEndpointAddress & 0x80 # direction mask + if attr == usb.util.ENDPOINT_TYPE_BULK: + if direction == EP_DIR_IN: + self._ep_in = ep + else: + self._ep_out = ep + elif attr == usb.util.ENDPOINT_TYPE_INTR: + if direction == EP_DIR_IN: + self._ep_int = ep + + if self._ep_in is None or self._ep_out is None: + raise RuntimeError( + "Could not find bulk endpoints on PTP device" + ) + + self._is_open = True + + # Disable pyusb's weakref.finalize cleanup for this Device. + # During Python shutdown, pyusb's finalizer can call + # libusb_open after the libusb context has been freed, + # causing a SIGSEGV. We handle all USB cleanup ourselves + # in close(). + self._dev._finalize_called = True + except BaseException: + try: + self.close() + except Exception: + pass + raise def close(self): """Release whatever is held, however far open() got. diff --git a/tests/test_usb_transport.py b/tests/test_usb_transport.py index fe02dae..c6650e1 100644 --- a/tests/test_usb_transport.py +++ b/tests/test_usb_transport.py @@ -139,6 +139,21 @@ def test_a_claim_is_given_back_only_once(self, mock_claim, mock_release): ptp.close() mock_release.assert_called_once_with(mock_dev, 0) + @patch("pychdk.usb_transport.usb.util.release_interface") + @patch("pychdk.usb_transport.usb.util.claim_interface") + def test_an_open_that_raises_holds_nothing(self, mock_claim, mock_release): + mock_dev = _make_mock_usb_device_without_bulk_endpoints() + ptp = PTPDevice(mock_dev) + with pytest.raises(RuntimeError, match="bulk endpoints"): + ptp.open() + # No caller did anything here: open() gave the claim back + # itself, so "open raised" means "nothing is held" without + # anyone having to remember a rollback. + mock_claim.assert_called_once_with(mock_dev, 0) + mock_release.assert_called_once_with(mock_dev, 0) + assert ptp._claimed_intf is None + assert not ptp._is_open + @patch("pychdk.usb_transport.usb.util.release_interface") @patch("pychdk.usb_transport.usb.util.claim_interface") def test_a_failed_open_in_a_with_block_claims_nothing( From 0e99ca7948f2792bad8b3c4436c0398e6e41c28e Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Sun, 13 Sep 2026 13:12:48 -0500 Subject: [PATCH 09/11] fix(multicam): close the cameras already open when one fails to open MultiCam built its cameras in a loop with no rollback, so a second camera that would not open left the first one open and claimed while the half-built MultiCam was thrown away. Nothing was then holding it: not the caller, who never received an object, and not _open_devices, which tracks weakly and simply loses the entry when the device is collected. The interface stayed claimed until the process ended. This is the two-camera case, which is the only way the rig is used, and at a bench it presents as a camera that worked a minute ago and now cannot be opened by anything, with no cure but unplugging it. An afternoon spent hunting a hardware fault that is not there is an afternoon of the only hardware time there is. The invariant added to PTPDevice.open does not cover this: the first camera opened successfully, so it is an orphan and not a partial open. Constructing now closes what it built before re-raising, which MultiCam.close already does defensively, one camera at a time. --- src/pychdk/multicam.py | 21 ++++++++++++++++++--- tests/test_multicam.py | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/pychdk/multicam.py b/src/pychdk/multicam.py index 2fd37e1..6ca1908 100644 --- a/src/pychdk/multicam.py +++ b/src/pychdk/multicam.py @@ -12,13 +12,28 @@ class MultiCam: """Manages multiple CHDK cameras for coordinated capture.""" def __init__(self): + """Open every camera found, or leave none of them open. + + A camera that fails to open partway down the list leaves the + ones before it open and claimed, and the half-built MultiCam is + discarded, so nothing is left holding them: not the caller, who + never got an object, and not the cleanup registry, which tracks + devices weakly. They stay claimed until the process ends. + + PTPDevice.open's guarantee does not reach this, because these + cameras opened successfully. They are orphans rather than + partial opens, so the rollback has to be here. + """ devices = list_devices() if not devices: raise RuntimeError("No CHDK cameras found") self.cameras = [] - for info in devices: - cam = ChdkDevice(info) - self.cameras.append(cam) + try: + for info in devices: + self.cameras.append(ChdkDevice(info)) + except BaseException: + self.close() + raise def shoot(self, **kwargs): """Capture from all cameras concurrently. diff --git a/tests/test_multicam.py b/tests/test_multicam.py index 6d0bd20..1c81a7e 100644 --- a/tests/test_multicam.py +++ b/tests/test_multicam.py @@ -16,6 +16,39 @@ def test_discovers_cameras(self, MockDevice, mock_list): mc = MultiCam() assert len(mc.cameras) == 2 + @patch("pychdk.multicam.list_devices") + @patch("pychdk.multicam.ChdkDevice") + def test_a_second_camera_that_fails_closes_the_first( + self, MockDevice, mock_list, + ): + mock_list.return_value = [ + DeviceInfo(0x04A9, 0x1234, 1, 5, "AAA"), + DeviceInfo(0x04A9, 0x1234, 1, 6, "BBB"), + ] + first = MagicMock() + MockDevice.side_effect = [first, RuntimeError("camera 2 will not open")] + + with pytest.raises(RuntimeError, match="camera 2 will not open"): + MultiCam() + + # The half-built MultiCam is discarded, so nothing else can + # ever close camera one: it would be claimed by a process with + # no handle on it until the card was unplugged. + first.close.assert_called_once() + + @patch("pychdk.multicam.list_devices") + @patch("pychdk.multicam.ChdkDevice") + def test_a_first_camera_that_fails_closes_nothing( + self, MockDevice, mock_list, + ): + mock_list.return_value = [ + DeviceInfo(0x04A9, 0x1234, 1, 5, "AAA"), + DeviceInfo(0x04A9, 0x1234, 1, 6, "BBB"), + ] + MockDevice.side_effect = RuntimeError("camera 1 will not open") + with pytest.raises(RuntimeError, match="camera 1 will not open"): + MultiCam() + @patch("pychdk.multicam.list_devices") def test_no_cameras_raises(self, mock_list): mock_list.return_value = [] From 27453bce819edbf61bd46b7c4589da85f0f03049 Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Sun, 13 Sep 2026 13:21:31 -0500 Subject: [PATCH 10/11] fix(usb): disable the pyusb finalizer when an open fails too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Setting _dev._finalize_called was the last statement in the try, so it ran only when opening succeeded. A failed open released the claim and disposed the resources but left pyusb's finalizer armed on a device we had already touched — and the comment immediately above says what that costs: libusb_open on a freed context at interpreter shutdown, and the process goes down with it. So the guarantee added last round, that a failed open holds nothing, was buying a released interface with a possible crash. That is the worse half of the trade: a leaked claim is cured by unplugging a camera, a SIGSEGV at shutdown is cured by nobody and reproduces on whichever machine happens to be unlucky. The rollback takes ownership first and releases second. Also narrows a docstring that claimed more than was checked. ChdkDevice.close said it was safe against a concurrent closer, citing the pyusb serialisation we verified — but that covers releasing the interface, not closing the session, which this method does first by sending a close over the wire with nothing guarding two callers from both sending one. It now says which half is guaranteed and which is the host's problem. --- src/pychdk/device.py | 15 ++++++++++++--- src/pychdk/usb_transport.py | 6 ++++++ tests/test_usb_transport.py | 16 ++++++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/pychdk/device.py b/src/pychdk/device.py index c6fb54c..f74524e 100644 --- a/src/pychdk/device.py +++ b/src/pychdk/device.py @@ -481,9 +481,18 @@ def reconnect(self, wait=2.0): def close(self): """Close the connection to the camera. - Safe to call more than once, and safe against a concurrent - closer on the same device. PTPDevice.close says why, and where - in pyusb to check it. + Safe to call more than once in sequence. + + Concurrently it is safe in one half and not the other, and the + halves are worth keeping apart. Releasing the USB interface is + serialised by pyusb itself, so two closers cannot double-release + it — PTPDevice.close carries the citation. Closing the PTP + session is not serialised: this sends a close over the wire, and + two threads can both find the session open and both send one, + because nothing here guards that. So a host that shares one + device across threads has to serialise its own teardown. + Nothing in this library shares one: MultiCam gives each worker + its own device. """ self._connected = False _open_devices.discard(self) diff --git a/src/pychdk/usb_transport.py b/src/pychdk/usb_transport.py index 0233f6b..302bc36 100644 --- a/src/pychdk/usb_transport.py +++ b/src/pychdk/usb_transport.py @@ -200,6 +200,12 @@ def open(self): # in close(). self._dev._finalize_called = True except BaseException: + # Take ownership of cleanup before letting go. The + # finalizer disabled above is no less dangerous on a device + # we opened part way: releasing the claim and then leaving + # pyusb to reopen a freed context at shutdown would trade a + # leaked interface for a killed process. + self._dev._finalize_called = True try: self.close() except Exception: diff --git a/tests/test_usb_transport.py b/tests/test_usb_transport.py index c6650e1..d275401 100644 --- a/tests/test_usb_transport.py +++ b/tests/test_usb_transport.py @@ -154,6 +154,22 @@ def test_an_open_that_raises_holds_nothing(self, mock_claim, mock_release): assert ptp._claimed_intf is None assert not ptp._is_open + @patch("pychdk.usb_transport.usb.util.release_interface") + @patch("pychdk.usb_transport.usb.util.claim_interface") + def test_a_failed_open_disables_the_pyusb_finalizer( + self, mock_claim, mock_release, + ): + mock_dev = _make_mock_usb_device_without_bulk_endpoints() + mock_dev._finalize_called = False + ptp = PTPDevice(mock_dev) + with pytest.raises(RuntimeError, match="bulk endpoints"): + ptp.open() + # Releasing the claim is not enough: a device we opened far + # enough to touch must not be left to pyusb's finalizer, which + # can reopen a freed context at shutdown and take the process + # with it. Trading a leak for a crash is the worse bargain. + assert mock_dev._finalize_called is True + @patch("pychdk.usb_transport.usb.util.release_interface") @patch("pychdk.usb_transport.usb.util.claim_interface") def test_a_failed_open_in_a_with_block_claims_nothing( From 725d0c897d854847b12220d8dcdbaaa88ea96a41 Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Sun, 13 Sep 2026 13:30:24 -0500 Subject: [PATCH 11/11] docs(ptp): state the property the session comment is there to mark, not the failure modes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment said a failed session open "raises only when the camera refused OPEN_SESSION". It does not: open writes a command, reads a response and parses it, and any of those can raise before the refusal check is ever reached. The conclusion was right — none of them holds an operating-system resource — but the word "only" drew a boundary in the wrong place, and the next sentence of the same comment described one of the cases it had just excluded. Third instance this release of one pattern: a verified fact summarised into a broader claim, the generalisation happening in the sentence rather than in the investigation. It is the sharpest of the three because this comment exists to mark a boundary, which is the one job it then got wrong. Rewritten the way the other two should have been. It names the property — a session open that raises holds no operating-system resource, whatever raised it — and then says what is not covered, which is the camera's own state: a session it may hold that we will never close, indistinguishable on this side from one that was never opened. No claim about the set of ways the thing can fail, because that set was never what mattered. --- src/pychdk/ptp.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/pychdk/ptp.py b/src/pychdk/ptp.py index 81ff3a8..efdc469 100644 --- a/src/pychdk/ptp.py +++ b/src/pychdk/ptp.py @@ -213,13 +213,16 @@ def _receive_data(self, tx_id): return data[:total_length - CONTAINER_HEADER_SIZE] def __enter__(self): - # No rollback here, deliberately: unlike PTPDevice.open, a - # failed session open holds no operating-system resource. It - # raises only when the camera refused OPEN_SESSION, so there is - # no session to close and nothing claimed. The one asymmetry, - # should it ever matter: if the command reached the camera and - # the response read failed, the camera may hold a session we - # would not close, because close() returns early on _is_open. + # No rollback here, deliberately. The property: a session open + # that raises holds no operating-system resource, whatever + # raised it. Nothing is claimed, so there is nothing to give + # back, which is what makes this unlike PTPDevice.open. + # + # Not covered: the camera's own state. If OPEN_SESSION reached + # the camera and we failed before recording the session, the + # camera may hold one we will never close, since close() + # returns early on _is_open — and nothing on this side can tell + # that from a session that was never opened at all. self.open() return self