Adding new cameras, applying patches into trunk (with source code prepared)

reyalp

14117

Re: Adding new cameras, applying patches into trunk (with source code prepared)

« Reply #620 on: 16 / January / 2012, 23:23:50 »

Advertisements

Quote from: Lebeau on 16 / January / 2012, 15:19:21

I am in progress to deliver new code for DNG, RAW, Motion detection and shot histogram.

If you want to make significant changes, I'd suggest a thread to discuss them before posting here. Also, as I've mentioned before, I really prefer that unrelated changes be submitted in individual patches. If the changes are non-trivial, I will probably insist on this.

Quote

First, I start with some C optimizations and minor corrections to test the delivery process.

Did you check if these optimizations improve performance or binary size ?

I had a bit of a look through it, and most of the changes look like they will be insignificant.

Changing the for loops is unnecessary, modern compilers plenty smart to generate good code in the normal version:

Code: [Select]

0014b3cc <foo_old>:
  14b3cc:	b510      	push	{r4, lr}
  14b3ce:	2400      	movs	r4, #0
  14b3d0:	1c20      	adds	r0, r4, #0
  14b3d2:	3401      	adds	r4, #1
  14b3d4:	f7ff fff2 	bl	14b3bc <bar>
  14b3d8:	2c0a      	cmp	r4, #10
  14b3da:	d1f9      	bne.n	14b3d0 <foo_old+0x4>
  14b3dc:	bc10      	pop	{r4}
  14b3de:	bc01      	pop	{r0}
  14b3e0:	4700      	bx	r0

0014b3e2 <foo_new>:
  14b3e2:	b510      	push	{r4, lr}
  14b3e4:	240a      	movs	r4, #10
  14b3e6:	3c01      	subs	r4, #1
  14b3e8:	1c20      	adds	r0, r4, #0
  14b3ea:	f7ff ffe7 	bl	14b3bc <bar>
  14b3ee:	2c00      	cmp	r4, #0
  14b3f0:	d1f9      	bne.n	14b3e6 <foo_new+0x4>
  14b3f2:	bc10      	pop	{r4}
  14b3f4:	bc01      	pop	{r0}
  14b3f6:	4700      	bx	r0

One of these is for(i=0;i<10;i++) and the other is for(i=10;i--;)
Notice they are the exact same number of instructions, and the instructions are equivalent. In the old days, this might have been worthwhile...

Even if there was a marginal improvement, I'd rather not spend my time reviewing and apply stuff that doesn't make any significant difference. If it makes the code clearer or better structured, I'm certainly for that. If doing it in a lot of places makes the binary noticeably smaller, I could go for that too.

Also, the patch has a bunch of unrelated whitespace changes, which makes it a bit harder to follow. These are mostly from trailing whitespace which I'd be happy remove, but preferable in it's own changeset.

Logged

Don't forget what the H stands for.

philmoz

3450

Re: Adding new cameras, applying patches into trunk (with source code prepared)

« Reply #621 on: 17 / January / 2012, 00:55:58 »

Quote from: Lebeau on 16 / January / 2012, 15:19:21

Bonjour,

I am in progress to deliver new code for DNG, RAW, Motion detection and shot histogram.

First, I start with some C optimizations and minor corrections to test the delivery process.

Here is a patch file generated from CHDK shell's source tool "Diff" comparing with 1572 trunk.

In addition to reyalp's comments let me also add that your change of:
for (i = num_stacks - 1; i >= 0; --i)
to:
for (i = num_stacks; i--; )

is, in my opinion, poor programming practice and style.

In the first version it is obvious and transparent what the start and end conditions of the loop are.

In your version it appears at first glance that the starting value of 'i' in the loop is 'num_stacks', when in fact it is 'num_stacks-1'. The end condition also needs to be thought about to work out when the loop will exit.

The parts of the for loop syntax are start value, end condition, and value increment.

Having the end condition change the loop value as a side effect is not intuitive (and would fail code review in any development group I have worked in).

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: Adding new cameras, applying patches into trunk (with source code prepared)

« Reply #622 on: 17 / January / 2012, 22:26:18 »

I've made some changes to the A430 port.
Most of the work was done like almost half a year ago, and it now conflicts(*) with waterwingz's work in the trunk.
This post explains the problems, I hope.
About testing: in the linked thread, I tried to convince Gerhard34 to do some testing, the results are incomplete at the moment.
The changes are:
- histogram, zebra display now correct for replay mode
- raw numbering fixed
- optical data copied from A460 (same 4x zoom optics), updated
- modemap corrected
- some corrections in capt_seq.c
- * most of kbd.c thrown out, the generic one is used
- usb sensing identified, working (for usb remote)
- other small fixes
- promotion to beta status
- * some initialization in the generic kbd.c (it proved to be enough for the A410, A430 for the ptp-script issue)

