USB Remote Switch in CHDK - version 2 implementation thread - page 5 - General Discussion and Assistance

waterwingz

12134

Re: USB Remote Switch in CHDK - tester's needed.

« Reply #40 on: 08 / January / 2012, 19:00:27 »

Advertisements

If possible, I'd like to see a little more testing and user comments before committing this to the dev branch.

If anyone wants to PM me with your camera model and firmware version, I can send you a link to a compiled zip to try or a patch file if you want to look at the actual code ....

Logged

Ported : A1200 SD940 G10 Powershot N G16

waterwingz

12134

Re: USB Remote Switch in CHDK - an open discussion about cleaning up the code

« Reply #41 on: 15 / January / 2012, 20:57:26 »

Posted the first patch for the new release yesterday. Tonight I'm posting the second & final patch file for review by the devs.

Here's the technical details of what's involved :

Notes (ports with non-standard USB code) :

1) Zombie incomplete ports :

   ixusW_sd430, s80

2) cameras using generic/kbd.c ports :

   a410 a530 a540 a610 a620 a630 a640 a700 a710 ixus800_sd700

3) cameras with no code for wait_until_remote_button_is_released() (i.e. no sync) :

   ixus55 ixus60 ixus65 -> no usb support (so get_usb_bit() to always return 0)

   ixus40 ixus50 ixus700 -> has custom usb_power_status_override() routine

   ixus60 ixus65 ixus75 ixus80 ixus700 ixus750 ixuszoom_sd30 sx100is -> normal usb code

5) cameras with empty code in wait_until_remote_button_is_released() (i.e. no sync) :

   s2is s3is -> so get_usb_bit() set to always return 0


platform_camera.h changes :

// added to every file

Code: [Select]

   	#define REMOTE_SYNC_STATUS_LED 	0xC0xxyyyy		// specifies an LED that turns on while camera waits for USB remote to sync

platform/camera/kbd.c changes :

// fixed up this in each port - needs to be intialized or weird things happen prior to <ALT> mode first being entered

Code: [Select]

 				long kbd_new_state[3] = { 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF };
 		static	long kbd_prev_state[3] = { 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF };
		static	long kbd_mod_state[3] = { 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF };

// checked this in each port ( changed all USB_REG to USB_IDX where found) :

Code: [Select]

		#define USB_MASK   (0x12345678)
		#define USB_IDX    2

// deleted from each kbd.c (now in usb_remote.c)

Code: [Select]

		static int usb_power=0;
		static int remote_key, remote_count;
		static int shoot_counter=0;
		#define DELAY_TIMEOUT 10000

// added routine to make USB status bit available outside this file

Code: [Select]

		extern void usb_remote_key( int ) ;
		int get_usb_bit() 
		{
			long usb_physw[3];
			usb_physw[USB_IDX] = 0;
			_kbd_read_keys_r2(usb_physw);
			return(( usb_physw[USB_IDX] & USB_MASK)==USB_MASK) ; 
		}

// deleted wait_until_remote_button_is_released() code

// add to my_kbd_read_keys() to update USB status info

Code: [Select]

	usb_remote_key(physw_status[USB_IDX]) ;
	if (conf.remote_enable) {
		physw_status[USB_IDX] = physw_status[USB_IDX] & ~(SD_READONLY_FLAG | USB_MASK);
	} else {
		physw_status[USB_IDX] = physw_status[USB_IDX] & ~SD_READONLY_FLAG;
	}

platform/generic/kbd.c

   usb code used by a410 a530 a540 a610 a620 a630 a640 a700 a710 ixus800_sd700

core/kbd.c core/kbd.h core/Makefile

   deleted and replaces all existing usb remote code with calls to usb_remote.c

platform/generic/shooting.c

   mods to allow usb remote code to do bracketing based on CHDK setup (but not Canon timer modes needed)

platform/generic/capt_seq.c

   ASM_SAFE wrapper for wait_until_remote_button_is_released

CHDK/LANG/english.lng core/conf.c core/gui.c core/gui_lang.h include/conf.h

   setup & menu changes for new USB remote functionality

usb_remote.c usb_input.c usb_module.c usb_remote.h

   new modular USB remote code files


