Making kbd.c code generic

  • 51 Replies
  • 5757 Views
*

Online reyalp

  • ******
  • 11084
Making kbd.c code generic
« on: 16 / February / 2015, 01:57:43 »
Advertisements
As mentioned in the usb remote discussion (http://chdk.setepontos.com/index.php?topic=7127.msg120543#msg120543) I've wanted reduce the duplication in the kbd.c files and put more the logic in a common file. I started looking into this over the weekend.

It's a big invasive change, but I think it's both possible and worthwhile.

Motivation:
  • Make it easier to add or change features related to the keyboard and GPIO bits. The forced_usb stuff is a good example: the functionality is simple, the required values are known for almost every camera, but actually adding it to every camera is a lot of work.
  • Reduce copy / paste errors
  • Make porting simpler, because there's code to look though and figure out what is actually camera specific
  • Move more code to thumb, saving space

There is a lot of variation in the kbd.c files. Some of it is due to actual difference in the underlying firmware and hardware, and some of it is just drift, as the code as been copied and we've figured out better ways of doing things.

General approach
  • A new platform dependent file in core which contains most of the logic
  • A new per platform header that defines the various masks, indexes etc
  • Platform kbd.c reduced to mostly calling the core file and defining this that are truly platform dependent, but able to implement special cases where needed

From trawling through the various kbd.c files, I came up with some general areas that need attention:
  • touch screen cameras
  • cameras with inverted key logic
  • oddballs old cameras like ixus30
  • variations in jogdial implementation
I have more detailed notes on these I'll post about later.

I also started an initial implementation, attached below. d10, a540, and sx160 converted as examples. This is not anywhere close to final, it's a way to figure out whats required and experiment with different approaches.
Don't forget what the H stands for.

Re: Making kbd.c code generic
« Reply #1 on: 16 / February / 2015, 08:45:08 »
It's a big invasive change, but I think it's both possible and worthwhile.
Agreed on all three points.

We will want to think about a standard test that can be applied to any port and maybe a shared spreadsheet or something to log results?
Ported :   A1200    SD940   G10    Powershot N    G16

*

Online reyalp

  • ******
  • 11084
Re: Making kbd.c code generic
« Reply #2 on: 16 / February / 2015, 17:08:05 »
We will want to think about a standard test that can be applied to any port and maybe a shared spreadsheet or something to log results?
That's a good idea. We can't really test this fully with script but, the general check should be
1) boot, mode switching, shooting etc work normally. Messing up physw bits can have odd effects: http://chdk.setepontos.com/index.php?topic=12218.0
2) Firmware keys work as expected when not in alt mode
3) CHDK menu navigation, jogdial work as expected
4) Scripted key presses, jogdial work as expected
5) remote, usb forcing etc.

My plan was when we 1.4 to stabilization phase, we would link prominently on the news and download pages and specifically request people report which cams they had used.

That said

Another thing I've been planning is to make a #define you can use in kbd_platform.h that means the cameras kbd.c implements everything (with the possible exception of some of the really generic clicked/pressed etc) functions. This would both will allow converting cams over time rather than one moster change, and also handle the ones (like ixus30) that really are special cases.

We don't want to end up with the same situation as the old generic/kbd.c include where only a few cams used, but I think a large fraction of the cams can be converted blindly without too much risk.

From IRC:
> < waterwingz> reyalp : in your post "Making kbd.c code generic" the first point in "General Approach" should say "1. A new platform INDEPENDANT file in core which contains most of the logic"  I think?

I actually did mean platform dependent, in the sense that each cam needs it's own .o file like other core files that use platform specific defines.
Don't forget what the H stands for.

Re: Making kbd.c code generic
« Reply #3 on: 16 / February / 2015, 19:26:31 »
From IRC: < waterwingz> reyalp : in your post "Making kbd.c code generic" the first point in "General Approach" should say "1. A new platform INDEPENDANT file in core which contains most of the logic"  I think?
I actually did mean platform dependent, in the sense that each cam needs it's own .o file like other core files that use platform specific defines.
So that file that would allow stuff like

Code: [Select]
#if defined(CAMERA_a530) || defined(CAMERA_a540)
// blah
#endif
which AFAIK used to be all over the CHDK code but is only used in platform/generic/kbd.c in 1.3.0 now ?
Ported :   A1200    SD940   G10    Powershot N    G16


*

Online reyalp

  • ******
  • 11084
Re: Making kbd.c code generic
« Reply #4 on: 16 / February / 2015, 23:26:48 »
Code: [Select]
#if defined(CAMERA_a530) || defined(CAMERA_a540)
// blah
#endif
which AFAIK used to be all over the CHDK code but is only used in platform/generic/kbd.c in 1.3.0 now ?
A fair bit of the core code is still has platform specific ifdefs (for 1.4, everything in platform/makefile_sub.inc OBJS)

For the keyboard code,  I would hope to avoid ones for single cameras and instead rely on stuff like CAM_HAS_JOGDIAL, CAM_TOUCHSCREEN_UI etc.

We might for example need separate key press functions for touchscreen vs non touchscreen, but we shouldn't need a separate set for every camera.

Speaking of touch screen, phil said in IRC:
>  the duplicated hackkey/canonkey entries don't matter for kbd_is_key_pressed, it simply matches the first one (the assumption being they are always paired so a given hackkey always has the same canonkey).

This assumption holds for ixus310_elph500 but does not seem to hold for N and N_facebook, e.g.:

Code: [Select]
    { 3, KEY_DISPLAY        , 0x00000010,   0,  144, 39, 191,  0, " ",     0,     GUI_MODE_ALT, GUI_MODE_ALT, MODE_REC|MODE_PLAY },
...
    { 3, KEY_DISPLAY        , 0x00000080,   0,  144, 39, 191,  0, "BACK",  0,     GUI_MODE_MENU, GUI_MODE_MENU,  MODE_REC|MODE_PLAY },
...
    { 3, KEY_DISPLAY        , 0x00000100,   0,  144, 39, 191,  0, "DISP",  0,     GUI_MODE_SCRIPT, GUI_MODE_OSD,  MODE_REC|MODE_PLAY },

This means that functions that do a linear search for a particular key will only ever find the first one.

> The check for canonkey == 0 does seem redundant.

Attached is a patch for powershot N that removes the key check and makes the hackkey/canonkey mapping 1:1.

One thing I'm not clear on is the use of TS_ZOMBIE. The first one looks like it actually does something, while the later ones don't. For the moment I've left them with unique canonkey values.  I'd say if a key actually does something, it should probably have unique name.
Don't forget what the H stands for.

Re: Making kbd.c code generic
« Reply #5 on: 16 / February / 2015, 23:40:05 »
This means that functions that do a linear search for a particular key will only ever find the first one.
Agreed.  I likely numbered the canonkey field bit sequentially while I was trying to figure out how the rest of phil's code worked. Don't believe it causes any harm beyond creating some confusion about why I did it?


Quote
One thing I'm not clear on is the use of TS_ZOMBIE. The first one looks like it actually does something, while the later ones don't. For the moment I've left them with unique canonkey values.  I'd say if a key actually does something, it should probably have unique name.
TS_KEY_ZOMBIE is just an "out of range" value that can  never be used by script click() or press()functions.  So those entries in KeyMap keymap[] can never be activated from a script.   
Ported :   A1200    SD940   G10    Powershot N    G16

*

Online reyalp

  • ******
  • 11084
Re: Making kbd.c code generic
« Reply #6 on: 16 / February / 2015, 23:50:05 »
Don't believe it causes any harm beyond creating some confusion about why I did it?
If I understand correctly,  it depends how they are used in the code. If the code used kbd_is_key_pressed(KEY_DISPLAY) then it will only ever see the state of the first canonkey, which would break in contexts that show one of the later ones.

If it uses something like kbd_get_clicked_key() then it would be fine.

If the TS_ZOMBIE should never be clicked or queried, then the canon key value shouldn't matter.
Don't forget what the H stands for.

Re: Making kbd.c code generic
« Reply #7 on: 17 / February / 2015, 00:00:49 »
If the TS_ZOMBIE should never be clicked or queried, then the canon key value shouldn't matter.
Never clicked or queried from a script.  But now that I look at it,  I think I was wrong about my comment about the the stuff in canon_key for the touch buttons not mattering ( kbd_new_state[3] values ).  The code in chdk_process_touch() uses it
Code: [Select]
touch_panel_button &= ~keymap[i].canonkey;to manage touch_panel_state.

Ported :   A1200    SD940   G10    Powershot N    G16


*

Online reyalp

  • ******
  • 11084
Re: Making kbd.c code generic
« Reply #8 on: 22 / February / 2015, 20:22:10 »
Update:
I added some #defines to support the various permutations, and converted some cameras as examples. The defines are documented in the top of kbd_common.c

This ends up being a lot of ifdefs, but most of them are used in a straightforward way (if cam needs FOO, do FOO without a lot of nested conditionals), so I'm still fairly happy with it.

The following cameras are converted, untested.

Powershot N
Example of a touch screen cam. More of the touch screen specific stuff could be made generic, but there's currently only 4 touch screen cams so it doesn't have to happen right away.
This is similar to the patch I posted earlier for N, with a few additional changes
* The current N code defines a mask and index for SD read only bit, even though the camera presumably doesn't have have one. I took this out.
* One of the KEY_DISPLAY entries in the keymap had a blank name. I assume this means it wouldn't be meant to be touched, so I converted it to TS_ZOMBIE

A495
Example of a cam with some inverted key logic. I think support for this could be rolled into the common kbd_update_key_state, but again there's only a few cameras that use this so it may not be worth it.

ixus30_sd200
example of a camera that completely does it's own thing, ignoring the common code. Having a platform_kbd.h define for this is somewhat roundabout, but makes it easy to grep for such cameras. This  could probably be converted to use the common key_press* functions

Additionally, ixus140_elph130 is converted and tested.



Some other notes and issues

There are two common styles of using _GetKbdState (aka  kbd_fetch_data) and _kbd_read_keys_r2.

On cameras like d10 (edit for posterity: I subsequently converted d10 to the second style, a540 is an example of the first):
kbd_fetch_data into new_state
run kbd_process (all chdk kbd task code)
copy into physw_status, fiddling key bits as required
_kbd_read_keys_r2, into physw_status
fiddle USB and SD read-only bits.

On cameras like sx160:
kbd_fetch_data into new_state
_kbd_read_keys_r2, into new_state
run kbd_process (all chdk kbd task code)
copy into physw_status, fiddling key bits as required
fiddle USB and SD read-only bits.

I think this difference in ports mostly depends on which port people started form, not a difference in camera behavior.

The new code can support either (see the d10 and sx160 conversions) but might be simplified if we standardized on one or the other.


CAM_USE_ZOOM_FOR_MF
This is defined by default. Rather than undef'ing it in platform_camera.h, some cameras just make kbd_use_zoom_as_mf return 0 (a800 is an example)
Cameras with an MF (old S series) key treat this differently, using the MF key when it's active. This isn't supported yet in the common code.

G9 (and possibly others) have a jogdial, but only have support for scripted wheel_left/wheel_right, not using the jogdial in CHDK or masking it from the canon firmware when in alt mode.

GPS cameras have a strange thing:
 if (gps_key_trap > 0)
that makes kbd_task sleep for 1 second. I'm not sure what it does, but it seems odd.

Many old cameras have a KEY_DUMMY in the key map. AFAIK this is for some power saving prevention code that no longer exists, and can be removed.

a470 has a weird hack involving the power button

a490 and a800 have some code to swap the print and display keys when a script is running(?), but doesn't actually define either key in the key map

some cameras (e.g. s5is, inverted key logic cams like a495) read directly into physw_status rather than new_state
« Last Edit: 14 / March / 2015, 01:09:12 by reyalp »
Don't forget what the H stands for.

*

Offline srsa_4c

  • ******
  • 3538
Re: Making kbd.c code generic
« Reply #9 on: 23 / February / 2015, 19:28:26 »
A495
Example of a cam with some inverted key logic. I think support for this could be rolled into the common kbd_update_key_state, but again there's only a few cameras that use this so it may not be worth it.
Inverted keys are present on several other cams (search for "uses inverted logic" in the stubs_entry.S files), but they are usually not used by CHDK currently.

Quote
ixus30_sd200
example of a camera that completely does it's own thing, ignoring the common code. Having a platform_kbd.h define for this is somewhat roundabout, but makes it easy to grep for such cameras. This  could probably be converted to use the common key_press* functions
This wasn't my invention, it's just how the ixus40 port was done. The firmware code uses system timers, and is quite different from anything that came later, so it's impossible to make it look/work like the usual keyboard code.

Quote
GPS cameras have a strange thing:
 if (gps_key_trap > 0)
that makes kbd_task sleep for 1 second. I'm not sure shat it does, but it seems odd.
Related? Didn't check yet.

Quote
a470 has a weird hack involving the power button
+ same hack in ixus110, I'm the one who's responsible... Needed the extra button for CHDK.
Perhaps it can be replaced by something generic? Note that this is using an inverted button on the a470.

 

Related Topics