Patch in the above linked post, don't know whether this can be accepted now.

Logged

waterwingz

12140

Re: Adding new cameras, applying patches into trunk (with source code prepared)

« Reply #623 on: 19 / January / 2012, 00:14:38 »

fix to allow USB V2 remote debug display to not crash the dev trunk build ( adds a startup delay to allow things to settle before hammering the LCD display).

usb_remote_patch_3.patch (0.72 kB - downloaded 75 times.)

Logged

Ported : A1200 SD940 G10 Powershot N G16

reyalp

14117

Re: Adding new cameras, applying patches into trunk (with source code prepared)

« Reply #624 on: 19 / January / 2012, 00:32:41 »

Quote from: waterwingz on 19 / January / 2012, 00:14:38

fix to allow USB V2 remote debug display to not crash the dev trunk build ( adds a startup delay to allow things to settle before hammering the LCD display).

Added, trunk changeset 1578

Logged

Don't forget what the H stands for.

waterwingz

12140

Re: Adding new cameras, applying patches into trunk (with source code prepared)

« Reply #625 on: 19 / January / 2012, 20:03:10 »

Update 3 to USB V2 remote code.

1) Moves pulse counting into the core usb_remote_key() function that is hooked into my_kbd_read_keys().
2) Adds a usb remote mode for playback that scrolls displayed picture on one pulse, reverses direction on 2 pulses, sets direction left on 3 pulses, sets direction right on 4 pulses.
3) Some small code cleanups (more needed).

usb_remote_patch_3_for_trunk_1558.patch (15.25 kB - downloaded 70 times.)

« Last Edit: 19 / January / 2012, 20:10:13 by waterwingz »

Logged

Ported : A1200 SD940 G10 Powershot N G16

waterwingz

12140

Re: Adding new cameras, applying patches into trunk (with source code prepared)

« Reply #626 on: 19 / January / 2012, 22:36:04 »

My last patch post (above) did not "take" for some reason ?? So I did an svn revert & update, reapplied my original file and created a new patch file. Changes are the same as listed above. Let's hope it works this time.

usb_remote_patch_4_for_trunk_1584.patch (15 kB - downloaded 74 times.)

Logged

Ported : A1200 SD940 G10 Powershot N G16

reyalp

14117

Re: Adding new cameras, applying patches into trunk (with source code prepared)

« Reply #627 on: 19 / January / 2012, 22:52:58 »

Quote from: waterwingz on 19 / January / 2012, 22:36:04

My last patch post (above) did not "take" for some reason ?? So I did an svn revert & update, reapplied my original file and created a new patch file. Changes are the same as listed above. Let's hope it works this time.

Added changeset 1585

Logged

Don't forget what the H stands for.

waterwingz

12140

Re: Adding new cameras, applying patches into trunk (with source code prepared)

« Reply #628 on: 20 / January / 2012, 21:44:10 »

Patch #5 for USB remote V2. Changes are :

1) Deleted stubs for future logic control modules that will probably never be implemented (gentWIRE, Script)
2) Changed script function get_usb_power for both Lua and uBasic to take a parameter 0 - 3.
0 = return pulse width of most recent USB power-on time
1 = return current USB state ( 0=disconnected, 1=5V )
2 = return buffered mark / space time of last eight switch activations ( one per call)
3 = return pulse count of switch activations > 100 mSec ( 500 mSec timeout )
Change is backward compatible.
Note that previously uBasic supported mode 0 only, Lua supported 0 or 1.
3) Cleaned up some unneeded parameter passing and commented out lines.
4) Fixed a typo that would not allow debug mode to build.

usb_remote_patch_5_for_trunk_1584.patch (9.49 kB - downloaded 70 times.)

Logged

Ported : A1200 SD940 G10 Powershot N G16

reyalp

14117

Re: Adding new cameras, applying patches into trunk (with source code prepared)

« Reply #629 on: 21 / January / 2012, 02:58:38 »

Quote from: waterwingz on 20 / January / 2012, 21:44:10

Patch #5 for USB remote V2.

Added, changeset 1593

Quote

Change is backward compatible.

I think ubasic is not quite backward compatible.
b=get_usb_power
would be equivalent to mode 0, but now it's a syntax error unless you explicitly give it a number. To make the number optional, you'd have to do something like wait_click_statement()

(the TOKENIZER_ELSE is odd, but I guess it's so you can do if foo else ... ?)

Logged

Don't forget what the H stands for.