hide_osd() script function - page 2 - General Discussion and Assistance - CHDK Forum
« previous next »
Pages: 1 [2] 3 4 5 6 7 ... 10   Go Down

hide_osd() script function

  • 98 Replies
  • 34606 Views
*

Offline lapser

  • *****
  • 1093
Re: hide_osd() script function
« Reply #10 on: 29 / September / 2013, 12:32:32 »
Advertisements
Quote from: waterwingz on 29 / September / 2013, 09:49:46
I've take the liberty to update your patch a bit.  Two principal changes are :

1) Added code to support the function in uBASIC (with a test script).
2) Added an enum for the OSD state.   I get nervous when code has "magic numbers"  like 0,1,2 rather than #defined (or enum) constants - especially when the same magic numbers are used in different source files.

If you are okay with these changes,  and lacking any other feedback,  I'd suggest submitting this patch for inclusion in the 1.3.0 trunk.
Wow, great work! Actually, I was just ready to post my V3 patch, which included the uBasic changes and a uBasic test script. I'm happy to report that except for the enum part, we ended up with the exact same code, and same test script.

I'm OK with the enum changes, if you think that makes it more clear.
Quote
Update : I almost hate to bring this up,  but would it be better to extend this so that you can selectively turn off the <ALT> & script name and/or the OSD icons.  For example, I can see wanting the colored battery icon visible but nothing else.  Or am I complicating this too much?
I thought about doing that, but I decided it wasn't that useful. My time lapse script uses the toggle feature when you press <SET>, so I can see the entire OSD briefly whenever I want, including the battery, for example.

I also show the battery voltage  using drawstring() from my script. I figure anything the script wants to show, it can show on its own with the draw functions, exactly the way it wants. The idea of hide_osd() is to give total control of the screen to the script.

I compiled and tested your patch in uBasic and Lua, and it works great. Thanks again.

Quote from: waterwingz on 29 / September / 2013, 12:17:37
Update 2 : added the new uBASIC function to camera_functions.c and fixed a path error in run_ubasic.c .
I suspect run_ubasic.c  will still not build "as is" with the new modules versions of CHDK unless further work is done.   This has nothing to do with the hide_osd() patch.   Does anybody actually care anymore?
I'm not familiar with this, so I'll let you sort it out, OK? Will you submit the patch for inclusion in 1.3 when you have it ready?
Thanks.
Logged
EOS-M3_120f / SX50_100b / SX260_101a / G1X_100g / D20_100b
https://www.youtube.com/user/DrLapser/videos
*

Offline lapser

  • *****
  • 1093
Re: hide_osd() script function
« Reply #11 on: 29 / September / 2013, 14:43:54 »
Quote from: waterwingz on 29 / September / 2013, 09:49:46
2) Added an enum for the OSD state.   I get nervous when code has "magic numbers"  like 0,1,2 rather than #defined (or enum) constants - especially when the same magic numbers are used in different source files.
I figured out an easier way to clear the screen that doesn't require enum, and is easy to understand:

Code: [Select]
void gui_redraw()
{
    static int need_restore=1;
    if(camera_info.state.osd_off) //set to 1 by script hide_osd() function
    {
      if(need_restore)
      {
        draw_restore(); //clear screen first time
        need_restore=0;
      }
      return;
    }
    need_restore=1;

This also simplifies the script hide_osd() function:
Code: [Select]
//0 show osd, 1 hide osd, -1 toggle
static int luaCB_hide_osd( lua_State* L )
{
  int n=luaL_checknumber(L,1);
  if(n<0)n=(camera_info.state.osd_off==0); //toggle
  camera_info.state.osd_off=(n!=0); //0 show, 1 hide osd
  return 0;
}
I've attached my version of the patch with these changes. I tested it and it works with ubasic and Lua. What do you think?
Logged
EOS-M3_120f / SX50_100b / SX260_101a / G1X_100g / D20_100b
https://www.youtube.com/user/DrLapser/videos
*

Offline waterwingz

  • ******
  • 12140
    • Waterwingz
Re: hide_osd() script function
« Reply #12 on: 29 / September / 2013, 15:48:23 »
Quote from: lapser on 29 / September / 2013, 14:43:54
I figured out an easier way to clear the screen that doesn't require enum, and is easy to understand:
enum's are free - they don't take up code space and tend to make the code easier to follow.

If you think your new code is easier to understand then fine - its your code. I'm personally not a big fan of using the results of comparisons as integer values but I guess it does work.


Logged
Ported :   A1200    SD940   G10    Powershot N    G16
*

Offline philmoz

  • *****
  • 3450
    • Photos
Re: hide_osd() script function
« Reply #13 on: 29 / September / 2013, 16:06:27 »
Quote from: waterwingz on 29 / September / 2013, 15:48:23
Quote from: lapser on 29 / September / 2013, 14:43:54
I figured out an easier way to clear the screen that doesn't require enum, and is easy to understand:
enum's are free - they don't take up code space and tend to make the code easier to follow.

If you think your new code is easier to understand then fine - its your code. I'm personally not a big fan of using the results of comparisons as integer values but I guess it does work.

The enums make the C code easier to understand, and the compiler is smart enough these days that it will make negligible difference to the code size or speed. You could also check against the enum values in the switch statement to make things clearer.

Is 'hide_osd' a suitable name for the function when calling 'hide_osd(1)' does the exact opposite, and 'hide_osd(-1)' could either show or hide the display. It also does not make it clear that the function only affects the CHDK OSD, not the Canon one. Perhaps 'set_chkd_osd_state' might be better.

My other concern is that the 'magic' integer values as parameters to the functions make the script code harder to follow for new users, or if you've forgotten what the magic numbers mean. Not sure what the best solution for this is - having three separate functions seems overkill.

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)
*

Offline waterwingz

  • ******
  • 12140
    • Waterwingz
Re: hide_osd() script function
« Reply #14 on: 29 / September / 2013, 16:18:18 »
Quote from: philmoz on 29 / September / 2013, 16:06:27
You could also check against the enum values in the switch statement to make things clearer.
The switch in my code takes the passed paramter ( -1 , 0  ,1) as its arguement.  Didn't seem to be much point in using an enum there as the calling script code does not have access to that information?  Where I used an enum was to make sure the three or four separate C files used the value consistently.

Quote
Is 'hide_osd' a suitable name for the function when calling 'hide_osd(1)' does the exact opposite, and 'hide_osd(-1)' could either show or hide the display. It also does not make it clear that the function only affects the CHDK OSD, not the Canon one. Perhaps 'set_chkd_osd_state' might be better.
If you assume that "1" meams "true" the hide_osd(1) is valid.  The toggle value as -1 is strange but I don't have a better idea without adding another function.  And I assume the set_chkd_osd_state() suggestion is somewhat "tongue in check" ?

Quote
My other concern is that the 'magic' integer values as parameters to the functions make the script code harder to follow for new users, or if you've forgotten what the magic numbers mean.
I hate to say it, but its not nearly as bad as md_detect_motion() for needing to remember parameter values.  ;)

Quote
Not sure what the best solution for this is -  having three separate functions seems overkill.
Agreed on the overkill.  I couldn't come up with a better suggestion than what lapser invented .. so I figured to just go with that.
« Last Edit: 29 / September / 2013, 16:21:06 by waterwingz »
Logged
Ported :   A1200    SD940   G10    Powershot N    G16
*

Offline msl

  • *****
  • 1280
  • A720 IS, SX220 HS 1.01a
    • CHDK-DE links
Re: hide_osd() script function
« Reply #15 on: 29 / September / 2013, 16:26:18 »
Quote from: philmoz on 29 / September / 2013, 16:06:27
Perhaps 'set_chkd_osd_state' might be better.


For what need we -1 as argument? <0/1> or <false/true> should be suitable.

msl
Logged
CHDK-DE:  CHDK-DE links
*

Offline reyalp

  • ******
  • 14126
