pychdk 0.1.3 — make the library tell the truth - #4
Merged
Merged
Conversation
Three of these are behaviour, the rest are sentences that said more than
the code did.
switch_mode polled on uBASIC's convention while speaking Lua. CHDK's Lua
get_mode() returns is_record, is_video, mode, and is_record is
!camera_info.state.mode_play, so it is true in record
(luaCB_get_mode, modules/luascript.c); uBASIC's returns 0 for record, 1
for play, 2 for video record (lib/ubasic/ubasic.c). execute_lua_wait
hands back the first RET value, so the loop was negating is_record and
confirming the opposite of what it set. It now reads the answer the way
CHDK sends it, and an unconfirmed switch raises instead of returning
exactly as a success did - which is why this survived three releases
with no symptom.
get_frames asked for no framebuffer. live_view_get_data adds each block
only if its LV_TFR_ bit was requested (core/live_view.c), so flags=0
returns the header and the framebuffer descriptions and no pixels. It
now asks for the viewport, and the LV_TFR_ constants are named.
market_iso was never market ISO. iso_to_sv96 is the APEX96 conversion
for real sensitivity - CHDK's own shooting_get_sv96_from_iso notes it is
"equivalent to (short)(log2(iso/3.125)*96+0.5) [APEX equation]" - and
set_sv96 wants real. The two quantities are kept apart in CHDK by
separate properties and a per-camera offset, so rename the parameter
rather than invent a conversion we cannot verify.
download_after and remove_after were never implemented: the download
listed A/DCIM, threw the listing away and returned None, the delete did
nothing. Remove them, and say what MultiCam.shoot actually returns.
Narrow four claims to what they establish: close() does reach a shared
device, through the signal handler and the atexit hook, though no lock
is added here because close() runs at interpreter shutdown; drain_
messages is a bounded attempt at fifty; REMOTE_CAP_NOTSET means not
initialized now, since CHDK reports a cancelled capture the same way
("following a timeout, RemoteCaptureIsReady and RemoteCaptureGetData
will behave as if remote capture were not initialized",
set_remotecap_timeout); get_version is the PTP protocol version, not a
firmware build; and nothing falls back after a streaming failure, with
recovery left as an open question rather than a documented workaround.
None of this is checked against a camera. It is read from CHDK's
sources, which is how the original errors got in.
pick_disk asked "This will ERASE the disk. Continue? [y/N]" only when there was exactly one candidate. With two or more it printed the list, took an index and returned the chosen disk before the prompt was ever reached - so the path where picking the wrong disk is most likely was the one path that erased without asking. Selection is not consent, and the two are now separate steps. The prompt names the device node, media name and size rather than "the disk", because find_removable_disks accepts any physical removable medium: on a Mac an external USB drive qualifies, and the operator reading the prompt is the only check on that. A negative index is rejected rather than counted from the end of the list. Correct the safety notes while here: the filter is RemovableMedia plus VirtualOrPhysical, not an Internal check, and an internal SD slot is exactly what we want to flash from.
A release about sentences. Removing download_after/remove_after and renaming market_iso are API breaks; pre-1.0 with Captua as the only consumer, the version stays 0.1.3 rather than pretending otherwise, and the notes lead with the breaks. Captua's chdk_backend passes market_iso= and has to be updated with the pointer bump.
Review of this release found six more, five of them the same fault the
release exists to correct.
The real one: execute_lua_wait returns None when a script ends with no
RET message, and bool(None) is False - so the polarity fix confirmed
play mode on a camera that had said nothing at all. Silence is not an
answer of false. It is retried now, and a poll that answers after a
silence is still read.
The five sentences:
- "no path that fetches a card image" (shoot, _shoot_standard,
MultiCam.shoot). download_file fetches a card file by path. What is
missing is discovering the path of the image a shot just wrote.
- "the library has no call that reports either" (README, on the CHDK
build). lua_execute("return get_buildinfo()") reports it, and
examples/test_camera.py already calls it. There is no dedicated
method; that is a narrower claim and the true one.
- "USB remote capture is still enabled" after a streaming failure
(README). The exception establishes no such thing: it can come from
the script submission before init_usb_capture ran, or from a capture
that initialized and was cancelled on CHDK's own timeout - which this
release's own remote_capture_is_ready note explains.
- "either can close a device while a MultiCam worker is inside shoot()"
(close). The signal handler can; atexit cannot. concurrent.futures
registers its shutdown through threading._register_atexit, joined in
threading._shutdown, which runs before atexit handlers. The signal
hazard and the lock objection both stand without the atexit claim.
- and in the backend's companion fix, a test docstring claiming it
pinned the fake's polarity as well as the code's. It pins the code's;
stuck_in_play bypasses the fake's own switch. A second test pins the
other half.
…s name The value was running through iso_to_sv96 into set_sv96, both of which work in real sensitivity, while the argument is the number in the camera's own menu. shooting_sv96_market_to_real subtracts a per-camera offset - 69 sv96 units by default, overridable per platform - so real sensitivity sits below the menu number, and sending the menu number as real made the camera about 0.7 of a stop more sensitive than asked, silently. The first draft of this release renamed the argument to real_iso and kept the conversion, on the grounds that the code had never treated the value as a menu number. That was backwards. Captua's UI offers 100/200/400/800/1600 - the camera's own ladder, picked from a dropdown - and CameraConfig.iso is read back from the camera's own iso PTP widget on the DSLR path. It is a menu number and the library should take one. So market_iso keeps its name and goes to set_iso_mode, which for a value of 50 or more selects the nearest entry in the camera's iso_table (shooting_set_iso_mode, core/shooting.c). The camera's table does the work, the per-camera offset never touches this code, and Captua's call site needs no change - the API break is gone with it. set_iso_mode snaps and reports nothing back, so a requested ISO and the one used can differ. Every value our UI sends is already on the ladder, which makes it a no-op for us; B11a should log requested against reported anyway, in case that is untrue on the A2500. The shutter path was checked for the same fault and does not have it: tv96 has no market variant, and set_tv96_direct applies the value as given.
Review of the previous commit found the flaw in it. set_iso_mode takes
a menu number and lets the camera's iso_table resolve it, which is why
it looked right - but it writes the menu property and leaves the script
override unset. At capture CHDK applies a script's deferred
photo_param_put_off.sv96 first and only falls back to the camera's own
configured ISO override when none was set
(shooting_expo_param_override_thumb, core/shooting.c). set_sv96 outside
a shot populates that deferred value; set_iso_mode does not. So on a
card with CHDK's ISO override enabled, the operator's ISO would have
been silently beaten by whatever the card was configured with.
Emit the whole conversion as CHDK's own Lua instead:
set_sv96(sv96_market_to_real(iso_to_sv96(N)))
That keeps script-override priority, applies the value exactly rather
than snapping to the nearest ladder entry, and still computes nothing
here - SV96_MARKET_OFFSET stays the camera's own, per platform.
Two tests now separate the two ways this can regress: one pins the
emitted chain, the other pins that it is a script override and not a
menu write.
Also narrow the filename claim, which was still too broad: shoot() does
not discover or return the saved filename. download_file fetches by
path and CHDK's Lua can be asked where images go; this library just
does not ask.
Member
Author
|
Note on review: GitHub Copilot did not review this PR. It stopped responding after roughly fourteen reviews in a day on 2026-09-14 (a quota, not an outage) and is expected to stay out of action, so the merge is not waiting on it. External review here is Codex/Astra only — including a pass from a fresh thread, with no shared context, specifically to check the factual claims against CHDK's source rather than against each other. |
Two of these described an implementation this release had already replaced. The ISO argument was rewritten three times and util.py and the backend's camera.py were still describing the second version, which used set_iso_mode. util.py now says what it actually is - ISO-to-sv96 scaling, where feeding it a menu number yields a market sv96 and the mistake is treating that as real - and points at shoot(), which converts on the camera and does not call it. The rest turned source agreement into claims about hardware: - "this path applies the value asked for" and "it is also exact" claim achieved exposure. The source establishes conversion, rounding and which write happens; it says nothing about what the sensor delivers, and nobody has measured it. - 69 sv96 units is CHDK's default, not a constant: the IXUS700 platform overrides SV96_MARKET_OFFSET to 20, so the size of the old ISO error varies by body. - "CHDK cancels ... on a transfer error" is broader than the source supports. The reset follows the download timeout and certain chunk-selection errors; the PTP handler does not check what send_data returned, so a host-side transfer failure does not establish it. - the A2500 port is not "alpha-level" in the build we install. camera_list.csv in release-1_6 has a BETA_STATUS column, 34 of its 397 entries carry ALPHA or BETA, and a2500,100a,,, does not. The 0x2002 failure that sentence reported is likewise something we have never observed, so it is named as a thing to test. - "survived three releases with no visible symptom precisely because" is an unverified causal history. What the code shows is that an exhausted loop returned without raising. And the test asserting the ISO is not set as a menu write passed when the ISO was not set at all. It now requires both halves. Release notes: 225 -> 229 tests and six -> eight flasher cases, both wrong; plus the release order, including that v0.1.3 must be tagged on the merged tree and not on the version-bump commit, whose shoot() still took real_iso and would raise TypeError on the only consumer.
…pt named The multi-disk confirmation test checked that the prompt said /dev/disk6 and then threw away what pick_disk returned. Change the return value and leave the prompt alone and the test stays green - which is an erase of a disk the operator never saw, in the one part of this library that can destroy data. Found by mutation, not by reading. It now asserts the returned disk, and a new test follows the chosen disk through main() to format_card, since nothing downstream re-checks that the confirmed disk and the erased disk are the same one. Release notes: the order needs NEH-251 merged first. The final market_iso signature is compatible with the backend's call, so this is not a packaging dependency - but the backend before NEH-251 reads get_mode() with uBASIC's polarity and rejects a camera that reached record mode, so bumping the pin without it ships a corrected library to a caller that still refuses every body. The pin bump also has to carry the backend's test fake, which models 0.1.2's silent switch_mode.
Three copies of the same overclaim, two of which I wrote while fixing the first. "Made the camera 0.7 of a stop more sensitive" is a claim about achieved exposure; the source establishes the conversion and the override that was requested, and nobody has put a meter on one of these bodies. It requested an override about 0.72 stop above the corrected one on a platform using CHDK's default 69-unit offset - which is also the qualifier the earlier fix added to the docstrings and these two missed. And the reset claim: remotecap_reset follows the download timeout and certain chunk-selection errors. A host-side transfer failure does not establish it, because the PTP handler does not check what send_data returned.
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
1 participant
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.
Supersedes #3, which folds into this release rather than merging alone — its replacement sentence still overclaimed.
What this is
A release about sentences. An external review of the README and docstrings against CHDK's own sources returned thirteen findings, and almost none were bugs in what the code does — they were bugs in what it says it does. Three needed real code changes; the rest needed the truth written down.
pychdk has no branch protection and no automated review, so three releases went out with a false statement about the library's own behaviour intact. It surfaced only because a review of a different repository asked why the capture path had no fallback.
Behaviour fixed
switch_modeconfirmed the opposite of what it set. Its comment described uBASIC'sget_mode(0 = record); the call is Lua, whereget_mode()returnsis_record, is_video, modeand the first is!mode_play— true in record. It now reads the answer as CHDK sends it, and an unconfirmed switch raises instead of returning exactly as a success did, which is why this survived three releases symptomlessly.get_framesrequested no pixel data.get_display_data()defaulted toflags=0, andlive_view_get_dataadds each block only if itsLV_TFR_*bit was asked for — so the generator could yield frames with no image in them. It now defaults toLV_TFR_VIEWPORT; the constants are named.shoot(market_iso=...)sent a menu number as a real sensitivity.iso_to_sv96andset_sv96both work in real units, andshooting_sv96_market_to_realsubtracts a per-camera offset (69 sv96 units by default), so the camera was being set about 0.7 of a stop more sensitive than asked. The conversion now happens on the camera —set_sv96(sv96_market_to_real(iso_to_sv96(N))), every step CHDK's own — so the per-camera offset never touches our code. The argument keeps its name and Captua's call site needs no change. A draft of this usedset_iso_mode, which also takes a menu number; review caught that it writes the menu property without setting the script override, so on a card with CHDK's own ISO override enabled the operator's ISO would have been silently beaten (shooting_expo_param_override_thumb,core/shooting.c:1922-1928).Claims narrowed to what they establish
close()no longer claims nothing in the library shares a device (the signal handler reaches one a worker is using — thoughatexitdoes not, since executor threads are joined first).drain_messagesis a bounded attempt at fifty, not a drain.REMOTE_CAP_NOTSETmeans "not initialised now", since CHDK reports a cancelled capture identically.MultiCam.shootsays what each path returns. The README no longer promises a fallback that does not exist, and no longer says the library cannot report the CHDK build (get_buildinfo()does, and our own example calls it).Data-safety fix
tools/flash_chdk.pyasked "This will ERASE the disk. Continue? [y/N]" only when there was exactly one candidate. With two or more it took an index and returned the chosen disk before the prompt was reached — so the path where picking wrong is most likely was the one path that erased without asking. Selection and confirmation are now separate steps, the prompt names the specific disk, and a negative index is rejected rather than counted from the end.This is the only change here that can destroy someone's data.
Breaking changes
download_after/remove_afterare gone — neither was implemented — andutil.iso_to_sv96's parameter is renamed toreal_iso. Pre-1.0 with Captua as the only consumer, so the version stays0.1.3rather than pretending compatibility.RELEASE-NOTES-v0.1.3.mdleads with the breaks.On the review this went through
205 → 228 tests. Adversarial review after the first pass was green found six more: five sentences still claiming more than the code or the cited source established, and one real fault introduced by the polarity fix —
execute_lua_waitreturnsNonewhen a script ends with no RET message, andbool(None)isFalse, so a camera that said nothing at all was confirmed as being in play mode. Inverting a condition changes what happens to every value that is neither true nor false.That is the same failure mode this release exists to correct, reproduced inside the correction, with the suite green throughout.
None of this is verified against hardware. Every correction is reasoned from CHDK's sources — which is exactly how the original errors got in. A green gate means "consistent with what we now believe", not "correct".