From 64161ad5542ffcd3c944ece3aba51e2091b6fb80 Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Tue, 15 Sep 2026 16:44:38 -0700 Subject: [PATCH 1/9] fix(device): make the library's claims about CHDK true Three of these are behaviour, the rest are sentences that said more than the code did. switch_mode polled on uBASIC's convention while speaking Lua. CHDK's Lua get_mode() returns is_record, is_video, mode, and is_record is !camera_info.state.mode_play, so it is true in record (luaCB_get_mode, modules/luascript.c); uBASIC's returns 0 for record, 1 for play, 2 for video record (lib/ubasic/ubasic.c). execute_lua_wait hands back the first RET value, so the loop was negating is_record and confirming the opposite of what it set. It now reads the answer the way CHDK sends it, and an unconfirmed switch raises instead of returning exactly as a success did - which is why this survived three releases with no symptom. get_frames asked for no framebuffer. live_view_get_data adds each block only if its LV_TFR_ bit was requested (core/live_view.c), so flags=0 returns the header and the framebuffer descriptions and no pixels. It now asks for the viewport, and the LV_TFR_ constants are named. market_iso was never market ISO. iso_to_sv96 is the APEX96 conversion for real sensitivity - CHDK's own shooting_get_sv96_from_iso notes it is "equivalent to (short)(log2(iso/3.125)*96+0.5) [APEX equation]" - and set_sv96 wants real. The two quantities are kept apart in CHDK by separate properties and a per-camera offset, so rename the parameter rather than invent a conversion we cannot verify. download_after and remove_after were never implemented: the download listed A/DCIM, threw the listing away and returned None, the delete did nothing. Remove them, and say what MultiCam.shoot actually returns. Narrow four claims to what they establish: close() does reach a shared device, through the signal handler and the atexit hook, though no lock is added here because close() runs at interpreter shutdown; drain_ messages is a bounded attempt at fifty; REMOTE_CAP_NOTSET means not initialized now, since CHDK reports a cancelled capture the same way ("following a timeout, RemoteCaptureIsReady and RemoteCaptureGetData will behave as if remote capture were not initialized", set_remotecap_timeout); get_version is the PTP protocol version, not a firmware build; and nothing falls back after a streaming failure, with recovery left as an open question rather than a documented workaround. None of this is checked against a camera. It is read from CHDK's sources, which is how the original errors got in. --- README.md | 8 +- examples/test_camera.py | 8 +- examples/test_two_cameras.py | 7 +- src/pychdk/chdk.py | 41 +++++++- src/pychdk/device.py | 134 ++++++++++++++++++------ src/pychdk/multicam.py | 8 +- src/pychdk/util.py | 36 +++++-- tests/test_device.py | 192 ++++++++++++++++++++++++++++++++++- 8 files changed, 379 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index 056376e..8b86d26 100644 --- a/README.md +++ b/README.md @@ -36,9 +36,11 @@ devices = pychdk.list_devices() # Connect to a camera cam = pychdk.ChdkDevice(devices[0]) -# Check CHDK version +# Check which version of the CHDK PTP protocol the camera speaks. +# This is the protocol version, not the camera's firmware and not the +# CHDK build on the card; the library has no call that reports either. major, minor = cam._chdk.get_version() -print(f"CHDK {major}.{minor}") +print(f"CHDK PTP protocol {major}.{minor}") # Execute Lua on the camera result = cam.lua_execute("return 2 + 2") @@ -156,7 +158,7 @@ tools/ ## Known limitations -- **Remote capture on A2500**: The A2500 CHDK port is alpha-level. Streaming remote capture (`shoot(stream=True)`) may fail with PTP error `0x2002`. The library falls back to SD card capture (`shoot()`) which triggers the shutter but stores images on the card rather than streaming them back. +- **Remote capture on A2500**: The A2500 CHDK port is alpha-level, and streaming remote capture (`shoot(stream=True)`) can fail on it with PTP error `0x2002`. The library does not fall back — the exception reaches the caller and nothing else is attempted. What a caller can do to recover has not been established. In particular, calling `shoot()` afterwards is not a tested path: `_shoot_streaming` has already run `init_usb_capture` by the time the failure surfaces, so USB remote capture is still enabled on the camera, and we have not checked what an ordinary shot does in that state. Recovery is an open question for the bench, not a documented workaround. - **macOS only** for `tools/flash_chdk.py`. The library itself works on macOS and Linux. - **Canon cameras only** — CHDK is Canon-specific. Device discovery defaults to Canon's USB vendor ID (`0x04A9`). diff --git a/examples/test_camera.py b/examples/test_camera.py index 041042a..abcd52e 100644 --- a/examples/test_camera.py +++ b/examples/test_camera.py @@ -57,8 +57,12 @@ def main(): except Exception as e: print(f"Streaming capture failed: {e}") print("Trying non-streaming capture...") - cam.shoot(download_after=False) - print("Shot taken (saved to SD card)") + # This path waits for the shoot script to finish, so the script + # ran; it does not fetch anything back, and the library never + # checks that a file was written. + cam.shoot() + print("shoot() script finished; the image, if one was written, " + "is on the camera's card") cam.close() print("Done.") diff --git a/examples/test_two_cameras.py b/examples/test_two_cameras.py index 9ec3f23..bdb73d5 100644 --- a/examples/test_two_cameras.py +++ b/examples/test_two_cameras.py @@ -45,8 +45,13 @@ def main(): except Exception as e: print(f"Streaming capture failed: {e}") print("Trying non-streaming capture...") + # do_return=False submits the script and does not wait on it, + # so at this point all that is known is that each camera was + # asked to shoot. Whether a shutter fired, and whether a file + # reached either card, is not known here. mc.execute_all("shoot()", do_return=False) - print("Shots taken (saved to SD cards)") + print("shoot() submitted to each camera; not waited on, so " + "whether a picture reached either card is unknown here") mc.close() print("Done.") diff --git a/src/pychdk/chdk.py b/src/pychdk/chdk.py index 9d46d45..c217318 100644 --- a/src/pychdk/chdk.py +++ b/src/pychdk/chdk.py @@ -69,6 +69,22 @@ class ScriptFlag(IntEnum): FLUSH = 0x200 +# Live view transfer flags: which block(s) GetDisplayData should send. +# These are the LV_TFR_* values in CHDK's core/live_view.h. They matter +# because they are not optional: live_view_get_data tests each flag +# before it adds the matching block, so a request with no flag set +# comes back as the header and the framebuffer descriptions and no +# pixel data at all (core/live_view.c). CHDK's own protocol note says +# as much — "The frame buffer descriptions are returned regardless of +# whether the data is available" (core/live_view.h). +# +# Read from the CHDK sources named above, not confirmed against a +# camera. +LV_TFR_VIEWPORT = 0x01 +LV_TFR_BITMAP = 0x04 +LV_TFR_PALETTE = 0x08 +LV_TFR_BITMAP_OPACITY = 0x10 + # Remote capture format bits REMOTE_CAP_JPEG = 0x01 REMOTE_CAP_RAW = 0x02 @@ -360,8 +376,21 @@ def remote_capture_is_ready(self): poll can arrive before init_usb_capture has executed and get this answer perfectly legitimately. It is a failure only once the script has had its chance — which the caller knows and this - method does not. A caller seeing it after the script has ended - is looking at an initialization that never happened. + method does not. + + Even then it establishes less than it looks like it does. All a + caller seeing it after the script has ended knows is that + remote capture is not initialized NOW, and there are at least + two ways to arrive there that this status does not separate: + init_usb_capture never ran, or it ran and the capture was + cancelled afterwards. CHDK cancels on its own download timeout + — the set_remotecap_timeout documentation says "following a + timeout, RemoteCaptureIsReady and RemoteCaptureGetData will + behave as if remote capture were not initialized" + (modules/luascript.c) — and also when a transfer errors, both + by way of remotecap_reset clearing the capture target + (core/remotecap.c). So read this as "not initialized", not as + "never initialized". Returns: Tuple of (is_ready, status). status is REMOTE_CAP_NOTSET @@ -498,7 +527,13 @@ def wait_for_script(self, timeout=30.0, script_id=None): raise TimeoutError(f"Script still running after {timeout}s") def drain_messages(self): - """Drain all pending messages from the script message queue.""" + """Read up to fifty pending messages off the script queue. + + A bounded attempt, not a guarantee. It stops early when the + camera reports the queue empty, and stops regardless at fifty. + It returns nothing either way, so a caller cannot tell which of + the two happened and must not assume the queue is now empty. + """ for _ in range(50): _, has_msgs = self.get_script_status() if not has_msgs: diff --git a/src/pychdk/device.py b/src/pychdk/device.py index f74524e..1407573 100644 --- a/src/pychdk/device.py +++ b/src/pychdk/device.py @@ -14,6 +14,7 @@ from pychdk.ptp import PTPSession, PTPError from pychdk.chdk import ( ChdkPTP, + LV_TFR_VIEWPORT, MessageType, _script_error_name, REMOTE_CAP_JPEG, @@ -210,21 +211,49 @@ def switch_mode(self, mode): Waits for the switch_mode_usb script to finish, then polls get_mode() to confirm the physical switch completed. + + The poll is Lua, and the two CHDK script languages disagree + about this call. CHDK's Lua get_mode() pushes three values — + is_record, is_video, mode — where is_record is + `!camera_info.state.mode_play`, so it is TRUE in record mode + (luaCB_get_mode, modules/luascript.c). CHDK's own wait idiom + relies on that polarity: the set_record documentation beside it + says to spin on `while not get_mode() do sleep(10) end` until + record mode arrives. uBASIC's get_mode is the other way round, + returning 0 for record, 1 for play and 2 for video record + (lib/ubasic/ubasic.c), and it is not what this call speaks. + ChdkPTP.execute_lua_wait returns the first RET value only, so + what lands in `current` is is_record. + + A switch that is never confirmed raises rather than returning + quietly, so a wrong answer here cannot look like a right one. + + This polarity is read from the CHDK sources named above. It has + not been checked against a camera. + + Raises: + RuntimeError: If the camera never reports the requested + mode within the retries. """ - mode_val = 1 if mode == "record" else 0 + want_record = mode == "record" + mode_val = 1 if want_record else 0 script_id = self._chdk.execute_script(f"switch_mode_usb({mode_val})") # Wait for the script to finish before polling, matching its id # so an error left by an earlier shot is not blamed on this. self._chdk.wait_for_script(timeout=5, script_id=script_id) # Give the camera time to physically switch (lens motor, etc.) time.sleep(1) + current = None for _ in range(8): current = self.lua_execute("return get_mode()") - # get_mode() returns 0 (falsy) for record, nonzero for play - in_record = not current - if (mode == "record" and in_record) or (mode == "play" and not in_record): + in_record = bool(current) + if in_record == want_record: return time.sleep(0.5) + raise RuntimeError( + f"The camera did not switch to {mode} mode: get_mode() last " + f"reported is_record={current!r}" + ) def lua_execute(self, lua_code, do_return=True, timeout=10.0): """Execute Lua code on the camera. @@ -243,34 +272,40 @@ def lua_execute(self, lua_code, do_return=True, timeout=10.0): self._chdk.execute_script(lua_code) return None - def shoot(self, shutter_speed=None, market_iso=None, dng=False, - stream=False, download_after=False, remove_after=False): + def shoot(self, shutter_speed=None, real_iso=None, dng=False, + stream=False): """Capture a photo. Args: shutter_speed: Shutter speed in seconds (e.g., 1/100). - market_iso: ISO value (e.g., 100, 200). + real_iso: REAL ISO sensitivity, not the number printed in + the camera's ISO menu. It is converted by iso_to_sv96 + and sent to CHDK's set_sv96, both of which work in real + units; see iso_to_sv96 for why the two quantities are + not interchangeable and why this library will not + convert between them. dng: Request DNG. Not implemented on either path: streaming refuses it, and the card path ignores it. stream: If True, use remote capture (direct USB transfer). - download_after: If True (and stream=False), download from SD card. - remove_after: If True, delete from SD card after download. Returns: - Image data as bytes when stream=True or download_after=True. + The JPEG as bytes when stream=True. Otherwise None: the + camera shoots to its own SD card and nothing is fetched + back, because this library has no path that fetches a card + image. """ parts = [] if shutter_speed is not None: tv96 = shutter_to_tv96(shutter_speed) parts.append(f"set_tv96_direct({tv96})") - if market_iso is not None: - sv96 = iso_to_sv96(market_iso) + if real_iso is not None: + sv96 = iso_to_sv96(real_iso) parts.append(f"set_sv96({sv96})") if stream: return self._shoot_streaming(parts, dng) else: - return self._shoot_standard(parts, download_after, remove_after) + return self._shoot_standard(parts) def _shoot_streaming(self, setup_parts, dng): """Capture using remote capture (PTP commands 13/14). @@ -369,14 +404,21 @@ def _shoot_streaming(self, setup_parts, dng): # Checked after the queue, so a script that explained itself # is reported by its own words rather than by this status. # The two ways of getting here are different observations - # and read differently in a bench log: a script that ran and - # did not initialize, versus one still going after we gave - # up waiting. Neither proves the camera cannot do this. + # and read differently in a bench log: a script that ended + # with remote capture not initialized, versus one still + # going after we gave up waiting. Neither proves the camera + # cannot do this, and the first does not even prove + # init_usb_capture never ran — see + # ChdkPTP.remote_capture_is_ready for why. if status == REMOTE_CAP_NOTSET: if not running: raise RuntimeError( - "The capture script ended without initializing " - "remote capture" + "The capture script ended and remote capture is " + "not initialized: either init_usb_capture never " + "ran, or it ran and the capture was cancelled " + "afterwards — CHDK cancels on its own download " + "timeout and on a transfer error, and reports " + "both the same way as never having initialized" ) if time.monotonic() >= init_deadline: raise RuntimeError( @@ -400,21 +442,24 @@ def _shoot_streaming(self, setup_parts, dng): pass return image - def _shoot_standard(self, setup_parts, download, remove): - """Capture to SD card, optionally download and delete.""" + def _shoot_standard(self, setup_parts): + """Capture to the camera's SD card and leave it there. + + Nothing comes back. Earlier versions took download_after and + remove_after options; neither was implemented — the download + listed A/DCIM, threw the listing away and returned None, and + the delete did nothing at all — so they were removed rather + than left as a promise. Whether the card path is ever needed is + a bench question, not one this library has answered. + + Returns: + None. + """ script = "; ".join(setup_parts + ["shoot()"]) script_id = self._chdk.execute_script(script) # Wait for the shoot script to finish (shutter + SD write), # matching its id so a previous shot's error is not ours. self._chdk.wait_for_script(timeout=30, script_id=script_id) - - if not download: - return None - - # Find the most recent file — simplified approach - result = self.lua_execute( - "return os.listdir('A/DCIM')" - ) return None def upload_file(self, local_path, remote_path): @@ -439,15 +484,29 @@ def download_file(self, remote_path): """ return self._chdk.download_file(remote_path) - def get_frames(self): + def get_frames(self, flags=LV_TFR_VIEWPORT): """Generator yielding live preview frames. + The transfer flags are not optional. CHDK's live_view_get_data + adds each data block only if the matching LV_TFR_* bit was + asked for, so a request with no flag set returns the header and + the framebuffer descriptions and no pixels (core/live_view.c); + earlier versions of this generator sent none and could yield + frames with nothing in them. The default asks for the viewport, + which is the live image. + + Args: + flags: Bitmask of LV_TFR_* values from pychdk.chdk. + Defaults to LV_TFR_VIEWPORT. + Yields: - Raw frame data bytes. + Raw frame data bytes — CHDK's live view payload, which is + the header, the framebuffer descriptions and then whichever + blocks were requested. This library does not parse it. """ while True: try: - data = self._chdk.get_display_data() + data = self._chdk.get_display_data(flags) if data: yield data except PTPError: @@ -491,8 +550,17 @@ def close(self): 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. + + This library reaches that state on its own. MultiCam does give + each worker its own device, but the teardown path is shared: + _cleanup_all closes every open device from the main thread, and + it runs from the SIGINT/SIGTERM handler and from the atexit + hook. Either can close a device while a MultiCam worker is + inside shoot() on it. Nothing here currently serialises that, + and a plain lock is not an obvious fix, because close() runs + from a signal handler and at interpreter shutdown, where a lock + held by a thread being torn down would turn a clean exit into a + hang. It is an open design question, not a solved one. """ self._connected = False _open_devices.discard(self) diff --git a/src/pychdk/multicam.py b/src/pychdk/multicam.py index 6ca1908..dfd4358 100644 --- a/src/pychdk/multicam.py +++ b/src/pychdk/multicam.py @@ -42,8 +42,12 @@ def shoot(self, **kwargs): **kwargs: Passed to each ChdkDevice.shoot(). Returns: - List of image data bytes (one per camera), in the - same order as self.cameras. + One entry per camera, in the same order as self.cameras. + What the entries are depends on the path taken. With + stream=True each is the JPEG bytes that camera sent back. + Otherwise each camera shoots to its own SD card and every + entry is None — nothing is downloaded, because this library + has no path that fetches a card image. """ with concurrent.futures.ThreadPoolExecutor( max_workers=len(self.cameras) diff --git a/src/pychdk/util.py b/src/pychdk/util.py index e6b49ba..e4863f9 100644 --- a/src/pychdk/util.py +++ b/src/pychdk/util.py @@ -21,18 +21,42 @@ def shutter_to_tv96(shutter_speed): return round(-96 * math.log2(shutter_speed)) -def iso_to_sv96(iso): - """Convert ISO value to SV96 (APEX96 sensitivity value). - - Formula: SV96 = 96 * log2(ISO / 3.125) +def iso_to_sv96(real_iso): + """Convert REAL ISO sensitivity to SV96 (APEX96 sensitivity value). + + Formula: SV96 = 96 * log2(real_iso / 3.125) + + This is the APEX96 conversion for real sensitivity, and it is the + same arithmetic CHDK does: shooting_get_sv96_from_iso computes + log2(iso * 32 / 100) * 96 (core/shooting.c), which is the identical + quantity written with 32/100 instead of 1/3.125, and that function + is what CHDK's own Lua iso_to_sv96 calls (modules/luascript.c). The + result is what set_sv96 wants: Lua set_sv96 goes to + shooting_set_sv96, and its counterpart get_sv96 reads + shooting_get_sv96_real (core/shooting.c) — both in real units. + + Market ISO — the number in the camera's own ISO menu — is a + different quantity, and passing one here gives a wrong exposure + rather than an error. CHDK keeps the two apart deliberately: it + stores them in separate properties (PROPCASE_SV for real, + PROPCASE_SV_MARKET for market) and exposes iso_market_to_real, + iso_real_to_market, sv96_market_to_real and sv96_real_to_market to + move between them. The offset is per-camera — core/shooting.c + defaults SV96_MARKET_OFFSET to 69 sv96 units under + `#if !defined(SV96_MARKET_OFFSET)`, with the comment "Can be + overriden in platform_camera.h (see IXUS700 for example)". This + library does NOT convert between them: pass a real ISO, or convert + on the camera with CHDK's own functions before calling. + + Read from the CHDK sources named above. Not measured on a camera. Args: - iso: ISO sensitivity (e.g., 100, 200, 400). + real_iso: Real ISO sensitivity (e.g., 100, 200, 400). Returns: Integer SV96 value. """ - return round(96 * math.log2(iso / 3.125)) + return round(96 * math.log2(real_iso / 3.125)) def aperture_to_av96(aperture): diff --git a/tests/test_device.py b/tests/test_device.py index dd8e56d..e27487d 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -1,6 +1,7 @@ """Tests for high-level ChdkDevice API.""" import importlib import signal +import struct import sys import threading from unittest.mock import MagicMock, patch @@ -9,6 +10,9 @@ from pychdk import device from pychdk.chdk import ( ChdkCommand, + LV_TFR_BITMAP, + LV_TFR_PALETTE, + LV_TFR_VIEWPORT, MessageType, REMOTE_CAP_NOTSET, ScriptDataType, @@ -21,6 +25,8 @@ DeviceInfo, _open_devices, ) +from pychdk.ptp import PTPError +from pychdk.util import iso_to_sv96 class TestListDevices: @@ -337,9 +343,11 @@ def test_switch_mode_survives_a_previous_captures_error(self): ([MessageType.ERR, ScriptErrorType.RUN, 9, 12], b"stale error\x00"), # from script 9, not ours ([0], b""), # ours finished cleanly - ([0, 0], b""), # get_mode() script starts - ([0], b""), # drain: nothing pending - ([0], b""), # not running, no messages + ([0], b""), # drain before get_mode: nothing pending + ([31], b""), # get_mode() script starts, id 31 + ([0b11], b""), # running, its answer waiting + ([MessageType.RET, ScriptDataType.BOOLEAN, 31, 4], + struct.pack(" 1 + + def test_a_play_switch_the_camera_never_makes_raises(self, monkeypatch): + fake = _RecordPlayCamera(in_record=True, obeys=False) + dev = self._device(fake, monkeypatch) + with pytest.raises(RuntimeError, match="play"): + dev.switch_mode("play") + + +class TestLivePreviewAsksForPixels: + """GetDisplayData with no transfer flag sends no pixels at all.""" + + def _make_device(self): + info = DeviceInfo( + vendor_id=0x04A9, product_id=0x1234, + bus_num=1, device_num=5, serial_num="ABC", + ) + with patch("pychdk.device.PTPDevice"), \ + patch("pychdk.device.PTPSession"), \ + patch("pychdk.device.ChdkPTP") as MockChdk: + dev = ChdkDevice(info, _usb_device=MagicMock()) + return dev, MockChdk.return_value + + def test_the_viewport_flag_is_the_value_chdk_defines(self): + """core/live_view.h: #define LV_TFR_VIEWPORT 0x01.""" + assert LV_TFR_VIEWPORT == 0x01 + + def test_get_frames_asks_for_the_viewport(self): + dev, mock_chdk = self._make_device() + mock_chdk.get_display_data.side_effect = [b"pixels", PTPError(0x2002)] + assert list(dev.get_frames()) == [b"pixels"] + assert mock_chdk.get_display_data.call_args_list[0].args == ( + LV_TFR_VIEWPORT, + ) + + def test_a_caller_can_ask_for_something_else(self): + dev, mock_chdk = self._make_device() + mock_chdk.get_display_data.side_effect = [b"bm", PTPError(0x2002)] + list(dev.get_frames(flags=LV_TFR_BITMAP | LV_TFR_PALETTE)) + assert mock_chdk.get_display_data.call_args_list[0].args == ( + LV_TFR_BITMAP | LV_TFR_PALETTE, + ) + + +class TestShootTakesRealIsoAndNothingItCannotDo: + """The signature has to name the quantity, and drop the dead options.""" + + def _make_device(self): + info = DeviceInfo( + vendor_id=0x04A9, product_id=0x1234, + bus_num=1, device_num=5, serial_num="ABC", + ) + with patch("pychdk.device.PTPDevice"), \ + patch("pychdk.device.PTPSession"), \ + patch("pychdk.device.ChdkPTP") as MockChdk: + dev = ChdkDevice(info, _usb_device=MagicMock()) + return dev, MockChdk.return_value + + def test_real_iso_is_converted_with_the_apex_formula(self): + dev, mock_chdk = self._make_device() + dev.shoot(real_iso=100) + script = mock_chdk.execute_script.call_args.args[0] + assert f"set_sv96({iso_to_sv96(100)})" in script + + def test_market_iso_is_gone(self): + dev, _ = self._make_device() + with pytest.raises(TypeError): + dev.shoot(market_iso=100) + + def test_download_after_is_gone(self): + dev, _ = self._make_device() + with pytest.raises(TypeError): + dev.shoot(download_after=True) + + def test_remove_after_is_gone(self): + dev, _ = self._make_device() + with pytest.raises(TypeError): + dev.shoot(remove_after=True) + + def test_the_card_path_returns_nothing_and_lists_nothing(self): + dev, mock_chdk = self._make_device() + assert dev.shoot() is None + scripts = [ + call.args[0] for call in mock_chdk.execute_script.call_args_list + ] + assert not any("os.listdir" in s for s in scripts) From 4db89f0aa7edcd1542a40a55ef7e4a344b385e6c Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Tue, 15 Sep 2026 16:44:49 -0700 Subject: [PATCH 2/9] fix(flash): confirm on every path, and name the disk being erased pick_disk asked "This will ERASE the disk. Continue? [y/N]" only when there was exactly one candidate. With two or more it printed the list, took an index and returned the chosen disk before the prompt was ever reached - so the path where picking the wrong disk is most likely was the one path that erased without asking. Selection is not consent, and the two are now separate steps. The prompt names the device node, media name and size rather than "the disk", because find_removable_disks accepts any physical removable medium: on a Mac an external USB drive qualifies, and the operator reading the prompt is the only check on that. A negative index is rejected rather than counted from the end of the list. Correct the safety notes while here: the filter is RemovableMedia plus VirtualOrPhysical, not an Internal check, and an internal SD slot is exactly what we want to flash from. --- docs/chdk-flasher.md | 10 ++-- tests/test_flash_chdk.py | 125 +++++++++++++++++++++++++++++++++++++++ tools/flash_chdk.py | 40 ++++++++++--- 3 files changed, 163 insertions(+), 12 deletions(-) diff --git a/docs/chdk-flasher.md b/docs/chdk-flasher.md index 5fe6a3a..8a6773e 100644 --- a/docs/chdk-flasher.md +++ b/docs/chdk-flasher.md @@ -23,9 +23,9 @@ A single-file Python CLI tool that downloads, formats, and flashes CHDK firmware ## Safety -- Only lists removable, external disks — internal drives are filtered out via `diskutil info -plist` checking for `RemovableMedia` and `Internal` keys -- Requires explicit `y` confirmation before formatting -- If multiple removable disks are found, asks user to pick +- Lists only whole disks that `diskutil info -plist` reports as `RemovableMedia` and `VirtualOrPhysical: Physical`, which excludes fixed drives, disk images and synthesised volumes. There is no `Internal` check, and none is wanted: a built-in SD slot is internal and is exactly what we want to flash from. The filter does not distinguish an SD card from any other removable medium, so an external USB drive can appear in the list — the confirmation prompt below, which names the disk, is the check on that +- Requires explicit `y` confirmation before formatting, on every path +- If multiple removable disks are found, asks user to pick — and then asks the same confirmation, naming the chosen disk. Selection is not consent - If no removable disks found, tells user to insert a card and exits ## Boot sector patching @@ -54,8 +54,8 @@ Writes `ODD\n` or `EVEN\n` to `OWN.TXT` on the card root. This is how Captua ide $ python3 tools/flash_chdk.py Downloading CHDK 1.6.1-6315 for A2500... (cached at ~/.cache/pychdk/a2500-100a-1.6.1-6315-full.zip) -Found removable disk: /dev/disk4 (SDCARD, 16GB, FAT32) -This will ERASE /dev/disk4. Continue? [y/N] y +Found removable disk: /dev/disk4 (SDCARD, 16.0GB) +This will ERASE /dev/disk4 (SDCARD, 16.0GB) and everything on it. Continue? [y/N] y Formatting /dev/disk4 as FAT32... Extracting CHDK files to /Volumes/CHDK_A2500... Patching boot sector... diff --git a/tests/test_flash_chdk.py b/tests/test_flash_chdk.py index a61a52b..65d1aed 100644 --- a/tests/test_flash_chdk.py +++ b/tests/test_flash_chdk.py @@ -710,3 +710,128 @@ def test_a_filesystem_on_the_whole_device_is_not_inspectable( })) with pytest.raises(SystemExit): tool.read_existing_own_txt("/dev/disk4") + + +def _answerer(answers): + """Stand in for input(), recording every prompt it is shown.""" + remaining = list(answers) + prompts = [] + + def ask(prompt=""): + prompts.append(prompt) + if not remaining: + raise AssertionError(f"unexpected prompt: {prompt!r}") + return remaining.pop(0) + + ask.prompts = prompts + return ask + + +def _disks(count): + """`count` removable disks, as find_removable_disks reports them.""" + return [ + { + "disk": f"/dev/disk{4 + i}", + "name": f"Card {i}", + "size_gb": 15.9 + i, + } + for i in range(count) + ] + + +class TestNoDiskIsErasedWithoutAConfirmation: + """The multi-disk path is where picking wrong is most likely. + + It was also the only path that returned before the confirmation was + asked, so choosing from a list erased whatever was chosen. Every + path has to ask, and the question has to name the disk: the tool + accepts any physical removable medium, which on a Mac includes an + external USB drive that is not an SD card at all. + """ + + def test_choosing_from_several_disks_is_still_confirmed(self, monkeypatch): + tool = _load_tool() + ask = _answerer(["1", "y"]) + monkeypatch.setattr("builtins.input", ask) + assert tool.pick_disk(_disks(3)) == "/dev/disk5" + assert any("ERASE" in p for p in ask.prompts) + + def test_declining_after_choosing_from_several_disks_stops( + self, monkeypatch, + ): + tool = _load_tool() + monkeypatch.setattr("builtins.input", _answerer(["1", "n"])) + with pytest.raises(SystemExit) as excinfo: + tool.pick_disk(_disks(3)) + assert excinfo.value.code == 0 + + def test_saying_nothing_after_choosing_from_several_disks_stops( + self, monkeypatch, + ): + tool = _load_tool() + monkeypatch.setattr("builtins.input", _answerer(["2", ""])) + with pytest.raises(SystemExit) as excinfo: + tool.pick_disk(_disks(3)) + assert excinfo.value.code == 0 + + def test_the_multi_disk_prompt_names_the_disk_it_will_erase( + self, monkeypatch, + ): + tool = _load_tool() + ask = _answerer(["2", "y"]) + monkeypatch.setattr("builtins.input", ask) + tool.pick_disk(_disks(3)) + erase = [p for p in ask.prompts if "ERASE" in p] + assert erase, ask.prompts + assert "/dev/disk6" in erase[0] + assert "Card 2" in erase[0] + assert "17.9" in erase[0] + + def test_the_single_disk_prompt_names_the_disk_it_will_erase( + self, monkeypatch, + ): + tool = _load_tool() + ask = _answerer(["y"]) + monkeypatch.setattr("builtins.input", ask) + assert tool.pick_disk(_disks(1)) == "/dev/disk4" + erase = [p for p in ask.prompts if "ERASE" in p] + assert erase, ask.prompts + assert "/dev/disk4" in erase[0] + assert "Card 0" in erase[0] + assert "15.9" in erase[0] + + def test_an_unreadable_choice_stops_before_the_confirmation( + self, monkeypatch, + ): + tool = _load_tool() + monkeypatch.setattr("builtins.input", _answerer(["banana"])) + with pytest.raises(SystemExit) as excinfo: + tool.pick_disk(_disks(3)) + assert excinfo.value.code == 1 + + def test_a_negative_choice_is_not_quietly_counted_from_the_end( + self, monkeypatch, + ): + """A typed '-1' must not silently select a disk nobody named.""" + tool = _load_tool() + monkeypatch.setattr("builtins.input", _answerer(["-1"])) + with pytest.raises(SystemExit) as excinfo: + tool.pick_disk(_disks(3)) + assert excinfo.value.code == 1 + + def test_main_erases_nothing_when_the_confirmation_is_declined( + self, monkeypatch, + ): + tool = _load_tool() + formatted = [] + monkeypatch.setattr(tool, "download_chdk", lambda: None) + monkeypatch.setattr(tool, "find_removable_disks", lambda: _disks(3)) + monkeypatch.setattr("builtins.input", _answerer(["1", "n"])) + monkeypatch.setattr(tool, "_run", _fake_run(PARTITIONED_LAYOUT)) + monkeypatch.setattr( + tool, "format_card", lambda disk: formatted.append(disk), + ) + with pytest.raises(SystemExit) as excinfo: + tool.main() + assert excinfo.value.code == 0 + assert formatted == [] diff --git a/tools/flash_chdk.py b/tools/flash_chdk.py index 9e074e5..0b56019 100755 --- a/tools/flash_chdk.py +++ b/tools/flash_chdk.py @@ -80,31 +80,57 @@ def find_removable_disks() -> list[dict]: return disks +def _describe(disk: dict) -> str: + """Name a disk the way the confirmation prompt has to name it.""" + return f"{disk['disk']} ({disk['name']}, {disk['size_gb']}GB)" + + def pick_disk(disks: list[dict]) -> str: - """Let user pick a disk. Returns /dev/diskN path.""" + """Let the operator pick a disk, and confirm before it is erased. + + Selection and confirmation are separate steps, and the + confirmation happens on every path. The multi-disk path used to + return the chosen disk directly, so choosing from a list — the + case where picking the wrong one is most likely — was the one path + that erased without asking. + + The prompt names the specific disk rather than saying "the disk", + because find_removable_disks accepts any physical removable medium. + On a Mac that includes an external USB drive that is not an SD card + at all, and the operator is the only check on that. + + Returns: + The /dev/diskN path of the confirmed disk. + """ if not disks: print("No removable disks found. Insert an SD card and try again.") sys.exit(1) if len(disks) == 1: - d = disks[0] - print(f"Found removable disk: {d['disk']} ({d['name']}, {d['size_gb']}GB)") + chosen = disks[0] + print(f"Found removable disk: {_describe(chosen)}") else: print("Found multiple removable disks:") for i, d in enumerate(disks): - print(f" [{i}] {d['disk']} ({d['name']}, {d['size_gb']}GB)") + print(f" [{i}] {_describe(d)}") choice = input("Which disk? ").strip() try: - return disks[int(choice)]["disk"] + index = int(choice) + if index < 0: + raise IndexError(index) + chosen = disks[index] except (ValueError, IndexError): print("Invalid choice.") sys.exit(1) - confirm = input("This will ERASE the disk. Continue? [y/N] ").strip().lower() + confirm = input( + f"This will ERASE {_describe(chosen)} and everything on it. " + "Continue? [y/N] " + ).strip().lower() if confirm != "y": print("Aborted.") sys.exit(0) - return disks[0]["disk"] + return chosen["disk"] def format_card(disk: str) -> str: From a86230c34be544c556790ace73b44aabf36f840f Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Tue, 15 Sep 2026 16:44:49 -0700 Subject: [PATCH 3/9] chore(release): 0.1.3 A release about sentences. Removing download_after/remove_after and renaming market_iso are API breaks; pre-1.0 with Captua as the only consumer, the version stays 0.1.3 rather than pretending otherwise, and the notes lead with the breaks. Captua's chdk_backend passes market_iso= and has to be updated with the pointer bump. --- RELEASE-NOTES-v0.1.3.md | 134 ++++++++++++++++++++++++++++++++++++++++ pyproject.toml | 2 +- src/pychdk/__init__.py | 2 +- 3 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 RELEASE-NOTES-v0.1.3.md diff --git a/RELEASE-NOTES-v0.1.3.md b/RELEASE-NOTES-v0.1.3.md new file mode 100644 index 0000000..1926b74 --- /dev/null +++ b/RELEASE-NOTES-v0.1.3.md @@ -0,0 +1,134 @@ +# pychdk 0.1.3 + +A release about sentences. Most of what changed here is a claim the library +made that was broader than what the code did: docstrings, error messages and +README lines that told a reader something we had not established. Three +findings needed real code changes; the rest needed the truth written down. + +**None of this is verified against hardware.** Every correction below is +reasoned from reading CHDK's own sources — `modules/luascript.c`, +`lib/ubasic/ubasic.c`, `core/live_view.{c,h}`, `core/shooting.c`, +`core/remotecap.c`, `core/ptp.h` — and from reading this library. That is +exactly how the original errors got in. A green test suite here means "the +code now agrees with what we believe CHDK does", not "we watched a camera do +it". Everything below is a bench item. + +## Breaking changes + +Pre-1.0, with Captua as the only consumer. There is no compatibility shim and +no deprecation period, and the version stays `0.1.3` rather than pretending +otherwise. + +- **`ChdkDevice.shoot(market_iso=...)` is now `shoot(real_iso=...)`.** The + value was never treated as market ISO. `iso_to_sv96` computes + `96 * log2(ISO / 3.125)`, the APEX96 conversion for *real* sensitivity, and + `set_sv96` wants real sv96 as well. Market ISO — the number in the camera's + own ISO menu — is a different quantity, and the conversion between them is + per-camera, so this library does not do it. The parameter now says which one + it takes. **Callers must check whether the number they were passing was a + market number**; renaming the keyword makes the call fail loudly instead of + silently mis-exposing. Captua's `backend/capture/backends/chdk_backend.py` + passes `market_iso=` and will need updating with the pointer bump. +- **`shoot(download_after=...)` and `shoot(remove_after=...)` are gone.** + Neither was implemented: `download_after` listed `A/DCIM`, discarded the + listing and returned `None`; `remove_after` did nothing at all. They were + removed rather than implemented, so the documentation is true now. Whether + the card path ever needs building is a bench decision. +- **`ChdkDevice.switch_mode` now raises.** A switch that is never confirmed + raises `RuntimeError` naming the mode asked for and what `get_mode()` last + reported, instead of returning exactly as a success did. +- **`util.iso_to_sv96`'s parameter is renamed** `iso` to `real_iso`. Only + matters to a caller passing it by keyword. + +## Corrected behaviour + +- **`switch_mode` confirmed the opposite of what it set.** The polling code + carried the comment "get_mode() returns 0 (falsy) for record, nonzero for + play" and inverted the answer accordingly. That describes **uBASIC**'s + `get_mode` (`lib/ubasic/ubasic.c`: 0 record, 1 play, 2 video record). The + call is **Lua**, and CHDK's Lua `get_mode()` returns three values — + `is_record, is_video, mode` — where `is_record` is `!mode_play` and so is + *true* in record mode (`luaCB_get_mode`, `modules/luascript.c`). Since + `execute_lua_wait` returns only the first RET value, the library was reading + `is_record` and negating it. The inversion survived three releases with no + visible symptom precisely because the loop fell through and returned on + failure; it now raises, and the comment names the language and all three + return values. +- **`get_frames` requested no pixel data.** It called `get_display_data()` + with the default `flags=0`. CHDK's `live_view_get_data` adds each data block + only if the matching `LV_TFR_*` bit was requested (`core/live_view.c`), so a + flagless request returns the header and framebuffer descriptions and nothing + else — the generator could yield frames with no image in them. + `get_frames` now defaults to `LV_TFR_VIEWPORT` and takes `flags` as an + argument. The `LV_TFR_*` constants from `core/live_view.h` are now named in + `pychdk.chdk`. Captua was unaffected because it calls + `get_display_data(LV_TFR_VIEWPORT)` directly rather than using this + generator; that was luck. + +## Corrected claims + +- **`close()` claimed nothing in the library shares a device.** It does. + `_cleanup_all` closes every open device from the main thread, and it runs + from the SIGINT/SIGTERM handler and from the `atexit` hook — either can + close a device a `MultiCam` worker is inside `shoot()` on. The docstring now + says so. **No lock was added and the teardown path is unchanged**: a plain + lock is hazardous here, because `close()` runs from a signal handler and at + interpreter shutdown, where a lock held by a thread being torn down turns a + clean exit into a hang. The design question is open. +- **`drain_messages` said "drain all pending messages".** It reads at most + fifty and reports nothing about what remains. Documented as what it is: a + bounded attempt, up to fifty, with no indication whether the queue was + emptied. +- **`REMOTE_CAP_NOTSET` was described as "an initialization that never + happened".** CHDK cancels a remote capture on its own download timeout and + reports it identically — "following a timeout, RemoteCaptureIsReady and + RemoteCaptureGetData will behave as if remote capture were not initialized" + (`set_remotecap_timeout`, `modules/luascript.c`; the mechanism is + `remotecap_reset` in `core/remotecap.c`, also reached on a transfer error). + The docstring and the matching `RuntimeError` in `device.py` now say what + the status establishes: remote capture is not initialised *now*, for at + least two reasons it cannot distinguish. +- **`MultiCam.shoot`'s docstring promised "List of image data bytes (one per + camera)".** With the card path that was a list of `None`s. It now states + what each path returns. +- **README, A2500 remote capture.** The old sentence said the library "falls + back to SD card capture (`shoot()`) which triggers the shutter but stores + images on the card". It does not fall back, and a plain `shoot()` after a + streaming failure is not an established recovery path: `_shoot_streaming` + has already run `init_usb_capture`, so USB remote capture is still enabled + on the camera when the failure surfaces, and nobody has tested what an + ordinary shot does in that state. The README now says the failure mode and + says recovery is unestablished, rather than promising one. +- **README, "Check CHDK version".** `get_version()` returns the version of the + CHDK **PTP protocol** the camera speaks (`PTP_CHDK_VERSION_MAJOR/MINOR`, + `core/ptp.h`), not the camera's firmware or the CHDK build on the card. The + library has no call that reports either. Relabelled. +- **`examples/test_two_cameras.py`** printed "Shots taken (saved to SD cards)" + immediately after submitting a script with `do_return=False`, which it never + waits on. It now prints what is actually known at that point. + `examples/test_camera.py` got the same treatment and dropped its + `download_after=` argument. + +## Data-safety fix + +- **`tools/flash_chdk.py` could erase a disk the operator never confirmed.** + With one removable disk, `pick_disk` printed it and asked "This will ERASE + the disk. Continue? [y/N]". With two or more it printed the list, took an + index, and returned the chosen disk *before reaching the confirmation* — so + the multi-disk path, the one where picking wrong is most likely, was the one + path with no confirmation at all. Selection and confirmation are now + separate steps and the confirmation is asked on every path. The prompt names + the specific disk — device node, media name, size — rather than "the disk", + because `find_removable_disks` accepts any physical removable medium, which + on a Mac includes an external USB drive that is not an SD card. A negative + index is now rejected rather than counted from the end of the list. + + This is the only finding in this release that can destroy someone's data. + +## Tests + +`.venv/bin/python -m pytest tests/ -q` — 205 passed before, 225 after. New +coverage: the `switch_mode` polarity (written from CHDK's sources so that it +fails under either inversion, not from what the implementation expects), the +raise on an unconfirmed switch, the live-view transfer flags, the removed +`shoot` keywords, and six cases around the flasher's confirmation prompt. diff --git a/pyproject.toml b/pyproject.toml index 6362a34..69af9f7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "pychdk" -version = "0.1.2" +version = "0.1.3" 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 ad0d715..115c121 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.2" +__version__ = "0.1.3" __all__ = [ "ChdkDevice", "list_devices", "DeviceInfo", "install_signal_handlers", From 9db869e2c83b1003ea65790ddc1c8f2b3f383a1f Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Tue, 15 Sep 2026 16:58:56 -0700 Subject: [PATCH 4/9] fix(device): retry an unanswered mode poll, and narrow five claims Review of this release found six more, five of them the same fault the release exists to correct. The real one: execute_lua_wait returns None when a script ends with no RET message, and bool(None) is False - so the polarity fix confirmed play mode on a camera that had said nothing at all. Silence is not an answer of false. It is retried now, and a poll that answers after a silence is still read. The five sentences: - "no path that fetches a card image" (shoot, _shoot_standard, MultiCam.shoot). download_file fetches a card file by path. What is missing is discovering the path of the image a shot just wrote. - "the library has no call that reports either" (README, on the CHDK build). lua_execute("return get_buildinfo()") reports it, and examples/test_camera.py already calls it. There is no dedicated method; that is a narrower claim and the true one. - "USB remote capture is still enabled" after a streaming failure (README). The exception establishes no such thing: it can come from the script submission before init_usb_capture ran, or from a capture that initialized and was cancelled on CHDK's own timeout - which this release's own remote_capture_is_ready note explains. - "either can close a device while a MultiCam worker is inside shoot()" (close). The signal handler can; atexit cannot. concurrent.futures registers its shutdown through threading._register_atexit, joined in threading._shutdown, which runs before atexit handlers. The signal hazard and the lock objection both stand without the atexit claim. - and in the backend's companion fix, a test docstring claiming it pinned the fake's polarity as well as the code's. It pins the code's; stuck_in_play bypasses the fake's own switch. A second test pins the other half. --- README.md | 8 +++-- RELEASE-NOTES-v0.1.3.md | 67 +++++++++++++++++++++++++++++++---------- src/pychdk/device.py | 44 +++++++++++++++++++++------ src/pychdk/multicam.py | 5 +-- tests/test_device.py | 37 +++++++++++++++++++++++ 5 files changed, 130 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 8b86d26..82f672f 100644 --- a/README.md +++ b/README.md @@ -37,8 +37,10 @@ devices = pychdk.list_devices() cam = pychdk.ChdkDevice(devices[0]) # Check which version of the CHDK PTP protocol the camera speaks. -# This is the protocol version, not the camera's firmware and not the -# CHDK build on the card; the library has no call that reports either. +# This is the protocol version only — not the camera's firmware, and not +# the CHDK build on the card. There is no dedicated method for those; +# for the build, run CHDK's own Lua: +# cam.lua_execute("return get_buildinfo()") major, minor = cam._chdk.get_version() print(f"CHDK PTP protocol {major}.{minor}") @@ -158,7 +160,7 @@ tools/ ## Known limitations -- **Remote capture on A2500**: The A2500 CHDK port is alpha-level, and streaming remote capture (`shoot(stream=True)`) can fail on it with PTP error `0x2002`. The library does not fall back — the exception reaches the caller and nothing else is attempted. What a caller can do to recover has not been established. In particular, calling `shoot()` afterwards is not a tested path: `_shoot_streaming` has already run `init_usb_capture` by the time the failure surfaces, so USB remote capture is still enabled on the camera, and we have not checked what an ordinary shot does in that state. Recovery is an open question for the bench, not a documented workaround. +- **Remote capture on A2500**: The A2500 CHDK port is alpha-level, and streaming remote capture (`shoot(stream=True)`) can fail on it with PTP error `0x2002`. The library does not fall back — the exception reaches the caller and nothing else is attempted. Nor does the exception tell you what state the camera is in: the failure can come from the script submission, before `init_usb_capture` has run at all, or from a capture that initialized and was then cancelled. So whether USB remote capture is still enabled afterwards, and whether an ordinary `shoot()` would recover the picture, are both unestablished. Recovery is an open question for the bench, not a documented workaround. - **macOS only** for `tools/flash_chdk.py`. The library itself works on macOS and Linux. - **Canon cameras only** — CHDK is Canon-specific. Device discovery defaults to Canon's USB vendor ID (`0x04A9`). diff --git a/RELEASE-NOTES-v0.1.3.md b/RELEASE-NOTES-v0.1.3.md index 1926b74..84e16d9 100644 --- a/RELEASE-NOTES-v0.1.3.md +++ b/RELEASE-NOTES-v0.1.3.md @@ -36,7 +36,13 @@ otherwise. the card path ever needs building is a bench decision. - **`ChdkDevice.switch_mode` now raises.** A switch that is never confirmed raises `RuntimeError` naming the mode asked for and what `get_mode()` last - reported, instead of returning exactly as a success did. + reported, instead of returning exactly as a success did. A poll that comes + back with *no* answer — `execute_lua_wait` returns `None` when a script ends + with no RET message — is now retried rather than read as a falsy + `is_record`. The first version of this fix used a plain `bool()`, and since + `bool(None)` is `False`, a camera that said nothing at all would have been + confirmed as being in play mode. Caught in the review of this release, not + on a camera. - **`util.iso_to_sv96`'s parameter is renamed** `iso` to `real_iso`. Only matters to a caller passing it by keyword. @@ -69,10 +75,17 @@ otherwise. - **`close()` claimed nothing in the library shares a device.** It does. `_cleanup_all` closes every open device from the main thread, and it runs - from the SIGINT/SIGTERM handler and from the `atexit` hook — either can - close a device a `MultiCam` worker is inside `shoot()` on. The docstring now - says so. **No lock was added and the teardown path is unchanged**: a plain - lock is hazardous here, because `close()` runs from a signal handler and at + from the SIGINT/SIGTERM handler, which can close a device a `MultiCam` + worker is inside `shoot()` on. The `atexit` hook is *not* the same hazard: + `concurrent.futures` registers its pool shutdown through + `threading._register_atexit`, which joins the worker threads during + `threading._shutdown`, and that runs before `atexit` handlers — so a worker + has finished before `_cleanup_all` runs at exit. A daemon thread belonging + to the host is not joined that way, but that is the host's thread. The + docstring now draws that line. + + **No lock was added and the teardown path is unchanged**: a plain lock is + hazardous here, because `close()` runs from a signal handler and at interpreter shutdown, where a lock held by a thread being torn down turns a clean exit into a hang. The design question is open. - **`drain_messages` said "drain all pending messages".** It reads at most @@ -90,19 +103,30 @@ otherwise. least two reasons it cannot distinguish. - **`MultiCam.shoot`'s docstring promised "List of image data bytes (one per camera)".** With the card path that was a list of `None`s. It now states - what each path returns. -- **README, A2500 remote capture.** The old sentence said the library "falls - back to SD card capture (`shoot()`) which triggers the shutter but stores - images on the card". It does not fall back, and a plain `shoot()` after a - streaming failure is not an established recovery path: `_shoot_streaming` - has already run `init_usb_capture`, so USB remote capture is still enabled - on the camera when the failure surfaces, and nobody has tested what an - ordinary shot does in that state. The README now says the failure mode and - says recovery is unestablished, rather than promising one. + what each path returns. The first rewrite then overshot in the other + direction, saying the library "has no path that fetches a card image": + `download_file` does exactly that, by path. What is missing is discovering + the path the shot just taken was written to, and that is what the docstrings + now say. +- **README, A2500 remote capture — rewritten twice.** The old sentence said + the library "falls back to SD card capture (`shoot()`) which triggers the + shutter but stores images on the card". It does not fall back at all. The + replacement written for this release then claimed that `init_usb_capture` + had already run by the time the failure surfaced, so remote capture was + still enabled on the camera — which is itself more than the exception + establishes: the failure can come from the script submission, before + initialization ran at all, or from a capture that initialized and was then + cancelled on CHDK's own timeout. The README now says only what is + supported: there is no automatic fallback, and the exception establishes + neither what state the camera is in nor whether an ordinary `shoot()` would + recover the picture. - **README, "Check CHDK version".** `get_version()` returns the version of the CHDK **PTP protocol** the camera speaks (`PTP_CHDK_VERSION_MAJOR/MINOR`, - `core/ptp.h`), not the camera's firmware or the CHDK build on the card. The - library has no call that reports either. Relabelled. + `core/ptp.h`), not the camera's firmware or the CHDK build on the card. + There is no dedicated method for those, but the build is reachable through + CHDK's own Lua — `lua_execute("return get_buildinfo()")`, which + `examples/test_camera.py` already calls. Relabelled, and the README now + points at `get_buildinfo()` instead of saying no such call exists. - **`examples/test_two_cameras.py`** printed "Shots taken (saved to SD cards)" immediately after submitting a script with `do_return=False`, which it never waits on. It now prints what is actually known at that point. @@ -132,3 +156,14 @@ coverage: the `switch_mode` polarity (written from CHDK's sources so that it fails under either inversion, not from what the implementation expects), the raise on an unconfirmed switch, the live-view transfer flags, the removed `shoot` keywords, and six cases around the flasher's confirmation prompt. + +## A note on how this release was reviewed + +Six further corrections in the lists above were found by adversarial review +*after* the first pass was written and its tests were green — five of them +sentences that still claimed more than the code or the cited source +established, and one a real fault (`bool(None)`) introduced by the fix for +the inverted polarity. That is the same failure mode this release exists to +correct, reproduced inside the correction. The tests were green throughout. +Prose has no tests; the only check on it is someone reading it against the +source, twice. diff --git a/src/pychdk/device.py b/src/pychdk/device.py index 1407573..0e57a05 100644 --- a/src/pychdk/device.py +++ b/src/pychdk/device.py @@ -226,7 +226,9 @@ def switch_mode(self, mode): what lands in `current` is is_record. A switch that is never confirmed raises rather than returning - quietly, so a wrong answer here cannot look like a right one. + quietly, so a wrong answer here cannot look like a right one. A + poll that comes back with no answer at all is not an answer + either: it is retried, not read as false. This polarity is read from the CHDK sources named above. It has not been checked against a camera. @@ -246,6 +248,14 @@ def switch_mode(self, mode): current = None for _ in range(8): current = self.lua_execute("return get_mode()") + if current is None: + # execute_lua_wait returns None when the script ends with + # no RET message, so this is the absence of an answer, not + # an answer of false. Reading it as false would confirm + # play mode on a camera that said nothing at all — and + # since bool(None) is False, a plain bool() here did. + time.sleep(0.5) + continue in_record = bool(current) if in_record == want_record: return @@ -291,8 +301,9 @@ def shoot(self, shutter_speed=None, real_iso=None, dng=False, Returns: The JPEG as bytes when stream=True. Otherwise None: the camera shoots to its own SD card and nothing is fetched - back, because this library has no path that fetches a card - image. + back. download_file will fetch a file off the card by + path; what is missing is any way to learn the path the shot + just taken was written to. """ parts = [] if shutter_speed is not None: @@ -449,8 +460,10 @@ def _shoot_standard(self, setup_parts): remove_after options; neither was implemented — the download listed A/DCIM, threw the listing away and returned None, and the delete did nothing at all — so they were removed rather - than left as a promise. Whether the card path is ever needed is - a bench question, not one this library has answered. + than left as a promise. download_file can fetch a card file by + path; the missing piece is discovering the path of the image + this call just took. Whether that is ever needed is a bench + question, not one this library has answered. Returns: None. @@ -554,11 +567,22 @@ def close(self): This library reaches that state on its own. MultiCam does give each worker its own device, but the teardown path is shared: _cleanup_all closes every open device from the main thread, and - it runs from the SIGINT/SIGTERM handler and from the atexit - hook. Either can close a device while a MultiCam worker is - inside shoot() on it. Nothing here currently serialises that, - and a plain lock is not an obvious fix, because close() runs - from a signal handler and at interpreter shutdown, where a lock + it runs from the SIGINT/SIGTERM handler. That handler can close + a device while a MultiCam worker is inside shoot() on it, and + nothing here serialises the two. + + The atexit hook is a different case, and the difference is + worth keeping straight: concurrent.futures registers its pool + shutdown through threading._register_atexit, which joins the + worker threads during threading._shutdown, and that runs before + atexit handlers do. So a MultiCam worker has already finished + by the time _cleanup_all runs at exit. A daemon thread of the + host's own is not joined that way and could still be inside + shoot() — but that is the host's thread, not one this library + started. + + A plain lock is still not an obvious fix for the signal case, + because close() also runs at interpreter shutdown, where a lock held by a thread being torn down would turn a clean exit into a hang. It is an open design question, not a solved one. """ diff --git a/src/pychdk/multicam.py b/src/pychdk/multicam.py index dfd4358..b1bf20c 100644 --- a/src/pychdk/multicam.py +++ b/src/pychdk/multicam.py @@ -46,8 +46,9 @@ def shoot(self, **kwargs): What the entries are depends on the path taken. With stream=True each is the JPEG bytes that camera sent back. Otherwise each camera shoots to its own SD card and every - entry is None — nothing is downloaded, because this library - has no path that fetches a card image. + entry is None — nothing is downloaded. ChdkDevice.download_file + will fetch a card file by path; what is missing is any way to + learn the paths the shots just taken were written to. """ with concurrent.futures.ThreadPoolExecutor( max_workers=len(self.cameras) diff --git a/tests/test_device.py b/tests/test_device.py index e27487d..1eae3c6 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -693,6 +693,9 @@ def sentinel_sigterm(signum, frame): device._original_sigterm = saved_originals[1] +_UNSET = object() + + class _RecordPlayCamera: """A fake ChdkPTP that answers get_mode() the way CHDK's Lua does. @@ -715,6 +718,11 @@ def __init__(self, in_record=False, obeys=True): self.polls = 0 self._obeys = obeys self.last_capture_chunks = 0 + # Override what the poll answers, for the cases where the camera + # says something other than its state: `answer` pins every poll, + # `answers` is consumed one per poll and then falls back. + self.answer = _UNSET + self.answers = None def execute_script(self, script, *args, **kwargs): if self._obeys and script.startswith("switch_mode_usb("): @@ -727,6 +735,10 @@ def wait_for_script(self, *args, **kwargs): def execute_lua_wait(self, script, timeout=10.0): assert script == "return get_mode()", script self.polls += 1 + if self.answers: + return self.answers.pop(0) + if self.answer is not _UNSET: + return self.answer return self.in_record @@ -776,6 +788,31 @@ def test_a_play_switch_the_camera_never_makes_raises(self, monkeypatch): with pytest.raises(RuntimeError, match="play"): dev.switch_mode("play") + def test_a_poll_with_no_answer_does_not_confirm_play(self, monkeypatch): + """Silence is not an answer of false. + + execute_lua_wait returns None when a script ends without a RET + message, and bool(None) is False - so a poll that came back with + nothing at all used to satisfy a switch to play, because play is + the mode that expects a falsy is_record. A camera that never + answered would have been recorded as confirmed in playback. + """ + fake = _RecordPlayCamera(in_record=True, obeys=False) + fake.answer = None + dev = self._device(fake, monkeypatch) + with pytest.raises(RuntimeError) as excinfo: + dev.switch_mode("play") + assert "None" in str(excinfo.value) + assert fake.polls > 1, "an unanswered poll was taken as an answer" + + def test_a_poll_that_answers_after_silence_is_still_read(self, monkeypatch): + """Retrying a non-answer must not lose a real answer that follows.""" + fake = _RecordPlayCamera(in_record=False) + fake.answers = [None, None, True] + dev = self._device(fake, monkeypatch) + dev.switch_mode("record") + assert fake.polls == 3 + class TestLivePreviewAsksForPixels: """GetDisplayData with no transfer flag sends no pixels at all.""" From 3ff1f230ecbe6907401a8430fc18dcb254fe117c Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Tue, 15 Sep 2026 17:10:08 -0700 Subject: [PATCH 5/9] fix(shoot): send the menu ISO to set_iso_mode, and keep the argument's name The value was running through iso_to_sv96 into set_sv96, both of which work in real sensitivity, while the argument is the number in the camera's own menu. shooting_sv96_market_to_real subtracts a per-camera offset - 69 sv96 units by default, overridable per platform - so real sensitivity sits below the menu number, and sending the menu number as real made the camera about 0.7 of a stop more sensitive than asked, silently. The first draft of this release renamed the argument to real_iso and kept the conversion, on the grounds that the code had never treated the value as a menu number. That was backwards. Captua's UI offers 100/200/400/800/1600 - the camera's own ladder, picked from a dropdown - and CameraConfig.iso is read back from the camera's own iso PTP widget on the DSLR path. It is a menu number and the library should take one. So market_iso keeps its name and goes to set_iso_mode, which for a value of 50 or more selects the nearest entry in the camera's iso_table (shooting_set_iso_mode, core/shooting.c). The camera's table does the work, the per-camera offset never touches this code, and Captua's call site needs no change - the API break is gone with it. set_iso_mode snaps and reports nothing back, so a requested ISO and the one used can differ. Every value our UI sends is already on the ladder, which makes it a no-op for us; B11a should log requested against reported anyway, in case that is untrue on the A2500. The shutter path was checked for the same fault and does not have it: tv96 has no market variant, and set_tv96_direct applies the value as given. --- RELEASE-NOTES-v0.1.3.md | 57 ++++++++++++++++++++++++++++++++--------- src/pychdk/device.py | 36 ++++++++++++++++++-------- src/pychdk/util.py | 5 +++- tests/test_device.py | 31 +++++++++++++++++----- 4 files changed, 99 insertions(+), 30 deletions(-) diff --git a/RELEASE-NOTES-v0.1.3.md b/RELEASE-NOTES-v0.1.3.md index 84e16d9..987ebb0 100644 --- a/RELEASE-NOTES-v0.1.3.md +++ b/RELEASE-NOTES-v0.1.3.md @@ -19,21 +19,14 @@ Pre-1.0, with Captua as the only consumer. There is no compatibility shim and no deprecation period, and the version stays `0.1.3` rather than pretending otherwise. -- **`ChdkDevice.shoot(market_iso=...)` is now `shoot(real_iso=...)`.** The - value was never treated as market ISO. `iso_to_sv96` computes - `96 * log2(ISO / 3.125)`, the APEX96 conversion for *real* sensitivity, and - `set_sv96` wants real sv96 as well. Market ISO — the number in the camera's - own ISO menu — is a different quantity, and the conversion between them is - per-camera, so this library does not do it. The parameter now says which one - it takes. **Callers must check whether the number they were passing was a - market number**; renaming the keyword makes the call fail loudly instead of - silently mis-exposing. Captua's `backend/capture/backends/chdk_backend.py` - passes `market_iso=` and will need updating with the pointer bump. - **`shoot(download_after=...)` and `shoot(remove_after=...)` are gone.** Neither was implemented: `download_after` listed `A/DCIM`, discarded the listing and returned `None`; `remove_after` did nothing at all. They were removed rather than implemented, so the documentation is true now. Whether the card path ever needs building is a bench decision. +- **`util.iso_to_sv96`'s parameter is renamed** `iso` to `real_iso`, because + that is the quantity it takes. Only matters to a caller passing it by + keyword. `shoot()` no longer calls it at all — see "The ISO argument" below. - **`ChdkDevice.switch_mode` now raises.** A switch that is never confirmed raises `RuntimeError` naming the mode asked for and what `get_mode()` last reported, instead of returning exactly as a success did. A poll that comes @@ -43,8 +36,11 @@ otherwise. `bool(None)` is `False`, a camera that said nothing at all would have been confirmed as being in play mode. Caught in the review of this release, not on a camera. -- **`util.iso_to_sv96`'s parameter is renamed** `iso` to `real_iso`. Only - matters to a caller passing it by keyword. + +`shoot(market_iso=...)` **keeps its name and its callers.** An earlier draft of +this release renamed it to `real_iso`, which would have broken Captua's only +call site for no gain; the real fix turned out to be better than the rename. +See "The ISO argument" below. ## Corrected behaviour @@ -71,6 +67,43 @@ otherwise. `get_display_data(LV_TFR_VIEWPORT)` directly rather than using this generator; that was luck. +### The ISO argument + +`shoot(market_iso=...)` ran the number through `iso_to_sv96` and sent the +result to `set_sv96`. Both of those work in **real** sensitivity, and the +argument is the number in the camera's own ISO menu — a different quantity, +related to real sensitivity by a per-camera offset (`SV96_MARKET_OFFSET`, +defaulting to 69 sv96 units and overridable per platform, `core/shooting.c`). +`shooting_sv96_market_to_real` subtracts that offset, so real sensitivity sits +*below* the menu number: sending the menu number as though it were real made +the camera about 0.7 of a stop **more** sensitive than the operator asked for +(69 of the 96 sv96 units that make a stop), silently. + +The first draft of this release renamed the argument to `real_iso` and left +the conversion alone, on the grounds that the code had never treated the value +as a menu number. That was the wrong way round. Captua's UI offers +`100, 200, 400, 800, 1600` — the camera's own ISO ladder, picked from a +dropdown by an operator — and its `CameraConfig.iso` field is read back from +the camera's own `iso` PTP widget on the DSLR path. The value is a menu +number, and the library should take it as one. + +So `market_iso` keeps its name and now goes to CHDK's `set_iso_mode`, which +for a value of 50 or more finds the nearest entry in the camera's `iso_table` +and selects it (`shooting_set_iso_mode`, `core/shooting.c`). The camera's own +table does the conversion; nothing is computed here, and the per-camera offset +never touches our code. Captua's call site needs no change. + +One consequence to watch on the bench: `set_iso_mode` **snaps** to the nearest +entry and reports nothing back, so a requested ISO and the one actually used +can differ, and neither the library nor Captua will know. For every value our +UI can send this is a no-op, since those values *are* the ladder — but B11a +should log the requested ISO against what the camera reports afterwards, so we +find out if that assumption is wrong on the A2500. + +The shutter path was checked for the same disease and does not have it: `tv96` +is a single real quantity with no market variant, and `set_tv96_direct` +applies the value as given rather than snapping it (`core/shooting.c`). + ## Corrected claims - **`close()` claimed nothing in the library shares a device.** It does. diff --git a/src/pychdk/device.py b/src/pychdk/device.py index 0e57a05..c0c6658 100644 --- a/src/pychdk/device.py +++ b/src/pychdk/device.py @@ -22,7 +22,7 @@ REMOTE_CAP_RAW, REMOTE_CAP_DNG_HDR, ) -from pychdk.util import shutter_to_tv96, iso_to_sv96 +from pychdk.util import shutter_to_tv96 # How long a capture script is allowed to still be starting before an @@ -282,18 +282,33 @@ def lua_execute(self, lua_code, do_return=True, timeout=10.0): self._chdk.execute_script(lua_code) return None - def shoot(self, shutter_speed=None, real_iso=None, dng=False, + def shoot(self, shutter_speed=None, market_iso=None, dng=False, stream=False): """Capture a photo. Args: shutter_speed: Shutter speed in seconds (e.g., 1/100). - real_iso: REAL ISO sensitivity, not the number printed in - the camera's ISO menu. It is converted by iso_to_sv96 - and sent to CHDK's set_sv96, both of which work in real - units; see iso_to_sv96 for why the two quantities are - not interchangeable and why this library will not - convert between them. + Converted to APEX96 and sent with set_tv96_direct, + which applies the value as given rather than snapping + it to one of the camera's own shutter speeds. + market_iso: ISO as it appears in the camera's own menu — + 100, 200, 400 and so on. It goes to CHDK's + set_iso_mode, which for a value of 50 or more finds + the nearest entry in the camera's iso_table and + selects that (shooting_set_iso_mode, core/shooting.c). + So the camera's own table does the work and nothing is + converted here. + + Two consequences worth knowing. A value off the ladder + is SNAPPED, not rejected: asking for 250 on a body + offering 200 and 400 gets one of those, and nothing + reports back which. And this is deliberately not the + set_sv96 path — that one takes real sensitivity, a + different quantity from the menu number, related to it + by a per-camera offset (see util.iso_to_sv96). Passing + a menu number as though it were real sensitivity is a + wrong exposure rather than an error, which is what this + argument used to do. dng: Request DNG. Not implemented on either path: streaming refuses it, and the card path ignores it. stream: If True, use remote capture (direct USB transfer). @@ -309,9 +324,8 @@ def shoot(self, shutter_speed=None, real_iso=None, dng=False, if shutter_speed is not None: tv96 = shutter_to_tv96(shutter_speed) parts.append(f"set_tv96_direct({tv96})") - if real_iso is not None: - sv96 = iso_to_sv96(real_iso) - parts.append(f"set_sv96({sv96})") + if market_iso is not None: + parts.append(f"set_iso_mode({market_iso})") if stream: return self._shoot_streaming(parts, dng) diff --git a/src/pychdk/util.py b/src/pychdk/util.py index e4863f9..fa82b5e 100644 --- a/src/pychdk/util.py +++ b/src/pychdk/util.py @@ -46,7 +46,10 @@ def iso_to_sv96(real_iso): `#if !defined(SV96_MARKET_OFFSET)`, with the comment "Can be overriden in platform_camera.h (see IXUS700 for example)". This library does NOT convert between them: pass a real ISO, or convert - on the camera with CHDK's own functions before calling. + on the camera with CHDK's own functions before calling. That is what + ChdkDevice.shoot does — it takes the menu number and hands it to + set_iso_mode, letting the camera's own iso_table resolve it, and so + it does not call this function at all. Read from the CHDK sources named above. Not measured on a camera. diff --git a/tests/test_device.py b/tests/test_device.py index 1eae3c6..71cc1ab 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -849,7 +849,7 @@ def test_a_caller_can_ask_for_something_else(self): ) -class TestShootTakesRealIsoAndNothingItCannotDo: +class TestShootTakesTheMenuIsoAndNothingItCannotDo: """The signature has to name the quantity, and drop the dead options.""" def _make_device(self): @@ -863,16 +863,35 @@ def _make_device(self): dev = ChdkDevice(info, _usb_device=MagicMock()) return dev, MockChdk.return_value - def test_real_iso_is_converted_with_the_apex_formula(self): + def test_market_iso_goes_to_the_cameras_own_iso_table(self): + """The menu number goes to set_iso_mode, which owns the ladder. + + set_iso_mode with a value of 50 or more picks the nearest entry + in the camera's iso_table (shooting_set_iso_mode, + core/shooting.c), so no conversion happens on this side. + """ + dev, mock_chdk = self._make_device() + dev.shoot(market_iso=400) + script = mock_chdk.execute_script.call_args.args[0] + assert "set_iso_mode(400)" in script + + def test_the_menu_number_is_not_sent_as_real_sensitivity(self): + """set_sv96 takes real sensitivity, which the menu number is not. + + This is the fault the argument used to have: the menu number was + run through iso_to_sv96 and sent to set_sv96, silently setting a + different sensitivity from the one asked for. + """ dev, mock_chdk = self._make_device() - dev.shoot(real_iso=100) + dev.shoot(market_iso=400) script = mock_chdk.execute_script.call_args.args[0] - assert f"set_sv96({iso_to_sv96(100)})" in script + assert "set_sv96" not in script + assert str(iso_to_sv96(400)) not in script - def test_market_iso_is_gone(self): + def test_real_iso_is_gone(self): dev, _ = self._make_device() with pytest.raises(TypeError): - dev.shoot(market_iso=100) + dev.shoot(real_iso=100) def test_download_after_is_gone(self): dev, _ = self._make_device() From 427021d843d72b4c3207b69eabdf27f887bf1d9f Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Tue, 15 Sep 2026 20:28:53 -0700 Subject: [PATCH 6/9] fix(shoot): convert the menu ISO on the camera, keeping script priority Review of the previous commit found the flaw in it. set_iso_mode takes a menu number and lets the camera's iso_table resolve it, which is why it looked right - but it writes the menu property and leaves the script override unset. At capture CHDK applies a script's deferred photo_param_put_off.sv96 first and only falls back to the camera's own configured ISO override when none was set (shooting_expo_param_override_thumb, core/shooting.c). set_sv96 outside a shot populates that deferred value; set_iso_mode does not. So on a card with CHDK's ISO override enabled, the operator's ISO would have been silently beaten by whatever the card was configured with. Emit the whole conversion as CHDK's own Lua instead: set_sv96(sv96_market_to_real(iso_to_sv96(N))) That keeps script-override priority, applies the value exactly rather than snapping to the nearest ladder entry, and still computes nothing here - SV96_MARKET_OFFSET stays the camera's own, per platform. Two tests now separate the two ways this can regress: one pins the emitted chain, the other pins that it is a script override and not a menu write. Also narrow the filename claim, which was still too broad: shoot() does not discover or return the saved filename. download_file fetches by path and CHDK's Lua can be asked where images go; this library just does not ask. --- RELEASE-NOTES-v0.1.3.md | 37 +++++++++++--------- src/pychdk/device.py | 75 +++++++++++++++++++++++++++-------------- src/pychdk/multicam.py | 7 ++-- tests/test_device.py | 38 +++++++++++++++------ 4 files changed, 103 insertions(+), 54 deletions(-) diff --git a/RELEASE-NOTES-v0.1.3.md b/RELEASE-NOTES-v0.1.3.md index 987ebb0..19676d1 100644 --- a/RELEASE-NOTES-v0.1.3.md +++ b/RELEASE-NOTES-v0.1.3.md @@ -87,18 +87,24 @@ dropdown by an operator — and its `CameraConfig.iso` field is read back from the camera's own `iso` PTP widget on the DSLR path. The value is a menu number, and the library should take it as one. -So `market_iso` keeps its name and now goes to CHDK's `set_iso_mode`, which -for a value of 50 or more finds the nearest entry in the camera's `iso_table` -and selects it (`shooting_set_iso_mode`, `core/shooting.c`). The camera's own -table does the conversion; nothing is computed here, and the per-camera offset -never touches our code. Captua's call site needs no change. - -One consequence to watch on the bench: `set_iso_mode` **snaps** to the nearest -entry and reports nothing back, so a requested ISO and the one actually used -can differ, and neither the library nor Captua will know. For every value our -UI can send this is a no-op, since those values *are* the ladder — but B11a -should log the requested ISO against what the camera reports afterwards, so we -find out if that assumption is wrong on the A2500. +So `market_iso` keeps its name, and the conversion moves **onto the camera**: + + set_sv96(sv96_market_to_real(iso_to_sv96(N))) + +Every step is CHDK's own, so the per-camera offset never touches our code and +nothing is computed here. Captua's call site needs no change. + +A second draft used `set_iso_mode(N)`, which also takes a menu number and lets +the camera's `iso_table` resolve it. **That was wrong, and adversarial review +caught it.** At capture, CHDK applies a script's deferred sv96 first and falls +back to the camera's *own configured* ISO override only when none was set +(`shooting_expo_param_override_thumb`, `core/shooting.c:1922-1928`). +`set_sv96` outside a shot populates that deferred value +(`photo_param_put_off.sv96`, `:558`); `set_iso_mode` does not. So on a card +with CHDK's ISO override enabled, `set_iso_mode` would have been silently +beaten and the operator's ISO lost. The chain above keeps script priority +*and* converts honestly. It is also exact, where `set_iso_mode` snaps to the +nearest ladder entry. The shutter path was checked for the same disease and does not have it: `tv96` is a single real quantity with no market variant, and `set_tv96_direct` @@ -138,9 +144,10 @@ applies the value as given rather than snapping it (`core/shooting.c`). camera)".** With the card path that was a list of `None`s. It now states what each path returns. The first rewrite then overshot in the other direction, saying the library "has no path that fetches a card image": - `download_file` does exactly that, by path. What is missing is discovering - the path the shot just taken was written to, and that is what the docstrings - now say. + `download_file` does exactly that, by path. The narrow true claim, which the + docstrings now make, is that **`shoot()` does not discover or return the + saved filename** — CHDK's Lua can be asked where images go, and this library + simply does not ask. - **README, A2500 remote capture — rewritten twice.** The old sentence said the library "falls back to SD card capture (`shoot()`) which triggers the shutter but stores images on the card". It does not fall back at all. The diff --git a/src/pychdk/device.py b/src/pychdk/device.py index c0c6658..1c6a012 100644 --- a/src/pychdk/device.py +++ b/src/pychdk/device.py @@ -292,23 +292,40 @@ def shoot(self, shutter_speed=None, market_iso=None, dng=False, which applies the value as given rather than snapping it to one of the camera's own shutter speeds. market_iso: ISO as it appears in the camera's own menu — - 100, 200, 400 and so on. It goes to CHDK's - set_iso_mode, which for a value of 50 or more finds - the nearest entry in the camera's iso_table and - selects that (shooting_set_iso_mode, core/shooting.c). - So the camera's own table does the work and nothing is - converted here. - - Two consequences worth knowing. A value off the ladder - is SNAPPED, not rejected: asking for 250 on a body - offering 200 and 400 gets one of those, and nothing - reports back which. And this is deliberately not the - set_sv96 path — that one takes real sensitivity, a - different quantity from the menu number, related to it - by a per-camera offset (see util.iso_to_sv96). Passing - a menu number as though it were real sensitivity is a - wrong exposure rather than an error, which is what this - argument used to do. + 100, 200, 400 and so on. It is converted to a real + sensitivity ON THE CAMERA and applied as a script + exposure override: + + set_sv96(sv96_market_to_real(iso_to_sv96(N))) + + Each step is CHDK's own. iso_to_sv96 is the APEX96 + conversion, whose source comment reads "equivalent to + (short)(log2(iso/3.125)*96+0.5) [APEX equation]"; + sv96_market_to_real subtracts SV96_MARKET_OFFSET, which + is per-camera and overridable in platform_camera.h; and + set_sv96 takes the real value. Nothing is converted + here, and the offset never touches this library. + + Two things make this the right call rather than + set_iso_mode, which also takes a menu number: + + First, priority. At capture CHDK applies a script's + deferred sv96 first and falls back to its own + configured ISO override only if none was set + (shooting_expo_param_override_thumb, core/shooting.c). + set_sv96 outside a shot populates that deferred value + (photo_param_put_off.sv96); set_iso_mode does not. On a + card with CHDK's ISO override enabled, set_iso_mode + would be silently overridden and the caller's ISO lost. + + Second, exactness. set_iso_mode snaps to the nearest + entry in the camera's iso_table; this path applies the + value asked for. + + Passing a menu number straight to set_sv96 as though it + were already real — which this argument used to do — + makes the camera about 0.7 of a stop more sensitive + than asked, silently. dng: Request DNG. Not implemented on either path: streaming refuses it, and the card path ignores it. stream: If True, use remote capture (direct USB transfer). @@ -316,16 +333,23 @@ def shoot(self, shutter_speed=None, market_iso=None, dng=False, Returns: The JPEG as bytes when stream=True. Otherwise None: the camera shoots to its own SD card and nothing is fetched - back. download_file will fetch a file off the card by - path; what is missing is any way to learn the path the shot - just taken was written to. + back: shoot() does not discover or return the saved + filename. download_file fetches a card file by path, and + CHDK's own Lua (get_image_dir, the exposure counter, a + directory listing) can be used to find one — none of that + happens here. """ parts = [] if shutter_speed is not None: tv96 = shutter_to_tv96(shutter_speed) parts.append(f"set_tv96_direct({tv96})") if market_iso is not None: - parts.append(f"set_iso_mode({market_iso})") + # Converted on the camera, so the per-camera market-to-real + # offset is the camera's own. See shoot() for why this is + # set_sv96 rather than set_iso_mode. + parts.append( + f"set_sv96(sv96_market_to_real(iso_to_sv96({market_iso})))" + ) if stream: return self._shoot_streaming(parts, dng) @@ -474,10 +498,11 @@ def _shoot_standard(self, setup_parts): remove_after options; neither was implemented — the download listed A/DCIM, threw the listing away and returned None, and the delete did nothing at all — so they were removed rather - than left as a promise. download_file can fetch a card file by - path; the missing piece is discovering the path of the image - this call just took. Whether that is ever needed is a bench - question, not one this library has answered. + than left as a promise. What is missing is narrow: this method + does not discover or return the saved filename. download_file + fetches a card file by path, and CHDK's Lua can be asked where + images go. Whether shoot() should do that for the caller is a + bench question, not one this library has answered. Returns: None. diff --git a/src/pychdk/multicam.py b/src/pychdk/multicam.py index b1bf20c..044a535 100644 --- a/src/pychdk/multicam.py +++ b/src/pychdk/multicam.py @@ -46,9 +46,10 @@ def shoot(self, **kwargs): What the entries are depends on the path taken. With stream=True each is the JPEG bytes that camera sent back. Otherwise each camera shoots to its own SD card and every - entry is None — nothing is downloaded. ChdkDevice.download_file - will fetch a card file by path; what is missing is any way to - learn the paths the shots just taken were written to. + entry is None — shoot() does not discover or return the saved + filenames. ChdkDevice.download_file fetches a card file by + path, and CHDK's own Lua can be asked where images go; neither + happens here. """ with concurrent.futures.ThreadPoolExecutor( max_workers=len(self.cameras) diff --git a/tests/test_device.py b/tests/test_device.py index 71cc1ab..46d3f36 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -863,30 +863,46 @@ def _make_device(self): dev = ChdkDevice(info, _usb_device=MagicMock()) return dev, MockChdk.return_value - def test_market_iso_goes_to_the_cameras_own_iso_table(self): - """The menu number goes to set_iso_mode, which owns the ladder. + def test_the_menu_number_is_converted_on_the_camera(self): + """The whole market-to-real conversion happens in CHDK's own Lua. - set_iso_mode with a value of 50 or more picks the nearest entry - in the camera's iso_table (shooting_set_iso_mode, - core/shooting.c), so no conversion happens on this side. + iso_to_sv96 then sv96_market_to_real then set_sv96, each of them + CHDK's, so the per-camera SV96_MARKET_OFFSET is the camera's own + and nothing is computed on this side. """ dev, mock_chdk = self._make_device() dev.shoot(market_iso=400) script = mock_chdk.execute_script.call_args.args[0] - assert "set_iso_mode(400)" in script + assert "set_sv96(sv96_market_to_real(iso_to_sv96(400)))" in script + + def test_the_iso_is_set_as_a_script_override_not_a_menu_write(self): + """set_sv96 outside a shot is what beats CHDK's own ISO override. + + shooting_expo_param_override_thumb applies a script's deferred + photo_param_put_off.sv96 first and falls back to the camera's + configured ISO override only when none was set (core/shooting.c). + set_sv96 populates that deferred value; set_iso_mode does not, so + a card with ISO override enabled would silently win over the + caller. Emitting set_iso_mode here would be that regression. + """ + dev, mock_chdk = self._make_device() + dev.shoot(market_iso=400) + script = mock_chdk.execute_script.call_args.args[0] + assert "set_iso_mode" not in script def test_the_menu_number_is_not_sent_as_real_sensitivity(self): - """set_sv96 takes real sensitivity, which the menu number is not. + """The market-to-real step is not optional. This is the fault the argument used to have: the menu number was - run through iso_to_sv96 and sent to set_sv96, silently setting a - different sensitivity from the one asked for. + run through iso_to_sv96 and handed straight to set_sv96, which + takes real sensitivity — about 0.7 of a stop more sensitive than + asked, silently. The conversion has to be in the script. """ dev, mock_chdk = self._make_device() dev.shoot(market_iso=400) script = mock_chdk.execute_script.call_args.args[0] - assert "set_sv96" not in script - assert str(iso_to_sv96(400)) not in script + assert "sv96_market_to_real" in script + assert f"set_sv96({iso_to_sv96(400)})" not in script def test_real_iso_is_gone(self): dev, _ = self._make_device() From a1836134e9db181ba7c1cc07f4b1108a69ed2ca4 Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Wed, 16 Sep 2026 07:27:42 -0700 Subject: [PATCH 7/9] docs: narrow six claims a fresh-context review found, and one weak test Two of these described an implementation this release had already replaced. The ISO argument was rewritten three times and util.py and the backend's camera.py were still describing the second version, which used set_iso_mode. util.py now says what it actually is - ISO-to-sv96 scaling, where feeding it a menu number yields a market sv96 and the mistake is treating that as real - and points at shoot(), which converts on the camera and does not call it. The rest turned source agreement into claims about hardware: - "this path applies the value asked for" and "it is also exact" claim achieved exposure. The source establishes conversion, rounding and which write happens; it says nothing about what the sensor delivers, and nobody has measured it. - 69 sv96 units is CHDK's default, not a constant: the IXUS700 platform overrides SV96_MARKET_OFFSET to 20, so the size of the old ISO error varies by body. - "CHDK cancels ... on a transfer error" is broader than the source supports. The reset follows the download timeout and certain chunk-selection errors; the PTP handler does not check what send_data returned, so a host-side transfer failure does not establish it. - the A2500 port is not "alpha-level" in the build we install. camera_list.csv in release-1_6 has a BETA_STATUS column, 34 of its 397 entries carry ALPHA or BETA, and a2500,100a,,, does not. The 0x2002 failure that sentence reported is likewise something we have never observed, so it is named as a thing to test. - "survived three releases with no visible symptom precisely because" is an unverified causal history. What the code shows is that an exhausted loop returned without raising. And the test asserting the ISO is not set as a menu write passed when the ISO was not set at all. It now requires both halves. Release notes: 225 -> 229 tests and six -> eight flasher cases, both wrong; plus the release order, including that v0.1.3 must be tagged on the merged tree and not on the version-bump commit, whose shoot() still took real_iso and would raise TypeError on the only consumer. --- README.md | 2 +- RELEASE-NOTES-v0.1.3.md | 55 +++++++++++++++++++++++++++++++++++------ src/pychdk/chdk.py | 18 ++++++++------ src/pychdk/device.py | 29 +++++++++++++--------- src/pychdk/util.py | 30 ++++++++++++---------- tests/test_device.py | 3 +++ 6 files changed, 96 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index 82f672f..86a820e 100644 --- a/README.md +++ b/README.md @@ -160,7 +160,7 @@ tools/ ## Known limitations -- **Remote capture on A2500**: The A2500 CHDK port is alpha-level, and streaming remote capture (`shoot(stream=True)`) can fail on it with PTP error `0x2002`. The library does not fall back — the exception reaches the caller and nothing else is attempted. Nor does the exception tell you what state the camera is in: the failure can come from the script submission, before `init_usb_capture` has run at all, or from a capture that initialized and was then cancelled. So whether USB remote capture is still enabled afterwards, and whether an ordinary `shoot()` would recover the picture, are both unestablished. Recovery is an open question for the bench, not a documented workaround. +- **Remote capture on A2500 is unverified**: `tools/flash_chdk.py` installs CHDK 1.6.1 r6315 for A2500 firmware 1.00A, and this library's streaming remote capture has never been run against that camera. Earlier versions of this file called the port "alpha-level"; that is not supported by the build we install — `camera_list.csv` in release-1_6 carries a `BETA_STATUS` column, 34 of its 397 entries are marked `ALPHA` or `BETA`, and the A2500's entry (`a2500,100a,,,`) is not one of them. The same sentence reported a `0x2002` failure on this camera; we have no record of observing it, so treat that as a possibility to test rather than a known behaviour. What the code does establish is that if streaming does fail, the library does not fall back — the exception reaches the caller and nothing else is attempted. Nor does the exception tell you what state the camera is in: the failure can come from the script submission, before `init_usb_capture` has run at all, or from a capture that initialized and was then cancelled. So whether USB remote capture is still enabled afterwards, and whether an ordinary `shoot()` would recover the picture, are both unestablished. Recovery is an open question for the bench, not a documented workaround. - **macOS only** for `tools/flash_chdk.py`. The library itself works on macOS and Linux. - **Canon cameras only** — CHDK is Canon-specific. Device discovery defaults to Canon's USB vendor ID (`0x04A9`). diff --git a/RELEASE-NOTES-v0.1.3.md b/RELEASE-NOTES-v0.1.3.md index 19676d1..ceec13f 100644 --- a/RELEASE-NOTES-v0.1.3.md +++ b/RELEASE-NOTES-v0.1.3.md @@ -52,10 +52,11 @@ See "The ISO argument" below. `is_record, is_video, mode` — where `is_record` is `!mode_play` and so is *true* in record mode (`luaCB_get_mode`, `modules/luascript.c`). Since `execute_lua_wait` returns only the first RET value, the library was reading - `is_record` and negating it. The inversion survived three releases with no - visible symptom precisely because the loop fell through and returned on - failure; it now raises, and the comment names the language and all three - return values. + `is_record` and negating it. An exhausted confirmation loop returned + without raising, so a caller could not distinguish an unconfirmed switch + from a confirmed one — which is as much as the code establishes about why + this went unnoticed for three releases. It now raises, and the comment names + the language and all three return values. - **`get_frames` requested no pixel data.** It called `get_display_data()` with the default `flags=0`. CHDK's `live_view_get_data` adds each data block only if the matching `LV_TFR_*` bit was requested (`core/live_view.c`), so a @@ -103,8 +104,10 @@ back to the camera's *own configured* ISO override only when none was set (`photo_param_put_off.sv96`, `:558`); `set_iso_mode` does not. So on a card with CHDK's ISO override enabled, `set_iso_mode` would have been silently beaten and the operator's ISO lost. The chain above keeps script priority -*and* converts honestly. It is also exact, where `set_iso_mode` snaps to the -nearest ladder entry. +*and* converts honestly, and it requests the converted value as an override +rather than selecting the nearest entry in the camera's ISO table. What the +sensor then delivers is not something the source can establish, and nobody +has measured it on one of these bodies. The shutter path was checked for the same disease and does not have it: `tv96` is a single real quantity with no market variant, and `set_tv96_direct` @@ -191,11 +194,34 @@ applies the value as given rather than snapping it (`core/shooting.c`). ## Tests -`.venv/bin/python -m pytest tests/ -q` — 205 passed before, 225 after. New +`.venv/bin/python -m pytest tests/ -q` — 205 passed before, **229** after. New coverage: the `switch_mode` polarity (written from CHDK's sources so that it fails under either inversion, not from what the implementation expects), the raise on an unconfirmed switch, the live-view transfer flags, the removed -`shoot` keywords, and six cases around the flasher's confirmation prompt. +`shoot` keywords, and eight cases around the flasher's confirmation prompt. + +## Releasing this + +The order matters, and one step is a trap: + +1. **Merge this branch.** +2. **Tag `v0.1.3` on the merged tree — not on the `chore(release): 0.1.3` + commit.** That commit bumps the version while `shoot()` still took + `real_iso`, a signature Captua does not call. Tagging it would publish a + release that raises `TypeError` on the only consumer. Tag the final tree. +3. **Bump the backend's pin in all three places together**: `requirements.txt`, + `pixi.toml`, and `pixi.lock` (the git ref appears once per platform plus once + in the package stanza, where the `version:` field also has to move). A + requirements-only bump leaves the Pixi environment on 0.1.2, which is the + environment the appliance actually runs. +4. Only then release the backend. + +`pychdk`'s own dependencies are unchanged between 0.1.2 and 0.1.3 — `pyusb>=1.2.0`, +`pytest>=7.0` for dev, `requires-python >=3.11` — so the lock edit is a ref and +version substitution, not a re-resolution. + +Bumping before the tag exists breaks installation; changing only some pins leaves +the installed environment inconsistent with the declared one. ## A note on how this release was reviewed @@ -207,3 +233,16 @@ the inverted polarity. That is the same failure mode this release exists to correct, reproduced inside the correction. The tests were green throughout. Prose has no tests; the only check on it is someone reading it against the source, twice. + +A further review then read the whole thing from a fresh context, with none of +the earlier framing, and found eleven more. Two were sentences describing an +implementation this release had already replaced — the ISO argument was +rewritten three times, and two docstrings were still describing the second +version. Others turned source agreement into claims about achieved exposure, +stated a per-camera offset as though it were universal (the IXUS700 platform +sets 20, not 69), and repeated an "alpha-level" description of the A2500 that +CHDK's own `camera_list.csv` does not support. One test asserted that the ISO +was not set the wrong way without asserting that it was set at all. + +The reviewer with no shared context found more than the one that had been +following along. That is worth remembering the next time a review looks clean. diff --git a/src/pychdk/chdk.py b/src/pychdk/chdk.py index c217318..99a7c21 100644 --- a/src/pychdk/chdk.py +++ b/src/pychdk/chdk.py @@ -383,14 +383,16 @@ def remote_capture_is_ready(self): remote capture is not initialized NOW, and there are at least two ways to arrive there that this status does not separate: init_usb_capture never ran, or it ran and the capture was - cancelled afterwards. CHDK cancels on its own download timeout - — the set_remotecap_timeout documentation says "following a - timeout, RemoteCaptureIsReady and RemoteCaptureGetData will - behave as if remote capture were not initialized" - (modules/luascript.c) — and also when a transfer errors, both - by way of remotecap_reset clearing the capture target - (core/remotecap.c). So read this as "not initialized", not as - "never initialized". + cancelled afterwards. CHDK clears the capture target after its + own download timeout — the set_remotecap_timeout documentation + says "following a timeout, RemoteCaptureIsReady and + RemoteCaptureGetData will behave as if remote capture were not + initialized" (modules/luascript.c) — and after certain + chunk-selection errors, both by way of remotecap_reset + (core/remotecap.c). A host-side transfer failure on its own + does not establish that the reset happened: the PTP handler + does not check what send_data returned (core/ptp.c). So read + this as "not initialized", not as "never initialized". Returns: Tuple of (is_ready, status). status is REMOTE_CAP_NOTSET diff --git a/src/pychdk/device.py b/src/pychdk/device.py index 1c6a012..379fda3 100644 --- a/src/pychdk/device.py +++ b/src/pychdk/device.py @@ -302,9 +302,10 @@ def shoot(self, shutter_speed=None, market_iso=None, dng=False, conversion, whose source comment reads "equivalent to (short)(log2(iso/3.125)*96+0.5) [APEX equation]"; sv96_market_to_real subtracts SV96_MARKET_OFFSET, which - is per-camera and overridable in platform_camera.h; and - set_sv96 takes the real value. Nothing is converted - here, and the offset never touches this library. + is per-camera and overridable in platform_camera.h + (69 by default, 20 on the IXUS700); and set_sv96 takes + the real value. Nothing is converted here, and the + offset never touches this library. Two things make this the right call rather than set_iso_mode, which also takes a menu number: @@ -318,14 +319,20 @@ def shoot(self, shutter_speed=None, market_iso=None, dng=False, card with CHDK's ISO override enabled, set_iso_mode would be silently overridden and the caller's ISO lost. - Second, exactness. set_iso_mode snaps to the nearest - entry in the camera's iso_table; this path applies the - value asked for. + Second, the table. set_iso_mode selects the nearest + entry in the camera's iso_table; this path requests the + converted sv96 as an override without that lookup. + Whether the sensor then delivers that exposure is not + something the source can establish, and nobody has + measured it. Passing a menu number straight to set_sv96 as though it were already real — which this argument used to do — - makes the camera about 0.7 of a stop more sensitive - than asked, silently. + requested an override SV96_MARKET_OFFSET units above + the corrected request: 69 of the 96 units that make a + stop, on a camera using CHDK's default. The offset is + per-camera (the IXUS700 platform sets 20), so the size + of the old error varies by body. dng: Request DNG. Not implemented on either path: streaming refuses it, and the card path ignores it. stream: If True, use remote capture (direct USB transfer). @@ -465,9 +472,9 @@ def _shoot_streaming(self, setup_parts, dng): "The capture script ended and remote capture is " "not initialized: either init_usb_capture never " "ran, or it ran and the capture was cancelled " - "afterwards — CHDK cancels on its own download " - "timeout and on a transfer error, and reports " - "both the same way as never having initialized" + "afterwards — CHDK clears the capture target " + "after its own download timeout, and reports " + "that the same way as never having initialized" ) if time.monotonic() >= init_deadline: raise RuntimeError( diff --git a/src/pychdk/util.py b/src/pychdk/util.py index fa82b5e..991f9bb 100644 --- a/src/pychdk/util.py +++ b/src/pychdk/util.py @@ -35,21 +35,25 @@ def iso_to_sv96(real_iso): shooting_set_sv96, and its counterpart get_sv96 reads shooting_get_sv96_real (core/shooting.c) — both in real units. - Market ISO — the number in the camera's own ISO menu — is a - different quantity, and passing one here gives a wrong exposure - rather than an error. CHDK keeps the two apart deliberately: it - stores them in separate properties (PROPCASE_SV for real, - PROPCASE_SV_MARKET for market) and exposes iso_market_to_real, - iso_real_to_market, sv96_market_to_real and sv96_real_to_market to - move between them. The offset is per-camera — core/shooting.c + This is arithmetic, not a market-to-real conversion. Feed it a + market ISO — the number in the camera's own ISO menu — and you get + the sv96 for that number; the mistake is then treating that result + as a real sv96, which is what set_sv96 takes. + + CHDK keeps the two quantities apart deliberately: it stores them in + separate properties (PROPCASE_SV for real, PROPCASE_SV_MARKET for + market) and exposes iso_market_to_real, iso_real_to_market, + sv96_market_to_real and sv96_real_to_market to move between them. + The offset is per-camera and is not one number: core/shooting.c defaults SV96_MARKET_OFFSET to 69 sv96 units under `#if !defined(SV96_MARKET_OFFSET)`, with the comment "Can be - overriden in platform_camera.h (see IXUS700 for example)". This - library does NOT convert between them: pass a real ISO, or convert - on the camera with CHDK's own functions before calling. That is what - ChdkDevice.shoot does — it takes the menu number and hands it to - set_iso_mode, letting the camera's own iso_table resolve it, and so - it does not call this function at all. + overriden in platform_camera.h (see IXUS700 for example)" — and the + IXUS700 platform does override it, to 20. + + So this library does not convert between them. Before handing this + result to set_sv96, a market number needs the camera's own + market-to-real step. ChdkDevice.shoot does that on the camera and + therefore does not call this function at all; see its docstring. Read from the CHDK sources named above. Not measured on a camera. diff --git a/tests/test_device.py b/tests/test_device.py index 46d3f36..376601a 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -888,6 +888,9 @@ def test_the_iso_is_set_as_a_script_override_not_a_menu_write(self): dev, mock_chdk = self._make_device() dev.shoot(market_iso=400) script = mock_chdk.execute_script.call_args.args[0] + # Both halves are needed. Without the first this passes when the + # ISO is not set at all, which is the other way to get it wrong. + assert "set_sv96(" in script assert "set_iso_mode" not in script def test_the_menu_number_is_not_sent_as_real_sensitivity(self): From 542db3f9f7aa204838c6039d3eec37c3163b91ef Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Wed, 16 Sep 2026 08:55:32 -0700 Subject: [PATCH 8/9] test(flash): assert which disk is erased, not just which one the prompt named The multi-disk confirmation test checked that the prompt said /dev/disk6 and then threw away what pick_disk returned. Change the return value and leave the prompt alone and the test stays green - which is an erase of a disk the operator never saw, in the one part of this library that can destroy data. Found by mutation, not by reading. It now asserts the returned disk, and a new test follows the chosen disk through main() to format_card, since nothing downstream re-checks that the confirmed disk and the erased disk are the same one. Release notes: the order needs NEH-251 merged first. The final market_iso signature is compatible with the backend's call, so this is not a packaging dependency - but the backend before NEH-251 reads get_mode() with uBASIC's polarity and rejects a camera that reached record mode, so bumping the pin without it ships a corrected library to a caller that still refuses every body. The pin bump also has to carry the backend's test fake, which models 0.1.2's silent switch_mode. --- RELEASE-NOTES-v0.1.3.md | 41 +++++++++++++++++++++++++++++------ tests/test_flash_chdk.py | 47 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 80 insertions(+), 8 deletions(-) diff --git a/RELEASE-NOTES-v0.1.3.md b/RELEASE-NOTES-v0.1.3.md index ceec13f..20a11b2 100644 --- a/RELEASE-NOTES-v0.1.3.md +++ b/RELEASE-NOTES-v0.1.3.md @@ -194,27 +194,39 @@ applies the value as given rather than snapping it (`core/shooting.c`). ## Tests -`.venv/bin/python -m pytest tests/ -q` — 205 passed before, **229** after. New +`.venv/bin/python -m pytest tests/ -q` — 205 passed before, **230** after. New coverage: the `switch_mode` polarity (written from CHDK's sources so that it fails under either inversion, not from what the implementation expects), the raise on an unconfirmed switch, the live-view transfer flags, the removed -`shoot` keywords, and eight cases around the flasher's confirmation prompt. +`shoot` keywords, and nine cases around the flasher's confirmation prompt — the last of which follows the chosen disk through `main()` to `format_card`, because a prompt naming one disk while another is returned would approve an erase nobody saw. ## Releasing this -The order matters, and one step is a trap: +The order matters, and two steps are traps: -1. **Merge this branch.** -2. **Tag `v0.1.3` on the merged tree — not on the `chore(release): 0.1.3` +1. **Merge the backend's record-mode fix (NEH-251) first.** It is not a + dependency of this release in the packaging sense — the final `market_iso` + signature is compatible with the backend's existing call — but the backend + that exists before NEH-251 reads CHDK's `get_mode()` with uBASIC's polarity + and *rejects* a camera that reached record mode. Bumping the pin without it + ships a corrected library to a caller that will still refuse every body. +2. **Merge this branch.** +3. **Tag `v0.1.3` on the merged tree — not on the `chore(release): 0.1.3` commit.** That commit bumps the version while `shoot()` still took `real_iso`, a signature Captua does not call. Tagging it would publish a release that raises `TypeError` on the only consumer. Tag the final tree. -3. **Bump the backend's pin in all three places together**: `requirements.txt`, +4. **Bump the backend's pin in all three places together**: `requirements.txt`, `pixi.toml`, and `pixi.lock` (the git ref appears once per platform plus once in the package stanza, where the `version:` field also has to move). A requirements-only bump leaves the Pixi environment on 0.1.2, which is the environment the appliance actually runs. -4. Only then release the backend. +5. **In that same commit, update the backend's CHDK test fake.** + `tests/unit/chdk_fakes.py` models 0.1.2's `switch_mode`, which returns + whether or not the switch arrived; 0.1.3 raises. The change is not local to + the fake: the four polarity tests pass `stuck_in_play=True` and still call + through `switch_mode()`, so a raising fake stops them reaching the answer + they test. Fake and tests move with the pin. +6. Only then release the backend. `pychdk`'s own dependencies are unchanged between 0.1.2 and 0.1.3 — `pyusb>=1.2.0`, `pytest>=7.0` for dev, `requires-python >=3.11` — so the lock edit is a ref and @@ -246,3 +258,18 @@ was not set the wrong way without asserting that it was set at all. The reviewer with no shared context found more than the one that had been following along. That is worth remembering the next time a review looks clean. + +A second fresh-context pass then found nine more, and this time it mutated the +code instead of reading it. Two tests turned out to pass with the behaviour +they named removed — including the flasher's multi-disk test, which asserted +that the confirmation prompt named the right disk while discarding the disk +`pick_disk` actually returned. Change the return value and the prompt stays +right and the test stays green: an erase of something the operator never saw, +in the one part of this library that can destroy data. That test now asserts +the return value, and a new one follows the chosen disk through `main()` to +`format_card`. + +Three of the sentences it caught had been written during the previous round of +fixing sentences. The lesson this release keeps teaching is not that prose is +unreliable — it is that a claim about a test is as easy to get wrong as a claim +about a camera, and only a mutation tells you which you have. diff --git a/tests/test_flash_chdk.py b/tests/test_flash_chdk.py index 65d1aed..b5aa28f 100644 --- a/tests/test_flash_chdk.py +++ b/tests/test_flash_chdk.py @@ -780,7 +780,11 @@ def test_the_multi_disk_prompt_names_the_disk_it_will_erase( tool = _load_tool() ask = _answerer(["2", "y"]) monkeypatch.setattr("builtins.input", ask) - tool.pick_disk(_disks(3)) + # The returned disk is asserted, not just the prompt. A prompt that + # names one disk while the function returns another would approve an + # erase of something the operator never saw, and is the failure this + # whole class exists to prevent. + assert tool.pick_disk(_disks(3)) == "/dev/disk6" erase = [p for p in ask.prompts if "ERASE" in p] assert erase, ask.prompts assert "/dev/disk6" in erase[0] @@ -835,3 +839,44 @@ def test_main_erases_nothing_when_the_confirmation_is_declined( tool.main() assert excinfo.value.code == 0 assert formatted == [] + + def test_main_erases_exactly_the_disk_that_was_confirmed( + self, monkeypatch, + ): + """The confirmed disk and the erased disk have to be the same one. + + pick_disk names a disk in its prompt and returns a disk, and nothing + downstream re-checks that those agree: format_card erases whatever it + is handed. So this follows the chosen disk all the way from the + selection to the erase, through main(), rather than stopping at the + prompt text. + """ + tool = _load_tool() + formatted = [] + monkeypatch.setattr(tool, "download_chdk", lambda: None) + monkeypatch.setattr(tool, "find_removable_disks", lambda: _disks(3)) + ask = _answerer(["2", "y"]) + monkeypatch.setattr("builtins.input", ask) + monkeypatch.setattr(tool, "_run", _fake_run(PARTITIONED_LAYOUT)) + # The identity salvage and card classification are other tests' + # subject. Stubbed so the only thing this one can fail on is which + # disk reaches format_card. + monkeypatch.setattr( + tool, "read_existing_own_txt", lambda disk: (None, None), + ) + monkeypatch.setattr( + tool, "format_card", lambda disk: formatted.append(disk) or "/Volumes/X", + ) + monkeypatch.setattr(tool, "extract_chdk", lambda *a, **k: None) + monkeypatch.setattr(tool, "patch_boot_sector", lambda *a, **k: None) + monkeypatch.setattr(tool, "get_mount_point", lambda *a, **k: "/Volumes/X") + monkeypatch.setattr(tool, "write_camera_side", lambda *a, **k: None) + monkeypatch.setattr(tool, "eject_card", lambda *a, **k: None) + try: + tool.main() + except SystemExit as exc: + assert exc.code in (0, None), exc.code + erase = [p for p in ask.prompts if "ERASE" in p] + assert erase, ask.prompts + assert "/dev/disk6" in erase[0], erase[0] + assert formatted == ["/dev/disk6"], (formatted, erase) From 2dd6245e61244798c7fb0517464ca13fd3ee1566 Mon Sep 17 00:00:00 2001 From: Juan Cobo Betancourt Date: Wed, 16 Sep 2026 08:56:26 -0700 Subject: [PATCH 9/9] docs: say what the old ISO code requested, not what the sensor did Three copies of the same overclaim, two of which I wrote while fixing the first. "Made the camera 0.7 of a stop more sensitive" is a claim about achieved exposure; the source establishes the conversion and the override that was requested, and nobody has put a meter on one of these bodies. It requested an override about 0.72 stop above the corrected one on a platform using CHDK's default 69-unit offset - which is also the qualifier the earlier fix added to the docstrings and these two missed. And the reset claim: remotecap_reset follows the download timeout and certain chunk-selection errors. A host-side transfer failure does not establish it, because the PTP handler does not check what send_data returned. --- RELEASE-NOTES-v0.1.3.md | 11 ++++++++--- tests/test_device.py | 5 +++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/RELEASE-NOTES-v0.1.3.md b/RELEASE-NOTES-v0.1.3.md index 20a11b2..96948bf 100644 --- a/RELEASE-NOTES-v0.1.3.md +++ b/RELEASE-NOTES-v0.1.3.md @@ -77,8 +77,10 @@ related to real sensitivity by a per-camera offset (`SV96_MARKET_OFFSET`, defaulting to 69 sv96 units and overridable per platform, `core/shooting.c`). `shooting_sv96_market_to_real` subtracts that offset, so real sensitivity sits *below* the menu number: sending the menu number as though it were real made -the camera about 0.7 of a stop **more** sensitive than the operator asked for -(69 of the 96 sv96 units that make a stop), silently. +the old code **request** an override about 0.72 of a stop above the corrected +request — 69 of the 96 sv96 units that make a stop, on a platform using CHDK's +default offset — silently. What the sensor then did with that request is not +something the source establishes, and nobody has measured it. The first draft of this release renamed the argument to `real_iso` and left the conversion alone, on the grounds that the code had never treated the value @@ -139,7 +141,10 @@ applies the value as given rather than snapping it (`core/shooting.c`). reports it identically — "following a timeout, RemoteCaptureIsReady and RemoteCaptureGetData will behave as if remote capture were not initialized" (`set_remotecap_timeout`, `modules/luascript.c`; the mechanism is - `remotecap_reset` in `core/remotecap.c`, also reached on a transfer error). + `remotecap_reset` in `core/remotecap.c`, also reached on certain + chunk-selection errors; a host-side transfer failure alone does not establish + that the reset happened, since the PTP handler does not check what + `send_data` returned). The docstring and the matching `RuntimeError` in `device.py` now say what the status establishes: remote capture is not initialised *now*, for at least two reasons it cannot distinguish. diff --git a/tests/test_device.py b/tests/test_device.py index 376601a..761629e 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -898,8 +898,9 @@ def test_the_menu_number_is_not_sent_as_real_sensitivity(self): This is the fault the argument used to have: the menu number was run through iso_to_sv96 and handed straight to set_sv96, which - takes real sensitivity — about 0.7 of a stop more sensitive than - asked, silently. The conversion has to be in the script. + takes real sensitivity, so the override requested sat about 0.72 of + a stop above the corrected one on a platform using the default + 69-unit offset. The conversion has to be in the script. """ dev, mock_chdk = self._make_device() dev.shoot(market_iso=400)