diff --git a/README.md b/README.md index 056376e..86a820e 100644 --- a/README.md +++ b/README.md @@ -36,9 +36,13 @@ 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 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 {major}.{minor}") +print(f"CHDK PTP protocol {major}.{minor}") # Execute Lua on the camera result = cam.lua_execute("return 2 + 2") @@ -156,7 +160,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 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 new file mode 100644 index 0000000..96948bf --- /dev/null +++ b/RELEASE-NOTES-v0.1.3.md @@ -0,0 +1,280 @@ +# 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. + +- **`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 + 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. + +`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 + +- **`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. 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 + 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. + +### 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 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 +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 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, 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` +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. + `_cleanup_all` closes every open device from the main thread, and it runs + 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 + 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 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. +- **`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. 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. 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 + 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. + 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. + `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, **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 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 two steps are traps: + +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. +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. +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 +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 + +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. + +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. + +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/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/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/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", diff --git a/src/pychdk/chdk.py b/src/pychdk/chdk.py index 9d46d45..99a7c21 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,23 @@ 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 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 @@ -498,7 +529,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..379fda3 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, @@ -21,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 @@ -210,21 +211,59 @@ 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. 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. + + 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): + 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 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. @@ -244,33 +283,85 @@ def lua_execute(self, lua_code, do_return=True, timeout=10.0): return None def shoot(self, shutter_speed=None, market_iso=None, dng=False, - stream=False, download_after=False, remove_after=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). + 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 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 + (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: + + 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, 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 — + 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). - 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: 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: - sv96 = iso_to_sv96(market_iso) - parts.append(f"set_sv96({sv96})") + # 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) 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 +460,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 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( @@ -400,21 +498,27 @@ 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. 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. + """ 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 +543,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 +609,28 @@ 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. 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. """ self._connected = False _open_devices.discard(self) diff --git a/src/pychdk/multicam.py b/src/pychdk/multicam.py index 6ca1908..044a535 100644 --- a/src/pychdk/multicam.py +++ b/src/pychdk/multicam.py @@ -42,8 +42,14 @@ 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 — 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/src/pychdk/util.py b/src/pychdk/util.py index e6b49ba..991f9bb 100644 --- a/src/pychdk/util.py +++ b/src/pychdk/util.py @@ -21,18 +21,49 @@ 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. + + 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)" — 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. 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..761629e 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") + + 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.""" + + 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 TestShootTakesTheMenuIsoAndNothingItCannotDo: + """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_the_menu_number_is_converted_on_the_camera(self): + """The whole market-to-real conversion happens in CHDK's own Lua. + + 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_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] + # 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): + """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 handed straight to set_sv96, which + 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) + script = mock_chdk.execute_script.call_args.args[0] + 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() + with pytest.raises(TypeError): + dev.shoot(real_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) diff --git a/tests/test_flash_chdk.py b/tests/test_flash_chdk.py index a61a52b..b5aa28f 100644 --- a/tests/test_flash_chdk.py +++ b/tests/test_flash_chdk.py @@ -710,3 +710,173 @@ 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) + # 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] + 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 == [] + + 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) 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: