Adding new cameras, applying patches into trunk (with source code prepared) - page 162 - General Discussion and Assistance - CHDK Forum supplierdeeply

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

  • 1627 Replies
  • 454679 Views
*

Offline philmoz

  • *****
  • 3138
    • Photos
Advertisements
I wondered if stack size might be a concern, since the header is moderately large (36 bytes per buffer desc, plus 32 for header), but PTPSession seems to get a pretty generous size: 0x1000 on a540, 0x1800 on g7x and the stock PTP handlers are pretty complicated.


I considered that; but after testing it did not seem to be an issue.
It could be a static buffer; but I thought that was just wasting memory needlessly. Another option would be to malloc the buffer only once and not free it (or free it in the ptp code when the session is closed); but that was more complex. If it works on the A540 then it's probably safe to use the stack.


I missed a couple of initialisations when sections are not being sent - updated patch attached.
If you're happy with this I'll check it in.

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)

*

Offline reyalp

  • ******
  • 12217
I missed a couple of initialisations when sections are not being sent - updated patch attached.
If you're happy with this I'll check it in.
Seems fine to me.
Don't forget what the H stands for.

*

Offline srsa_4c

  • ******
  • 4057
Re: Adding new cameras, applying patches into trunk (with source code prepared)
« Reply #1612 on: 15 / November / 2019, 18:42:55 »
Spytask dereferences a NULL pointer during its startup.
gui_init() -> gui_set_mode() -> the gui_mode pointer is NULL initially

Does the following patch look good enough to fix this?
Code: [Select]
Index: core/gui.c
===================================================================
--- core/gui.c (revision 5293)
+++ core/gui.c (working copy)
@@ -2401,7 +2401,7 @@
 }
 
 //-------------------------------------------------------------------
-static gui_handler *gui_mode=0; // current gui mode. pointer to gui_handler structure
+static gui_handler *gui_mode = &altGuiHandler; // current gui mode. pointer to gui_handler structure
 
 static int gui_osd_need_restore = 0;    // Set when screen needs to be erase and redrawn
 static int gui_mode_need_redraw = 0;    // Set if current mode needs to redraw itself
I just pointed to one of the existing gui_handler instances, thought the pointer gets updated immediately anyway.

*

Offline reyalp

  • ******
  • 12217
Re: Adding new cameras, applying patches into trunk (with source code prepared)
« Reply #1613 on: 19 / November / 2019, 21:45:36 »
Spytask dereferences a NULL pointer during its startup.
gui_init() -> gui_set_mode() -> the gui_mode pointer is NULL initially

Does the following patch look good enough to fix this?
Code: [Select]
Index: core/gui.c
===================================================================
--- core/gui.c (revision 5293)
+++ core/gui.c (working copy)
@@ -2401,7 +2401,7 @@
 }
 
 //-------------------------------------------------------------------
-static gui_handler *gui_mode=0; // current gui mode. pointer to gui_handler structure
+static gui_handler *gui_mode = &altGuiHandler; // current gui mode. pointer to gui_handler structure
Without testing/digging too much, I think I'd use defaultGuiHandler, since the camera normally starts in non-alt mode. Or figure out where the initialized value is used.  Or adjust gui_set_mode to deal with null old_mode.
Don't forget what the H stands for.


*

Offline srsa_4c

  • ******
  • 4057
Re: Adding new cameras, applying patches into trunk (with source code prepared)
« Reply #1614 on: 20 / November / 2019, 12:16:57 »
I think I'd use defaultGuiHandler
That would not work very well because gui_init() would fail to initialize the gui mode.
Quote
Or adjust gui_set_mode to deal with null old_mode.
I tried that first and failed due to my insufficient knowledge about the inner workings of the CHDK GUI. Also, any change there would lead to an increase in core size.

Using &altGuiHandler as initial value of that pointer would mean no increase in core size. This does not mean CHDK would start in ALT mode, that struct would only provide values that (IMO) allow a successful transition to default GUI mode.
I don't think I see any related glitches on the M100.
Currently, the first few words at address zero are used as initial value when members of that struct are accessed.

*

Offline reyalp

  • ******
  • 12217
Re: Adding new cameras, applying patches into trunk (with source code prepared)
« Reply #1615 on: 20 / November / 2019, 13:27:27 »
I think I'd use defaultGuiHandler
That would not work very well because gui_init() would fail to initialize the gui mode.
Yeah, that makes sense, I didn't think through the if mode == current mode logic.

Quote
Quote
Or adjust gui_set_mode to deal with null old_mode.
I tried that first and failed due to my insufficient knowledge about the inner workings of the CHDK GUI. Also, any change there would lead to an increase in core size.

Using &altGuiHandler as initial value of that pointer would mean no increase in core size. This does not mean CHDK would start in ALT mode, that struct would only provide values that (IMO) allow a successful transition to default GUI mode.
I don't think I see any related glitches on the M100.
Currently, the first few words at address zero are used as initial value when members of that struct are accessed.
If it works, that's good enough for me. It should be better than the current behavior. My initial reaction was that I'd prefer not to have it initialized in an inconsistent state (that is gui_mode is "alt" but various other gui and alt state variables initialized as non-alt).

FWIW, I'm not at all concerned about increasing core size by a few hundreds of bytes if it makes things clearer or easier to maintain.