TODO
1) OSD USB icon to show half press / full press state
2) Handle cameras with seperate   KEY_VIDEO
3) Work on logic modules - new video mode & playback modes
4) Need to scale zoom mode logic for mega zooms with too many steps
5) Implement code for scripts to access buffered pulse / gap data & pulse count data
6) Implement code for scripts to access virtual state of input device (release/half press/full press)

« Last Edit: 15 / January / 2012, 21:39:26 by waterwingz »

Logged

Ported : A1200 SD940 G10 Powershot N G16

srsa_4c

4451

Re: USB Remote Switch in CHDK - an open discussion about cleaning up the code

« Reply #42 on: 15 / January / 2012, 22:04:19 »

@waterwingz
The following may cause side effects:

Code: [Select]

long kbd_new_state[3] = { 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF };
static	long kbd_prev_state[3] = { 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF };
static	long kbd_mod_state[3] = { 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF };

In the German forum I just had an experience with this.
A user with an A495 reported, that he had problems (cam crashed instantly) with any script when it was set to autostart. I prepared a build with this change, he tried it. With the above variables initialized, the camera no longer crashed when a script was automatically starting. The side effect is that on a regular startup, the camera behaves as if it had a long press on its play button (if I understand it correctly). The auto started script might be also affected.
The posts start here (German): http://forum.chdk-treff.de/viewtopic.php?f=12&t=2130&p=24207#p24140 .
On the A410, A430 I didn't notice any weirdness with the above change.
So my suggestion is that this needs testing on as many camera models as possible.

Nice work, btw

Logged

reyalp

13942

Re: USB Remote Switch in CHDK - an open discussion about cleaning up the code

« Reply #43 on: 15 / January / 2012, 22:17:15 »

From a first look through the patch, I like it. Patch applies cleanly and the resulting code builds and runs on my cameras. The result build is actually a tiny bit smaller than the previous trunk

Simple plug/unplug test with remote set to onepush / normal takes a shot on unplugging, twopush works on the second plug.

This is pretty huge, not really sure how to give it a proper review, it would be very difficult to spot something wrong in one of the 100+ kbd.c changes.

I'll try to give more of the code a closer look later. I'm inclined to get this in the trunk quickly, this is what we have an unstable branch for, and from a code cleanup point of view alone it looks like a huge step forward.

I would encourage people who regularly use the remote functionality to test this. I can provide builds if you post your camera/firmware version.

Quote from: srsa_4c on 15 / January / 2012, 22:04:19

@waterwingz
The following may cause side effects:

