First (quick) impression: it works, the code looks much better organized now. I copied over YUV support from my version, also works, but it may have to be modified (it will produce 0 byte long files with cameras without explicit yuv support OR when L picture size is selected). I think you can commit this.
Committed in changeset 2135. Please feel free to work in this branch.
edit: there's still some cleanup to do (removed functions' declarations)
Yes, there's still quite a bit to be done.
One thing that i'm not sure about is the
state_shooting_progress=SHOOTING_PROGRESS_DONE; //shoot() needs this...
In remotecap_get_data_chunk... since this can be called when remotecap is reset (on error or init(0)) it would happen at random times in the shooting process. Same for hook_raw_save_complete. I'm not clear exactly what the implications would be, but it seems like there could be problems.
init in Lua, also introduces some more potential synchronization issues, in theory a PTP_CHDK_RemoteCaptureGetData could be running at the same time a script decides to cancel remote capture. I guess the timeout could have the same problem too.
A few other thoughts:
- should the remote capture selection only apply to the next shot, like exposure overrides, or should you need to explicitly cancel it ? If it's left on after the PTP connection goes away, that could confuse users. We don't explicitly know whether there is a PTP connection, but I guess we could check USB power is present (and clear remotecap if it's not found ?).
Setting it once per shot shouldn't be much of a burden if you are using script to shoot, but I guess you could potentially set it once and have the user manually shooting with the shutter ?
Clearing it on script termination (like key presses and script overrides) is probably not good, because you could trigger the shot and have the script end before the data is retrieved (if you use button presses at least, shoot() may wait until everything is done). Users who do want it cleared on script exit could do so in restore().
- should we actually prohibit calling init in playback mode ? Since it just sets up some flags, this doesn't actually cause any harm. You can initialize it and switch to playback mode, so it's still possible to have remotecap enabled in play.
- hook_raw_save_complete() is only called after all the chunks are downloaded, meaning the the raw hook will be blocked until the jpeg is complete. This seems like it might have unwanted impacts, perhaps depending on where the raw hook is on a specific camera.
- The current raw remote capture implementation bypasses raw_savefile() completely, but this function also does some other stuff beyond raw saving, like handling bracketing in continuous mode and shot_histogram. It might be better to remove the remote_capture stuff to raw save_file. This would mean you'd block spytask, which in turn would block capt_seq thought
- The current implementation of remotecap_get_data_chunk requires an extra ptp transaction for each file type, because you only find out you are done by requesting another chunk and getting an empty response. This probably doesn't matter much, but if we can cleanly flag "this is the last chunk" it will save a little time. The status returned by this function could also possibly be made clearer.
