Menu
▾
▴
[gphoto-patches] [ gphoto-Patches-2819010 ] Bunch of ptp/canon_eos related improvements
|
From: SourceForge.net <no...@so...> - 2009-07-12 22:19:20
|
Patches item #2819010, was opened at 2009-07-09 13:00 Message generated for change (Comment added) made by marcusmeissner You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=308874&aid=2819010&group_id=8874 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: Axel Waggershauser (awagger) Assigned to: Marcus Meissner (marcusmeissner) Summary: Bunch of ptp/canon_eos related improvements Initial Comment: To Marcus and whoever is interested in ptp stuff... The attached set of patches contains: + random-warning-fixes.patch (just what the name says) + remote-release-doc.patch (simple two-liner explaining the EOS_RemoteReleaseOn/Off commands) + 5D-mark-ii-iso-and-fstop.patch (some additional iso and fstop values used by the 5D Mark II) + canon-eos-aeb-and-drive-mode.patch (support for the AEB and DriveMode properties of EOS cameras) + canon-eos-string-and-read-only-int-properties.patch (string properties like Owner/LensName/etc. and two read-only int properties AvailableShots/ShutterCounter) + canon-eos-custom-func-debug-output.patch (cosmetically improve / semi interpret the custom-func props of the 400D) + improve-debugging-output.patch (arguably improving debugging output especially for the "special" EOS-ImageFormat-type of enumerations) These patches might be applicable in any order and in any subset selection. ---------------------------------------------------------------------- >Comment By: Marcus Meissner (marcusmeissner) Date: 2009-07-12 20:48 Message: I applied the 2 new ones. The battery one had to use _() instead of N_(). N_() just marks up the string for translation, but does not translate it instantly. _() does. So N_() is usually used in tables (where the string is not used immediately), _() when it is required translated immediately. ---------------------------------------------------------------------- Comment By: Axel Waggershauser (awagger) Date: 2009-07-12 20:39 Message: Two new patches: + ptp-debugging-output-cosmetic.patch reduce the debugging messages in camera_set_config. it feels a bit more consistent to me now... + canon-eos-battery-property.patch should be obvious (I did not model it as an enumeration since it is inherently 'read-only'. better idea?) ---------------------------------------------------------------------- Comment By: Marcus Meissner (marcusmeissner) Date: 2009-07-10 17:12 Message: i applied both new ones, thanks! no better idea right now. i am cuyrrently working on the generic functions ---------------------------------------------------------------------- Comment By: Axel Waggershauser (awagger) Date: 2009-07-10 16:10 Message: Two new patches: + canon-eos-image-format-property.patch This code works with both 400D and 5DMii, i.e. it is probably general enough to work with every EOS device, as I am not aware of any camera having more ImageFormat options than the 5DMii. It is, however, not very elegant. I opted for the 'one-property' solution as it seemed more consistent and easier to implement. I sort of 'hacked' the custom image format property values into a uint16_t to be able to use the existing infrastructure. This resulted in some 'special' handling - you might have a better idea... + fix-drive-mode-enumeration.patch pretty obvious two-liner Ad generic commands: I did not really like the varargs version either. I'd rather prefer classic c++-style function overloading to be able to omit the n_params parameter. Which is of course only one possible source for trouble. The other (just occured to me) being the fact, that the caller has to know how many parameters the command expects. Now I am convinced, this is bad. You probably meant something like this?!?: #define ptp_canon_eos_requestdevicepropvalue(param, prop) \ ptp_generic_no_data (param, PTP_OC_CANON_EOS_RequestDevicePropValue, 1, prop) I'd appreciate that. P.S. to hfiguiere: you are absolutely right about the spelling of "bunch", thanks :-) ---------------------------------------------------------------------- Comment By: Marcus Meissner (marcusmeissner) Date: 2009-07-10 06:47 Message: as for generic commands. i am thinking about it. I am not sure I like the calling varargs directly solution, I would then use something like #define ptp_canon_eos_requestdevicepropvalue(p,prop) ptp_generic_no_data (p, 1, prop) and then call this. This ensures argument caller safety at least. ---------------------------------------------------------------------- Comment By: Marcus Meissner (marcusmeissner) Date: 2009-07-10 06:40 Message: you can add them there, and we should remove the ones already applied. ---------------------------------------------------------------------- Comment By: Axel Waggershauser (awagger) Date: 2009-07-09 22:38 Message: I started implementing your recommendations and was about half way through... ;-) Regarding your suggestion to implement a ptp_canon_eos_requestdevicepropvalue function. That is what I did first, actually. Then I had to add another two while experimenting with the RemoteReleaseOn/Off commands that looked _very_ similar (obviously) and I am a fan of neither code bloat nor redundancy. Also, the fact, that one sometimes has to check if a given command is available at all (as in this case) and therefore has to know the name of the respective op-code anyway, means one has to know two names for the same thing. Two make a long story short: I am not convinced (yet ;-)) that the approach of having a separate function for each and every command is the best solution. Anyway, there is more to come, like support for the ImageFormat property. Should I just add that here or open a new patch-tracking-ticket? ---------------------------------------------------------------------- Comment By: Marcus Meissner (marcusmeissner) Date: 2009-07-09 20:50 Message: I ported the last two patches to the style I would recommend and applied to both 2.4 branch and TRUNK. Thanks:) ---------------------------------------------------------------------- Comment By: Marcus Meissner (marcusmeissner) Date: 2009-07-09 16:49 Message: i applied: + random-warning-fixes.patch (just what the name says) + remote-release-doc.patch (simple two-liner explaining the EOS_RemoteReleaseOn/Off commands) + 5D-mark-ii-iso-and-fstop.patch (some additional iso and fstop values used by the 5D Mark II) + canon-eos-custom-func-debug-output.patch (cosmetically improve / semi interpret the custom-func props of the 400D) to TRUNK now. Will also do for 2.4 branch later on. canon-eos-string-and-read-only-int-properties.patch: - please define a ptp_canon_eos_requestdevicepropvalue() fuinction in ptp.c/ptp.h and use it. (generic functions are too difficult to handle) - get_INT() should have error return in the default case, and create the widget after the switch() canon-eos-aeb-and-drive-mode.patch: - we cannot pas a UINT32 dpd into the 16bit table function, it will not work on big endian machines. We basically either need to duplicate the 16 -> 32bit table function ... or check if we cannot express the values as 16bit values anyway in ptp-pack.c ---------------------------------------------------------------------- Comment By: Axel Waggershauser (awagger) Date: 2009-07-09 13:07 Message: I forgot to mention that they are based on the current version in the 2_4 svn branch. The only "tested" order of application is the one given in the details above. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=308874&aid=2819010&group_id=8874 |