v0.1.1: read against CHDK's protocol before the appliance pins it - #1
Merged
Merged
Conversation
remote_capture_get_data ran a single transaction and threw the response parameters away, so any still that CHDK split across more than one chunk came back truncated with no error to show for it. CHDK's PTP_CHDK_RemoteCaptureGetData handler in core/ptp.c reports the chunk size in param1, a "more chunks follow" flag in param2, and the file position to write at in param3, where -1 means append. PTP parameters are unsigned, so that -1 reaches us as 0xFFFFFFFF and has to be folded back before it is used as an offset. remote_capture_get_chunk now does one transaction and returns the chunk with those three facts decoded; remote_capture_get_data loops it and assembles the image, seeking where the camera asks and appending where it does not. The name, argument and return type are unchanged, so _shoot_streaming needs no change. A chunk ceiling keeps a camera that never clears the flag from spinning forever.
…-242) device.py called signal.signal at import time, and Python raises ValueError: signal only works in main thread of the main interpreter anywhere but the main thread. That made `import pychdk` fail outright for any host whose first import happens on a worker thread, which is exactly what a synchronous FastAPI route does. The two calls move into install_signal_handlers(), which declines off the main thread and also catches the ValueError for embeddings that refuse even on it. The import still calls it and ignores the result, so the common case is unchanged; a host that imported from a worker can call it from its main thread later, which is why it is exported. The originals are captured only when the current handlers are not already ours, so installing twice cannot save _signal_handler as its own predecessor and recurse. atexit stays unconditional; it is thread-safe.
OWN.TXT records a body's page parity — which pages it shoots — not a side of the table; which parity ends up on the left is the operator's reading direction and no business of the file's. The wording everywhere said left and right, so fix that first. The toolkit's CHDK backend also needs a stable identity per body, and pyusb cannot always read a serial from a Canon compact: list_devices hands back serial_num=None often enough that we cannot key on it. So the file gains a second line, id=<hex>, minted when a card is flashed. parse_own_txt and format_own_txt own the format. Parsing is deliberately forgiving — CRLF, a BOM, stray whitespace, blank and unknown lines, lowercase keywords — and never raises, because a card that has been through a Windows editor should still read as the camera it is. The flasher keeps an id already on the card rather than minting a new one, so re-flashing does not silently turn a body into a different camera, and it says which it did.
Cuts the release carrying the chunked remote capture fix (NEH-241), the main-thread signal handler guard (NEH-242) and the camera id in OWN.TXT (NEH-243).
…dlib-only (NEH-243) Reaching pychdk.util used to import the whole package: __init__ pulled in device, device pulls in usb_transport, and usb_transport imports pyusb. So teaching tools/flash_chdk.py to write OWN.TXT through format_own_txt quietly gave the flasher a pyusb dependency it has no use for — util itself imports nothing but math. That matters because the flasher is a card-preparation tool. It runs on whatever Mac is nearest the card reader, before anything else is set up, and the README promises it needs only the standard library. Making that promise true again is worth more than the eager imports. A module-level __getattr__ (PEP 562) now imports the submodule that owns a name on first access and caches the result, so each export costs only its own module. __version__ and __all__ stay literals at the top, every name in __all__ resolves exactly as before, and an unknown name raises the AttributeError Python would have raised anyway.
…(NEH-241) The third response parameter is documented in core/ptp.h as "seek required to pos (-1 = no seek)", and I read -1 as "append". Those are the same thing only until a chunk seeks backwards. Once one does, the current write position is no longer the end of the file, and the next unseeked chunk belongs at the cursor, not tacked onto the end: a camera that sent eight bytes, seeked to 0 to rewrite two, then continued without a seek produced xyCDEFGHzw where the file is xyzwEFGH. So keep the cursor a file would keep. A seek moves it, every chunk is written at it with any forward gap zero-filled, and it advances by the chunk's length. Appending was never the rule; it was the special case of a cursor that happened to sit at the end.
write_camera_side looked for an existing id in OWN.TXT, but main() calls format_card first, and eraseDisk has already taken the file away by then. The preservation branch could only ever fire for a card that had somehow not been formatted, so in the real workflow every re-flash minted a fresh identity — precisely what the docstring and the README promise it will not do. The promise was the only thing keeping it. main() now reads the id off the card before formatting and hands it to write_camera_side, which keeps its own read as a fallback for a card that was not formatted in this run. A card that will not mount, is unformatted, or has no readable OWN.TXT yields no id and no complaint: a blank card is the ordinary case, not an error worth stopping for.
…hide a good one (NEH-243) parse_own_txt took the first nonempty value after an 'id=' and stopped looking. A card whose id line had picked up a bad byte therefore parsed as the replacement character, and the flasher dutifully preserved that as the body's identity while the real id sat on the line below, unread. Being forgiving about the file's shape is right; being forgiving about what an identity looks like is not. An id is now twelve to thirty-two hex characters, and every 'id=' line is examined until one qualifies, so a corrupt line costs nothing. A further valid id is ignored: a file claiming two identities cannot say which body it belongs to, and picking the later one would silently rename a camera. Ids come back lowercased so that no caller has to remember to fold case before comparing.
shoot(dng=True, stream=True) asked the camera for RAW|DNG_HDR and then downloaded a single format off the readiness bitmask. But the DNG flag delivers only the DNG header: the raw data is a second transfer, and the file has to be spliced together on this side. What came back was therefore never a DNG, and nothing said so — it had a plausible size and a plausible header, which is the worst way for a file to be wrong. An operator would have found out at the far end of a scanning session. Refusing is better than half-doing it. The error names what is missing so the next person knows what implementing it involves, and points at the two routes that work today: DNG to the card, or JPEG over USB. Assembling the two transfers is real work and deserves its own change, not a silent guess inside this one.
…NEH-241) core/ptp.h gives RemoteCaptureIsReady three answers in param1, not two: 0 is not ready, 0x10000000 says remote capture was never initialized, and anything else is a bitmask of the data types that are ready. We treated every nonzero status as ready and handed the value straight back as a format, so a camera that had never seen init_usb_capture read as ready with a format of 0x10000000, and the next call asked for a data type that does not exist. What the operator saw was a protocol error from the wrong end of the exchange, or a thirty-second wait for a shot that was never coming — never the one fact that would have helped, which is that the capture was not set up. It now raises and says so. The download request also takes a single bit: the header calls param2 "bit indicating data type to get", while a ready mask can have several set. We ask for the format we requested if the camera has it and the lowest ready bit otherwise. With streamed DNG refused, only JPEG is ever asked for today, so this costs one line and stops the mask being mistaken for a data type if that changes.
An ERR message's param2 is a ptp_chdk_script_error_type, not a ptp_chdk_script_data_type, and the two enums overlap: we read COMPILE as NIL and RUN as BOOLEAN, so every Lua error decoded to None or True and the actual text sat unread in the data phase. What a caller saw was "Script error: None". A syntax error and a missing camera function were indistinguishable, and neither said anything about itself. An ERR now takes its text from the data phase as UTF-8, undecodable bytes replaced and the trailing NUL stripped, since the header promises at least one zero byte even for an empty message. Every other message type decodes exactly as before. execute_lua_wait names the kind alongside the text, so a compile error reads as "Script error (compile): attempt to call a nil value". The namedtuple keeps its field names — renaming data_type would break callers — and now says in its docstring which subtype it is carrying. This matters more than its size suggests: we get one bench session with the cameras, and an error that arrives as None costs a slot in it.
ExecuteScript returns the script id in param1 and a startup status in param2, and we read only the first. A script the camera refused — because it would not compile, or because one was already running and NOKILL was set — came back looking exactly like a script that had started, id and all. execute_lua_wait then polled for a message from it until the timeout ran out and reported that the script had not completed, which is true and useless: it never began. Reading param2 turns a thirty-second wait and a wrong diagnosis into an immediate one that names the kind. The NOKILL case says what 0x1000 means in words rather than leaving a bare code, because that is the one an operator will actually hit: shoot twice in quick succession and the second is refused while the first is still going.
The library is now tagged and pinned by the appliance, so the mocked protocol suite should run on every change rather than only where someone remembers to. It runs on 3.11, the floor pyproject sets and the version the backend pins in pixi.toml, and on 3.12 to catch anything that breaks on a newer interpreter before the appliance moves.
…ill its own setup Streaming sent init_usb_capture and shoot() as two unwaited scripts, and CHDK kills a running script when a new one arrives unless NOKILL is set. So the shot could terminate its own setup. The camera then did what a camera does with no USB capture configured — took an ordinary shot to the card — while we sat in the readiness poll for thirty seconds waiting for bytes that were never coming. It would have looked like a flaky camera, and on a bench with two bodies and one afternoon that is the worst thing for it to look like. One script now carries the setup, the init and the shutter, so nothing can arrive in between, and we wait for it instead of firing and hoping. The script reports the init's result, and only an explicit false is treated as a refusal: an older CHDK returns nothing from init_usb_capture, and a nil that read as failure would break cameras that work fine. Setup parts are joined in the order given, matching _shoot_standard; they were being reversed here for no reason.
Falling back to the lowest ready bit meant a camera offering RAW and a DNG header, but no JPEG, returned raw framebuffer bytes to a caller that had asked for a JPEG — the right number of bytes, the wrong picture, and nothing anywhere saying so. A file like that survives the whole pipeline and is only found later, if at all. A ready mask that lacks the format we asked for is a fault. It now raises, naming both the format requested and the mask that came back, so the next question is answerable from the message alone. The fallback's only defender was the test asserting it, which is replaced.
Answering "skip" — or pressing Enter, or mistyping — returned from write_camera_side without writing anything. By that point main has already erased the card, so the id it went out of its way to rescue beforehand was dropped on the floor at exactly the moment the operator said the least. Declining to choose a parity is not declining to be a camera. The id is now written back whichever way the operator answers, which needed format_own_txt to accept no parity and emit the id line alone; parse_own_txt already read that shape correctly. A card with neither a parity nor an id has nothing to record, so nothing is written. Passing None as the parity used to produce a file whose first line was the word NONE, which parse_own_txt would have read as no parity at all — right answer, wrong reason, and only by luck.
…be read read_existing_camera_id caught OSError and ValueError together and returned None, so a permission failure or a bad read on OWN.TXT was indistinguishable from a card that simply has no id. The card was then erased and given a fresh identity — the exact outcome the id exists to prevent, reached by way of an error nobody saw. Three cases, three answers. A card with no mountable volume is blank, which is what this tool is for, so it says so and carries on. A mounted card with no OWN.TXT genuinely has no id, so it carries on silently. A card whose OWN.TXT is there but unreadable is unknown, and unknown is not empty: it stops with a message saying the card could not be checked and that erasing it would lose the body's identity. Stopping is in read_existing_camera_id itself rather than left to a check in main, so there is no path that formats after an unknown result.
Waiting for the combined script to return before downloading traded one deadlock for another. CHDK can hold the capture pipeline until the host takes the data, so the script does not return until we download and we did not download until it returned. The capture ran out its thirty seconds without asking once whether data was ready — the failure looked identical to a camera that never fired. The script is started rather than waited on, and the loop services the camera while it runs: readiness first, so the data is taken the moment it exists, then the message queue, so an error arrives with its text and a false return is recognized as the refusal it is. A script that ends with nothing waiting and no data means the shot did not happen, which is worth saying immediately instead of spinning to the deadline. The single script stays, since that is what stops the shot from killing its own setup. Messages left over are drained after the download so the next capture does not read a stale one, and a failure draining cannot take away a picture already in hand.
_mounted_path collapsed two different answers into None: a card with nothing on it, and a card whose volume we failed to mount or whose diskutil output we failed to parse. main read None as "blank" and went on to erase, so a reader that dropped out at the wrong moment cost a body its identity — the same failure the id was added to prevent, reached by a different route. The card is now asked what it holds before that conclusion is drawn. diskutil info exits nonzero for a partition that is not there, so a card reporting no filesystem is genuinely blank and formatting one is the point of this tool. A card that reports a volume it will not then let us read is not blank, it is unexamined, and it stops the run with a message saying so. The stop stays inside read_existing_camera_id, so there is still no path that formats after an unknown answer.
Only the id was salvaged before the format, so skipping the parity prompt on a card that already said EVEN wrote back an id and no parity. Skip should mean "leave the assignment as it is", and instead it was the one answer that quietly erased half of it — the operator declines to make a decision and the tool makes one for them. Both fields are now read off the card before it is erased, and both are written back when the prompt is skipped or misanswered. Choosing a parity still overrides whatever the card said, since that is the point of choosing. A card that had no parity to begin with has none to preserve, so the id-only file stays exactly as it was.
wait_for_script watched the running flag and nothing else. A script that started cleanly and then failed cleared that flag exactly like one that succeeded, so _shoot_standard treated a failed shot as a finished one and went looking for a file that was never written. The error stayed in the queue for whichever call read it next, which is how a failure ends up reported against the wrong operation. It now reads waiting messages while it waits and raises on an error with its kind and text, the same way the streaming loop does. _shoot_standard needs no change to benefit: it already waits here. That closes the last path where a script could fail in silence.
remote_capture_is_ready raised the moment CHDK answered 0x10000000, and that was the wrong place to decide. CHDK acknowledges a script as loaded and scheduled, not as run, so the first readiness poll can legitimately arrive before init_usb_capture has executed and get that answer for no reason worse than being early. We raised on it before the loop had looked at the script's status even once — and when the initialization really had failed, its error or its false return was sitting unread in the queue while we reported the status instead. The protocol layer now reports the status as itself, which lets a caller tell "not initialized" from "nothing ready yet", and the decision moves to _shoot_streaming, which is the only place that knows whether the script has had its chance. It treats the status as "not yet" while the script may still be starting, keeps servicing the message queue in the same pass so a real failure is reported in the script's own words, and calls it a failure once the script has ended without initializing or a bounded grace has run out.
_card_holds_a_volume asked diskutil about diskNs1 and drew a conclusion about the whole card from the answer. Both directions were wrong. An existing but unformatted first partition reports fine and then refuses to mount, and a missing diskNs1 proves nothing, since the filesystem may sit on the whole device or on another partition — diskutil info inspects whatever it is pointed at, it does not test for a filesystem. A card we simply failed to inspect was being called blank and erased. It now reads the whole device's layout and judges from every entry. Three outcomes, not two: a layout that positively shows an empty card may be erased, a layout showing any filesystem must be mounted and read first, and anything unreadable or unrecognized is unknown and stops the run. Unknown never falls through to blank, which is the property that matters — the cost of stopping is a rerun, the cost of guessing is a body's identity. The tests parse a real `diskutil list -plist` capture from this machine rather than stubbing the classification. A genuinely blank card cannot be produced here, so its layout is written as the captured structure with the partition map removed, and the test file says so: that one still wants confirming at the bench.
wait_for_script raised on any error in the queue without asking whose it was, and _shoot_standard threw away the id execute_script hands back. Starting a script does not flush the queue, so an error left behind by the previous shot failed the next one — a camera that hiccups once then fails every shot after it, each with the first shot's message. On a bench that reads as a dead camera rather than as one bad frame. wait_for_script now takes the id to match and ignores errors belonging to anything else, and _shoot_standard keeps its id and passes it. The streaming path already matched. A caller with no id to give still matches any error, which is the best that can be done without one. The collection after a successful download was already bounded, by drain_messages' own limit, and already unable to take the picture away, by the try around it. Both now have tests saying so rather than relying on someone rereading the loop: a camera that never stops reporting messages ends the drain, and it ends the shot with the bytes.
The last commit claimed the classification failed closed and it did
not. Blank was whatever was left after looking for a filesystem and not
finding one, so a payload of {} — or one made entirely of keys we do
not know, or one describing a different disk than the one asked about —
came back blank and authorized the erase. Failing to recognize a
filesystem is not the same as recognizing there is none, and the whole
point of the check was the difference.
Blank is now something the payload has to say. It must parse, describe
exactly one entry for the device we asked about, carry the keys real
diskutil output carries, and show that device empty. Everything else —
unreadable, unfamiliar, about another disk, or more entries than we can
account for — is unknown and stops the run. The nested lists are type
checked too, since a payload that is the wrong shape inside is no more
trustworthy than one that is the wrong shape outside.
Giving wait_for_script an id to match fixed _shoot_standard and left switch_mode exactly as it was: it discarded the id execute_script returns and waited without one, so a queued error from an earlier capture failed a mode switch that had started perfectly well. The symptom is worse than the one it was meant to fix, because a failed mode switch looks like a camera that will not go into record at all. switch_mode keeps its id and passes it. These were the only two callers of wait_for_script, so with this one the gap is closed rather than narrowed. The test drives the real waiter with a stale error from another script in the queue, rather than asserting on a mock, since what needed proving was that the waiter ignores it.
Both ways of failing to initialize produced the same sentence, and they are not the same observation. A script that ended while the camera still reported remote capture uninitialized ran and did not initialize. A script still going when our grace expired may be nothing worse than slow. At a bench those want distinguishing from the log alone, without anyone opening this file to work out which branch fired. They now read differently, and the grace one names the seconds it waited, so a camera that just needs longer says so and points at the number to raise. Neither message suggests the camera cannot do remote capture, because neither observation shows that — an operator who concludes the body is unsupported and sets it aside has lost a camera to a message, which on two bodies and one afternoon is half the rig. Also corrects the docstring's account of the post-download drain. It called it collecting the script's result, which it is not: it sweeps what is already queued and never waits for anything still to come.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain, including unsafe mount fallback behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates pychdk to v0.1.1 with CHDK protocol fixes, safer flashing, worker-safe imports, and CI coverage.
Changes:
- Adds chunked capture assembly and improved script/status/error handling.
- Preserves camera identity during flashing and strengthens card safety.
- Adds regression tests, version synchronization, documentation, and Python 3.11/3.12 CI.
File summaries
| File | Summary |
|---|---|
tools/flash_chdk.py |
Preserves IDs and classifies cards; critical finding remains that a fallback mount path can permit erasing an uninspectable card (3 votes). |
tests/test_version.py |
Verifies version consistency. |
tests/test_util.py |
Tests OWN.TXT parsing and formatting. |
tests/test_imports.py |
Tests import isolation; nit remains that the flasher module is not imported by the subprocess (1 vote). |
tests/test_flash_chdk.py |
Tests flasher safety and identity preservation. |
tests/test_device.py |
Tests capture, streaming, readiness, and signal handling. |
tests/test_chdk.py |
Tests protocol statuses, errors, and chunk handling. |
src/pychdk/util.py |
Handles OWN.TXT; moderate finding remains regarding ID-only files conflicting with the parity-first contract (1 vote). |
src/pychdk/device.py |
Coordinates capture and signals; three moderate findings remain concerning handler rollback, standard DNG support, and streaming error details (1, 2, and 1 votes). |
src/pychdk/chdk.py |
Implements CHDK protocol, status, error, and chunk handling. |
src/pychdk/__init__.py |
Provides lazy exports and signal-handler access. |
README.md |
Documents identity and parity; nit remains that the no-parity form is undocumented (1 vote). |
pyproject.toml |
Updates the package version. |
.github/workflows/ci.yml |
Adds continuous integration for Python 3.11 and 3.12. |
Review details
Suppressed comments (5)
README.md:90
- The documented layout is not always what this code writes:
format_own_txt(None, camera_id)intentionally produces an id-only file when parity is skipped, soid=...can be the first and only line rather than a second line after ODD/EVEN. Please document that no-parity form so consumers do not assume line 1 is always a parity.
For book scanning with two cameras, each body is assigned a page parity — which pages it shoots — stored in a file called `OWN.TXT` on the camera's SD card. The first line is `ODD` or `EVEN`; which of the two sits on the left is the operator's reading direction, and the file says nothing about it. A second line, `id=3f9a1c2b7d4e`, gives the body a stable identity, because pyusb cannot always read a serial number from a Canon compact. The Captua workflow reads the file via `download_file('A/OWN.TXT')` to determine page sequencing and EXIF orientation, and to tell one body from the other across replugs. Note the `A/` prefix: `download_file` and `upload_file` both take a card path.
src/pychdk/device.py:133
- If installing SIGINT succeeds but installing SIGTERM raises
ValueError, this returnsFalsewhile leaving SIGINT replaced by_signal_handler. That violates the documented declined/failed result and can unexpectedly close and re-raise signals in an embedding. Roll back any handler installed before returning from this exception path.
signal.signal(signal.SIGINT, _signal_handler)
signal.signal(signal.SIGTERM, _signal_handler)
except ValueError:
# Some embeddings refuse even on the main thread.
return False
src/pychdk/device.py:308
- This streaming error path now has the correct error kind in
msg.data_type, but discards it and reports only the text. That leaves capture failures inconsistent withwait_for_script()andexecute_lua_wait(), and an empty-text failure is still unhelpful; include the decoded error kind here as well.
raise RuntimeError(f"Capture script failed: {msg.value}")
src/pychdk/util.py:130
- When
sideis absent, this deliberately writesid=...as the first line, but the new OWN.TXT contract says the first line is alwaysODDorEVEN(README.md:90 and the PR description). On the skip path a downstream consumer that follows that contract will interpret the ID as parity; preserve the parity-line invariant or update and verify every consumer for ID-only files.
of the table. Each line is written only when there is something to
put on it: an id with no parity is a valid file, since a body keeps
its identity whether or not an operator has decided which pages it
shoots, and parse_own_txt reads it back. With neither, there is
tests/test_imports.py:28
- This subprocess imports only
pychdk.util, nottools/flash_chdk.py, so a direct non-stdlib dependency added to the flasher would still pass. Execute the flasher module in the child (without invokingmain) so the regression test covers the class named byTestFlasherStaysStdlibOnly.
def test_importing_pychdk_util_does_not_pull_in_pyusb(self):
result = _run_import(
"import pychdk.util, sys; print('usb' in sys.modules)"
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
get_mount_point ended with a fallback to /Volumes/<label>, so a disk reporting no mount point still got a path back — and a plausible one, indistinguishable from a real mount. The identity read then opened OWN.TXT under a directory that had never been a card, got the ordinary "not found", and reported a card carrying no id. main erased it. That is the same failure the last two rounds were about, walked in through a different door, and it walked straight past the classification safeguard because it never reported a failure to classify. The helper now refuses: no mount point, or one that is not a directory that exists, is not a mount point. Both callers already tell a failure from a path — _mounted_path turns it into the unknown that stops the identity read unless the card is positively blank, and main exits rather than writing OWN.TXT somewhere that is not the card. The fix belongs here and not in a caller catching more exceptions, because here is where the untrue answer was invented.
The refusal told the caller to capture DNG with stream=False. That path does not capture a DNG: _shoot_standard runs shoot() and never requests one, so following the advice produces whatever the camera was already set to and no error to say otherwise. A refusal that sends someone somewhere useless is worse than a bare refusal, because they go. It now says what is true. Streaming would need the header and the raw data fetched separately and assembled here, which this library does not do, and the card path does not ask for a DNG at all. Neither is implemented and the message no longer implies one of them is. It points at streamed JPEG, which does work. The shoot() docstring said dng captures in DNG raw format, which was the same claim in the same file one line up, so that goes too.
format_card repeated the guess that get_mount_point had just stopped making: when the freshly erased disk reported no mount point it returned /Volumes/<label> anyway. extractall then created that directory on the Mac, DISKBOOT.BIN and CHDK/ both landed in it, both existence checks passed, and the tool printed that it had extracted successfully. The boot sector patch ran against a card with nothing on it and the run finished clean. A silent success that produces a broken card is worse than the failure it replaced, because nothing anywhere says which card it happened to. At the bench it would present as CHDK not working on that body, which is the most expensive thing it could look like: we have no cameras and no cards in front of us until that one session, and a body written off as unsupported is a body we do not get back. get_mount_point now takes mount=False, so format_card asks the same question the same way instead of asking a similar one differently, and there is one place that decides what a mount point is and one that fails when there is not one. Its message says what happened — the card was formatted, it did not come back mounted, nothing was written to it. A format that reports a mount point behaves exactly as before.
A mountable first partition was standing in for having inspected the card. If s1 mounted, read_existing_own_txt read s1/OWN.TXT, found nothing and reported a clean absence — and the classifier we built for exactly this was never consulted, because it only ran when the mount failed. A card holding its identity on a second volume was therefore erased without anything ever looking at that volume. The safeguard was doing nothing in precisely the case it exists for: a card whose shape we did not expect. The layout is now read first and decides. Positively blank proceeds, since there is no identity to lose. Anything unclassifiable already stopped. A card that holds a filesystem may proceed only when its layout is the one this tool can actually read — a single volume, the first partition — and that is settled by the layout data rather than by a mount having happened to succeed. A second volume, or a filesystem on the whole device, stops. Not by inspecting every volume: this tool prepares a card, and reading arbitrary layouts is not its job. Stopping is the honest answer, and an operator who knows the card is safe can erase it themselves. Two tests that reached real diskutil for their layout are stubbed, so they no longer depend on what is plugged into the machine running them.
There was a problem hiding this comment.
🟡 Changes recommended
Critical flasher classification and moderate capture error-reporting issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
README.md:92
format_own_txt(None, camera_id)intentionally emitsid=<...>as the only line when parity is skipped (and a new card can emit no file), so this description's unconditional “first line”/“second line” and “writes both lines” claims are not true for supported cases. Please document the id-only/no-file cases so consumers do not assume the first line is always parity.
src/pychdk/device.py:312
- This streaming error path discards the error kind that
read_script_message()now preserves, so compile and run failures are both reported only asCapture script failed: .... Include the subtype in this exception as well; otherwise the new diagnostic information is lost on the capture path.
if msg.msg_type == MessageType.ERR:
raise RuntimeError(f"Capture script failed: {msg.value}")
tools/flash_chdk.py:410
- These branches silently ignore malformed child entries. A top-level record with
Content="",DeviceIdentifier, andSize, butPartitions=[{}](or a malformed APFS volume), therefore finds no filesystem and is later accepted asCARD_BLANK, somain()can erase a layout this code could not inspect. Treat any unrecognized child entry asCARD_UNKNOWNbefore allowing the blank classification.
for part in list(parts) + list(volumes):
if isinstance(part, dict) and _entry_holds_a_filesystem(part):
return CARD_HAS_FILESYSTEM
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+274
to
+278
| def _entry_holds_a_filesystem(entry: dict) -> bool: | ||
| """Whether one diskutil layout entry describes anything mountable.""" | ||
| if entry.get("Content"): | ||
| return True | ||
| return bool(entry.get("Partitions") or entry.get("APFSVolumes")) |
_entry_holds_a_filesystem treated any non-empty Content as mountable, but on a whole-disk entry Content names the partition map, not a filesystem. So a card carrying a scheme and nothing else — which is every card that has ever been formatted, which is every card an operator is likely to pick up — classified as holding a filesystem, then failed the inspectability check for having no volume to inspect, and the tool refused to flash it. The only shape that reached blank was a device with no Content at all. The bench would have found the flasher unusable on the first card anyone tried. A filesystem is now judged from the nested volumes, by what each says about itself: a Content naming a type, a mount point, or a volume name. All three are needed, because real APFS volumes carry no Content and identify themselves by name and mount point alone. Blank means the entry is a shape we recognize, carries at most a partition map, and lists nothing beneath it. The scheme names are named, so a Content on a whole-disk entry that is not one of them still counts as a filesystem written straight to the device — something present that we have no partition to read. A test caught that on the way through, which is why the scheme list exists rather than the whole-disk Content simply being ignored.
… the error kind Two things the review was right about and I had left inconsistent. The README described OWN.TXT as a first line and a second line and said the flasher writes both. Neither has been true since skipping the parity prompt started producing an identity-only file, and a card with no parity and no id gets no file at all. A reader written from that description would treat the id-only shape as malformed, which is the shape we deliberately created to stop a skipped prompt destroying a body's identity. It now names all three shapes and says a missing parity means unassigned, not broken. The streaming capture path raised "Capture script failed: <text>" and dropped the error kind, so a compile error and a runtime error read identically — undoing, at the one place an operator actually sees, the distinction read_script_message was taught to preserve. It passes the kind through now, as wait_for_script already did.
…ve its claim The README listed three shapes and omitted a parity with no identity, which both parse_own_txt and format_own_txt handle, and it said skipping the prompt produces an identity-only file. Skipping does not do that: it keeps whatever parity the card already had, so a card that said EVEN still says EVEN, and an identity-only file is what comes out of skipping when there was no parity to preserve. Checked against the functions rather than from memory, by running every combination of answer and salvaged state through write_camera_side. That turned up one thing worth writing down: the flasher never writes a parity on its own, because it mints an identity whenever it writes one. Parity-only is a shape the library reads and writes but the tool never produces, so the README says where such a file comes from rather than implying the flasher makes them. The APFS test asserted that a volume is recognized by its name and mount point, and proved nothing: its parent entry carried an Apple_APFS_Container content, which is not a partition scheme, so classification returned at the parent and never reached the nested volume. The parent now carries only a scheme, so the nested entry is the one thing that can decide, and the parent-level case has its own test that says that is what it is testing. A unit test pins the three ways a volume can identify itself.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The toolkit is about to grow a CHDK camera backend (NEH-231), so this library stops being something written alongside Captua and becomes something the appliance depends on. Reading it against CHDK's own
core/ptp.hbefore pinning it turned up eight defects, three of them in the paths a capture actually takes. None was found by running against a camera: there is no hardware available until a single bench session, so every claim here rests on the protocol headers and on tests that stand in for the device.What was wrong
A still larger than one chunk came back truncated, and said it had succeeded (NEH-241). CHDK answers a capture download one chunk at a time, reporting a size, whether more follow, and a position to seek to. The library took the first chunk and discarded those three numbers. It now assembles every chunk at the position CHDK asks for, with a write cursor, because a position of -1 means "no seek", which is not the same as "append" once a chunk has seeked backwards.
A shot could kill its own setup. Initializing remote capture and firing the shutter ran as two separate scripts, and CHDK terminates a running script when a new one starts without the no-kill flag. They are now one script. The first attempt at this waited for that script before downloading, which deadlocks, because the camera can hold the capture pipeline until the host takes the data: the script now runs while the host services readiness and script messages in the same loop.
A ready mask without the requested format returned the wrong picture. Raw framebuffer bytes could come back as the JPEG that was asked for. The requested format is now required.
Importing the package from a worker thread raised (NEH-242). Signal handlers were installed at import, which Python only allows on the main thread, so a host importing from a request handler failed before touching a camera. They install only where they can, and a host can ask for them later.
Every Lua failure read as
Script error: None(NEH-244). For an error message CHDK's second parameter is an error kind, not a data type; decoding it as a data type turned a compile error into nil and threw away the error text sitting in the data phase. Errors now carry their kind and their text.A script that never started looked like it started (NEH-245), so the caller waited out its timeout and reported that instead of the refusal. The startup status is now read. A script that starts and then fails is no longer reported as a clean completion either.
Streaming a DNG returned something that was not a DNG (NEH-246). CHDK sends the header and the raw data as separate transfers for the client to assemble. It now refuses and says so; NEH-246 records the work.
The flasher could take a body's identity away (NEH-243). Cameras are told apart by USB serial, which Canon compacts do not always report, so
OWN.TXTnow carries an id beside the page parity. Along the way: the card was formatted before the id was read, so every re-flash minted a new one; a malformed id line hid a valid one below it; skipping the parity question dropped both fields; and a card whose filesystem could not be inspected was erased anyway.What this does not prove
No camera has run any of this. The chunk protocol, the seek semantics, the readiness statuses and the error kinds are all read from CHDK's headers, and the tests encode that reading rather than the device's behaviour. Bench row B11 is where the A2500 gets to disagree. In particular, remote capture on that body is alpha in CHDK and may simply refuse, which is the risk the toolkit's backend takes deliberately.
Also
OWN.TXT's first line is page parity, ODD or EVEN, not a side of the table: which parity appears on the left is the operator's reading direction, and right-to-left volumes are the same file. The repository gets continuous integration, which it did not have, running the mocked suite on 3.11 and 3.12.Tests: 46 on
main, 134 here.