supplierdeeply
« previous next »
Pages: 1 ... 58 59 60 61 62 [63] 64 65 66 67 68 ... 160   Go Down

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

  • 1590 Replies
  • 270813 Views
*

Online reyalp

  • ******
  • 11267
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.
*

Offline philmoz

  • *****
  • 3066
    • Photos
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)
*

Offline srsa_4c

  • ******
  • 3601
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
*

Offline waterwingz

  • ******
  • 11471
    • Waterwingz
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).
Logged
Ported :   A1200    SD940   G10    Powershot N    G16
*

Online reyalp

  • ******
  • 11267
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.
*

Offline waterwingz

  • ******
  • 11471
    • Waterwingz
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).
« Last Edit: 19 / January / 2012, 20:10:13 by waterwingz »
Logged
Ported :   A1200    SD940   G10    Powershot N    G16
*

Offline waterwingz

  • ******
  • 11471
    • Waterwingz
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.
Logged
Ported :   A1200    SD940   G10    Powershot N    G16
*

Online reyalp

  • ******
  • 11267
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.
*

Offline waterwingz

  • ******
  • 11471
    • Waterwingz
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.

Logged
Ported :   A1200    SD940   G10    Powershot N    G16
*

Online reyalp

  • ******
  • 11267
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.
Pages: 1 ... 58 59 60 61 62 [63] 64 65 66 67 68 ... 160   Go Up
« previous next »
 

Related Topics