Re: hide_osd() script function
« Reply #16 on: 29 / September / 2013, 16:29:28 »
Quote from: philmoz on 29 / September / 2013, 16:06:27
My other concern is that the 'magic' integer values as parameters to the functions make the script code harder to follow for new users, or if you've forgotten what the magic numbers mean. Not sure what the best solution for this is - having three separate functions seems overkill.
I agree with this.

The Lua convention for functions that take enum-like options is to use a string, e.g. collectgarbage http://www.lua.org/manual/5.1/manual.html#5.1 - however, I don't really like that in this case since two of the values are basically boolean.

I'm not really convinced toggle needs to be a parameter, you could trivially write your own using the on/off values, though it would be clearer if there was a corresponding get_ function.

Quote from: waterwingz on 29 / September / 2013, 16:18:18
I hate to say it, but its not nearly as bad as md_detect_motion() for needing to remember parameter values.  ;)
Yes, and if we wrote md_detect_motion now, we would do it differently for Lua at least.

I know it seems silly to go back and forth over this, but making interfaces clear really does make life easier in the long run (I'm still regretting set_record...)
Logged
Don't forget what the H stands for.
*

Offline waterwingz

  • ******
  • 12140
    • Waterwingz
Re: hide_osd() script function
« Reply #17 on: 29 / September / 2013, 16:30:47 »
Quote from: msl on 29 / September / 2013, 16:26:18
Quote from: philmoz on 29 / September / 2013, 16:06:27
Perhaps 'set_chkd_osd_state' might be better.

For what need we -1 as argument? <0/1> or <false/true> should be suitable.
The -1 value is useful if you just want to tie the OSD state to a button on your camera and not remember the OSD state in your script.

Code: [Select]
--[[
@title OSD Toggle
--]]
repeat wait_click()
  if ( is_key("display")) then hide_osd(-1) end
until is_key("menu")
Logged
Ported :   A1200    SD940   G10    Powershot N    G16
*

Offline waterwingz

  • ******
  • 12140
    • Waterwingz
Re: hide_osd() script function
« Reply #18 on: 29 / September / 2013, 16:33:02 »
Quote from: reyalp on 29 / September / 2013, 16:29:28
I know it seems silly to go back and forth over this, but making interfaces clear really does make life easier in the long run.
No issue here with that statement and hopefully no ego on the best way to do it.   "We" just want to to be able to use the function somehow in our scripts.

Update : incidentally, as mentioned earlier, I believe that run-ubasic.c will no longer compile.  Does anyone care?
« Last Edit: 29 / September / 2013, 16:35:37 by waterwingz »
Logged
Ported :   A1200    SD940   G10    Powershot N    G16
*

Offline ahull

  • *****
  • 634
Re: hide_osd() script function
« Reply #19 on: 29 / September / 2013, 16:37:28 »
Quote from: waterwingz on 29 / September / 2013, 16:30:47
Quote from: msl on 29 / September / 2013, 16:26:18
Quote from: philmoz on 29 / September / 2013, 16:06:27
Perhaps 'set_chkd_osd_state' might be better.

For what need we -1 as argument? <0/1> or <false/true> should be suitable.
The -1 value is useful if you just want to tie the OSD state to a button on your camera and not remember the OSD state in your script.

Code: [Select]
--[[
@title OSD Toggle
--]]
repeat wait_click()
  if ( is_key("display")) then hide_osd(-1) end
until is_key("menu")

Just an observation if you have an equivalent get_chdk_osd_state, then to invert the state you could you not then use  something like

Code: [Select]
--[[
@title OSD Toggle
--]]
repeat wait_click()
  if ( is_key("display")) then set_osd_state(!get_chdk_osd() ) end
until is_key("menu")

(Feel free to tell me to but out, since I have just come in to the conversation half way through  ;) )
Logged
Pages: 1 [2] 3 4 5 6 7 ... 10   Go Up
« previous next »
 

Related Topics


SimplePortal © 2008-2014, SimplePortal