I had wondered about this. I wonder if it would be safer to initialize these to the existing canon physw_status values once at startup, either when kbd_task starts, or on the first pass through through the main loop (i'm thinking the latter would be better).

edit:
Alternately, you might be able to initialize only the bits that correspond to buttons to 1, but that seems like it would be more work to get all the cameras right.

edit:
It's slightly odd that you can enable the remote with none/none

« Last Edit: 15 / January / 2012, 22:31:38 by reyalp »

Logged

Don't forget what the H stands for.

philmoz

3445

Re: USB Remote Switch in CHDK - an open discussion about cleaning up the code

« Reply #44 on: 15 / January / 2012, 22:21:08 »

Quote from: srsa_4c on 15 / January / 2012, 22:04:19

The side effect is that on a regular startup, the camera behaves as if it had a long press on its play button (if I understand it correctly).

What exactly happens (I can't read German) - a long press on the play button should not do anything special on startup (a long press on the power button should start in record mode).

Phil.

Logged

CHDK ports:
sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
g12 (1.00c, 1.00e, 1.00f & 1.00g)
sx130is (1.01d & 1.01f)
ixus310hs (1.00a & 1.01a)
sx40hs (1.00d, 1.00g & 1.00i)
g1x (1.00e, 1.00f & 1.00g)
g5x (1.00c, 1.01a, 1.01b)
g7x2 (1.01a, 1.01b, 1.10b)

srsa_4c

4451

Re: USB Remote Switch in CHDK - an open discussion about cleaning up the code

« Reply #45 on: 15 / January / 2012, 22:34:29 »

Quote from: philmoz on 15 / January / 2012, 22:21:08

What exactly happens

Quote

Nach dem Einschalten reagiert die Kamera mit einer Funktion des Original-Menüs - so, als ob die [>]-(ALT-,Play-)-Taste lang gedrückt worden wäre. Das ist verwirrend, aber nicht schlimm.

This could be translated like this (I think):

Quote

After switching on the camera, it reacts with a function of the original menu (??), as if the [>] button was long pressed. That is confusing, but not bad.

I think this model uses the [>] button as <ALT>.

Logged

waterwingz

12134

Re: USB Remote Switch in CHDK - an open discussion about cleaning up the code

« Reply #46 on: 15 / January / 2012, 22:38:46 »

Quote from: srsa_4c on 15 / January / 2012, 22:04:19

@waterwingz
The following may cause side effects:
Code: [Select]
long kbd_new_state[3] = { 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF }; static long kbd_prev_state[3] = { 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF }; static long kbd_mod_state[3] = { 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF };

As you might expect, I did not put that there because I wanted to edit 73 or so kdb.c files one at a time.

Without the initialization, the zoom lens move during a USB remote "half shoot". And only if <ALT> mode had not been entered at least once since boot. Took me a while to track down but I figured out early it was probably an initialization issue. Once I added the default values, the problem went away.

I like the idea of loading them with whatever is in the camera when it boots rather than all 0xFFFFFFFF.

Logged

Ported : A1200 SD940 G10 Powershot N G16

srsa_4c

4451

Re: USB Remote Switch in CHDK - an open discussion about cleaning up the code

« Reply #47 on: 15 / January / 2012, 22:42:46 »

@waterwingz
Oops. I misread your post above. Sorry.

« Last Edit: 15 / January / 2012, 22:45:10 by srsa_4c »

Logged

waterwingz

12134

Re: USB Remote Switch in CHDK - an open discussion about cleaning up the code

« Reply #48 on: 15 / January / 2012, 22:45:00 »

Quote from: reyalp on 15 / January / 2012, 22:17:15

I wonder if it would be safer to initialize these to the existing canon physw_status values once at startup, either when kbd_task starts, or on the first pass through through the main loop (i'm thinking the latter would be better).

I'll give that a try - makes some sense. Especially as I have a completely repeatable bug when the initialization is not there.

Quote

It's slightly odd that you can enable the remote with none/none

I was thinking about the interations with scripting. Selecting None for the input allows scripts to have clear access to the USB port (especially if someone modifies the code later and ignores the coding rules I suggested in the comments). Selecting None for the control module is to allow the input driver to control release-halfpress-fullpress independent of the device seleted and for those values to show up in the iskey script code.

Logged

Ported : A1200 SD940 G10 Powershot N G16

philmoz

3445

Re: USB Remote Switch in CHDK - an open discussion about cleaning up the code

« Reply #49 on: 15 / January / 2012, 22:50:33 »

Compiles and works fine on G12, SX30, IXUS310 and IXUS700.

Problem on the SX40 -> USB_IDX != SD_READONLY_IDX so the code in my_kbd_read_keys is not clearing the SD read-only flag correctly. In the original kbd.c code I had to seperate the USB & SD read only settings onto two lines.
Not sure if any other cameras may be like this.

Phil.

Edit, this is what I use on the SX40 - this might be safer code for all cameras.

Code: [Select]

	physw_status[SD_READONLY_IDX] = physw_status[SD_READONLY_IDX] & ~SD_READONLY_FLAG;
	if (conf.remote_enable) {
		physw_status[USB_IDX] = physw_status[USB_IDX] & ~USB_MASK;
	}

« Last Edit: 15 / January / 2012, 22:54:29 by philmoz »

Logged

CHDK ports:
sx30is (1.00c, 1.00h, 1.00l, 1.00n & 1.00p)
g12 (1.00c, 1.00e, 1.00f & 1.00g)
sx130is (1.01d & 1.01f)
ixus310hs (1.00a & 1.01a)
sx40hs (1.00d, 1.00g & 1.00i)
g1x (1.00e, 1.00f & 1.00g)
g5x (1.00c, 1.01a, 1.01b)
g7x2 (1.01a, 1.01b, 1.10b)