One thing that might thinking about more is if any of the gui_mode stuff might be accessed in other tasks before gui_init is run, since that happens quite late after the filesystem is ready and CHDK settings have been initialized.
The fact that your solution works on M100 suggests it isn't on that camera. One possible exception is cameras with touch screen support (n, ixus310, ixus240) which reference it in the touch task. I doubt your change will hurt those, but it would probably be better to have chdk_process_touch immediately return if gui_init hasn't run yet. I don't think this needs to be done to check your change in.

Startup scripts are run from spytask after gui_init, so they shouldn't be a concern.
Don't forget what the H stands for.

*

Offline philmoz

  • *****
  • 3138
    • Photos
Re: Adding new cameras, applying patches into trunk (with source code prepared)
« Reply #1616 on: 20 / November / 2019, 15:03:07 »
I think I'd use defaultGuiHandler
That would not work very well because gui_init() would fail to initialize the gui mode.
Yeah, that makes sense, I didn't think through the if mode == current mode logic.

Quote
Quote
Or adjust gui_set_mode to deal with null old_mode.
I tried that first and failed due to my insufficient knowledge about the inner workings of the CHDK GUI. Also, any change there would lead to an increase in core size.

Using &altGuiHandler as initial value of that pointer would mean no increase in core size. This does not mean CHDK would start in ALT mode, that struct would only provide values that (IMO) allow a successful transition to default GUI mode.
I don't think I see any related glitches on the M100.
Currently, the first few words at address zero are used as initial value when members of that struct are accessed.
If it works, that's good enough for me. It should be better than the current behavior. My initial reaction was that I'd prefer not to have it initialized in an inconsistent state (that is gui_mode is "alt" but various other gui and alt state variables initialized as non-alt).

FWIW, I'm not at all concerned about increasing core size by a few hundreds of bytes if it makes things clearer or easier to maintain.

One thing that might thinking about more is if any of the gui_mode stuff might be accessed in other tasks before gui_init is run, since that happens quite late after the filesystem is ready and CHDK settings have been initialized.
The fact that your solution works on M100 suggests it isn't on that camera. One possible exception is cameras with touch screen support (n, ixus310, ixus240) which reference it in the touch task. I doubt your change will hurt those, but it would probably be better to have chdk_process_touch immediately return if gui_init hasn't run yet. I don't think this needs to be done to check your change in.

Startup scripts are run from spytask after gui_init, so they shouldn't be a concern.


After looking at the code it seems the de-reference issue comes from two areas:
- the touch screen code in lines 2460-2461 of gui.c
- keyboard processing on cameras with touch screens where gui_get_mode() may get called before gui_init()


I can't see any other cases - am I missing anything?


The first case is easily fixed with a check for gui_mode == NULL added to the current checks (should set redraw_buttons to 1 in this case).
The second case can be fixed by using camera_info.state.gui_mode instead of gui_get_mode() in the platform code (the IXUS 310 does this). This also removes the need for gui_get_mode altogether.


Setting the initial value of gui_mode will work; but feels wrong IHMO.


Phil.

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)

*

Offline reyalp

  • ******
  • 12217
Re: Adding new cameras, applying patches into trunk (with source code prepared)
« Reply #1617 on: 20 / November / 2019, 15:23:42 »
I can't see any other cases - am I missing anything?
Perhaps stating the obvious, but I assume the ones srsa_4c is hitting is the ones gui_set_mode where it references old_mode->... on 2474 and 2477
It seems like those could just be skipped if old_mode is null, though I'm not immediately sure whether redraw / restore stuff should be called on the first call.

gui_set_mode also returns old_mode, which would be NULL in this case, but the value isn't used in gui_init.
Don't forget what the H stands for.


*

Offline philmoz

  • *****
  • 3138
    • Photos
Re: Adding new cameras, applying patches into trunk (with source code prepared)
« Reply #1618 on: 20 / November / 2019, 16:04:19 »
I can't see any other cases - am I missing anything?
Perhaps stating the obvious, but I assume the ones srsa_4c is hitting is the ones gui_set_mode where it references old_mode->... on 2474 and 2477
It seems like those could just be skipped if old_mode is null, though I'm not immediately sure whether redraw / restore stuff should be called on the first call.

gui_set_mode also returns old_mode, which would be NULL in this case, but the value isn't used in gui_init.


Good point, missed that case.


Setting a default for gui_mode makes more sense now - I'm not fully comfortable with the idea of using the alt mode handler though.
Perhaps a dummy handler struct for startup until gui_init is called might work.


Phil.

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)

*

Offline philmoz

  • *****
  • 3138
    • Photos
Re: Adding new cameras, applying patches into trunk (with source code prepared)
« Reply #1619 on: 20 / November / 2019, 19:10:21 »
I can't see any other cases - am I missing anything?
Perhaps stating the obvious, but I assume the ones srsa_4c is hitting is the ones gui_set_mode where it references old_mode->... on 2474 and 2477
It seems like those could just be skipped if old_mode is null, though I'm not immediately sure whether redraw / restore stuff should be called on the first call.

gui_set_mode also returns old_mode, which would be NULL in this case, but the value isn't used in gui_init.


Good point, missed that case.


Setting a default for gui_mode makes more sense now - I'm not fully comfortable with the idea of using the alt mode handler though.
Perhaps a dummy handler struct for startup until gui_init is called might work.


Phil.


Alternate patch attached.
- Adds a dummy handler for the initial value of gui_mode.
- Remove the gui_get_mode() function in favour of camera_info.state.gui_mode


Phil.

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)

 

Related Topics