release
dev builds
extras
themes manual
wiki
device status forums
mailing lists
IRC bugs
patches
dev guide



Search | Go
Wiki > Main > RockboxArchitectureImprovements > ButtonActionIdea (r49)

Explanation of the Button Actions Idea

Requirements

The idea of the new Action based button system is to fix these problems in the current code.
  1. Remove 99+% of the #ifdef's in the existing button loops DONE
  2. Make the button loops simpler by not needing last_button and _PRE DONE
  3. Move all button configuration to a single location to make porting new targets easier DONE
  4. Bring consistent button operation to the entire core
  5. Allow button configuration changes on-the-fly (e.g iriver remote) DONE

The current incarnation!

  • I decided that seen as there is plenty of example usage in the code (and the text here was soo outdated) it was safe to remove this section..
  • read status and comments for updates.

IRC conversation re this.

linkage starts at 09.40.04

Status

  • Converted and Working
    • The synclist widget
    • The menus
    • The file browser (uses synclist)
    • The WPS (99% finished, maybe needs some more cleaning up)
    • The syncselect widget (includes all the settings in the menus)
    • The quickscreen (iriver keymappings only)
    • First version of the action lib for plugins is done, button_action_test.c is the example usage file.
  • Not converted
    • fm / recorder / quick screen / yesno / colour picker / eq screen /
    • plugins (major task in itself... do plugin struct first, then add plugin contexts, then do this slowly after we are committed!)
    • ?
  • Targets "ported"
    • iriver h1x0 h3x0
    • ipod (needs to be tested)
    • archos recorder (thanks AntoineCellerier:)
    • ondio (needs to be tested)
    • player (needs to be tested)
    • x5 (needs to be tested)
    • only the iriver targets have mappings for the remotes.

$ grep -nr "button_get" apps/ ( i removed the plugins/ folder from the output)
apps/main.c:337:        while(!(button_get(true) & BUTTON_REL));  - I don't think we should change this
apps/main.c:346:        if (button_get_w_tmo(HZ/10) == SYS_USB_CONNECTED)  - I don't think we should change this
apps/main.c:373:            while(button_get(true) != SYS_USB_CONNECTED) {};  - I don't think we should change this
apps/pcm_recording.c:147:        button = button_get_w_tmo(HZ/8);
apps/screens.c:96:    while (button_get(true) & BUTTON_REL); 
apps/recorder/keyboard.c:682:        button = button_get_w_tmo(HZ/2);
apps/recorder/peakmeter.c:1186:        button = button_get(false); - no need to change this
apps/recorder/peakmeter.c:1257:        btn = button_get(true);  - or this
apps/recorder/radio.c:520:            button = button_get(false);
apps/recorder/radio.c:522:            button = button_get_w_tmo(HZ / PEAK_METER_FPS);
apps/recorder/radio.c:1024:            button = button_get(true);
apps/recorder/recording.c:1111:            button = button_get(true);
apps/recorder/recording.c:1221:        button = button_get(true);
apps/recorder/recording.c:1331:        button = button_get(true);

Incorrect/changed button combos

What else needs doing?

  • Plugin support (can wait until after commited.)
  • F* keys on targets that have them.
  • WPS_KEYLOCK ? - I think I've done this correctly, need to test (-- JonathanGordon - 08 Aug 2006)
  • A-B stuff in gwps.c - Done (-- JonathanGordon - 09 Aug 2006)

Comments

Phew! Narrowly averted a problem which could have killed this whole idea! The last_button checking from the old system actually did more than just the pre handling. It meant you didn't have a cascade menu exit on the ipod (where play hold is exit). The way I got around that is that on certain screens a call to void action_signalscreenchange(void) is needed which will prevent the action handler returning anything until the BUTTON_REL flag was set on a button. I haven't had enough time to test this, but it works in the ipod sim so I think it's ok, but if someone has a better idea please speak up!
void action_signalscreenchange(void)
{
    if (last_button&BUTTON_REPEAT) {
        ignore_until_release = true;
   }
}
BUTTON_REPEAT is checked because without this it will "eat" keypresses which can be valid, no time to check this, and that function doesn't have the check in the attached .diff, so if you do try it, please add that line. -- JonathanGordon - 28 Jul 2006

Comments from code review (LinusNielsenFeltzing - 28 Jul 2006)

  • I think TIMEOUT_BLOCK should be -1, otherwise we can't wait for 1 tick
  • Bug spotted by HristoKovachev: apps/tree.c:1003: = if (button_get(false) = ACTION_STD_EXIT)
  • In the current incarnation, there is no way to create your own, local context. I think plugins might want that.
  • If there is no get_action() anymore, let's rename get_default_action() to get_action() instead

Reply to above comments (-- JonathanGordon - 29 Jul 2006)

  • Thanks HristoKovachev: for fixing the TIMEOUT_BLOCK define big grin
  • 2 new funcitons in action.c action_userabort instead of manually checking the get_action return value for ACTION_STD_EXIT, and get_custom_action() to supply a custom button mapping array.
  • get_default_action renamed to get_action
  • HristoKovachev: 's patches have also been incorporated, (thanks again for them)
  • action_signalscreenchange() has been added to everywhere that get_action is called (hopefully its all done correctly, but it is possible i may have missed an exit branch..)
    • I decided not to add it after the default case when we are exiting because of USB connect because I'm lazy and I dont think it's needed. If this needs to be fixed, it should be easy enough though.
  • converted more of the code, the last few pages are gonna be a bit painful to convert, and 1am saturday night after a night on the booze isn't the best time for it smile

Linus read please!

Hey, OK, well I have added a nice level of complexity that I think i need to explain.

The action checking was too limited by always immediately falling back to CONTEXT_STD if the button was not found. So now I have implemented "chaining" where the last item of the "points" to the context to search next. The idea is that if you only want to change one key on a screen from one of the existing contexts you only need to redefine that one and then point to the other context.

The iriver keymap file uses this for the CONTEXT_BOOKMARKSCREEN (except the mapping doesn't work correctly).

The reason for the function pointer in the do_action_worker function is so this will work fine with plugins, they will need to do their own context -> button_mapping function which shouldn't be too hard, and they will be able to fall back onto the standard context's very easily. -- JonathanGordon - 31 Jul 2006

Comments and update (LinusNielsenFeltzing - 31 Jul 2006)

  • I still can't figure out why the action_signalscreenchange() function checks the BUTTON_REPEAT bit. I changed this to check for the absence of BUTTON_REL, and it worked much better.

void action_signalscreenchange(void)
{
    if (!(last_button&BUTTON_REL)) {
        ignore_until_release = true;
   }
}

  • I also added an ACTION_STD_WPS action, which takes you to the WPS. The earlier patch uses ACTION_STD_ACCEPT for that, but that's a totally unrelated action.

reply to above (-- JonathanGordon - 01 Aug 2006)

  • That's how I had the signal change function also, except it would eat presses that are needed, putting it back to button_repeat seemed to fix it here, I'll try again though.
  • action_tree_backtowps should be used instead of a std define, because on can is back to wps from the tree and accept in menus.
  • Just looked at your diff and it looks like you used the old version, without the changes i mentinoed above?

Reply (-- LinusNielsenFeltzing - 01 Aug 2006)

  • Ehum, it's not back to WPS. The browser is the main screen.
  • ACCEPT to leave the menus??? Sounds odd.
  • I thought I used your latest patch from the wiki page. My mistake.

counter reply stick out tongue (-- JonathanGordon - 01 Aug 2006)

  • Hehe ok, actually, putting it in STD means that the there is no need to setup a mapping list for the tree unless you want pg up/down and stop in the tree.
  • ?? accept is save the change. Oh, in irc last night (my time) linuxstb and others thought it silly that left could be used as return and accept the setting. My suggestion is allow both but make it usersettable, easiest way to please everyone.
  • Oh well... better grab it again, I'm about to upload a new version which makes the signalscreenchange function safer. Also fixed a minor thing with the timeouts.

Comment (-- LinusNielsenFeltzing - 01 Aug 2006)

  • Why is it that your keymaps deviate so much from what we have today? For example, why are so many buttons mapped to ACTION_STD_OK? I'm especially concerned about the mapping of BUTTON_ON to ACTION_STD_OK, which forces you to map it to ACTION_NONE in the listtree map. In which screen do we need to map BUTTON_ON to ACTION_STD_OK?
    • Because both _std_accept and _std_select were merged, if a target has plenty of buttons spare why not allow it to have more than one button for a given action? -- JonathanGordon - 02 Aug 2006
    • In this particular case, I think we want to reserve the Play button for special stuff, like taking you to the WPS. -- LinusNielsenFeltzing - 02 Aug 2006
  • Returning from WPS to filetree got stuck in the screenchange loop on the X5, removing the call to button_clear_queue() solved it.

more comments

  • The new version has the button_clear_queue() call removed and ACTION_TREE_WPS.

Latest Version (-- JonathanGordon - 08 Aug 2006)

  • Implemented software keylocking in the action loop to allow remote buttons to work while the main unit is locked (haven't tested, but someone look at it please.) - I've been playing a bit more and decided to not bother with the #ifdef HAVE_SOFT_KEYLOCK because the needed code is tiny compared to the rest of it, and i hate the #ifdefs cluttering the code stick out tongue
  • Minor changes to the iriver keymap file
  • A-B repeat is now working
  • quickscreen converted and working (needs kaymaps for all targets except iriver)
  • PLUGIN LIB!! please look at apps/plugins/lib/pluginactions.[ch] and comment on it, im not sure if im happy with it or not. it does work tho! (so much for that... i forgot to cvsdo add the pluginactions files and accidently deleted my working directory so i lost them... good thing because i didnt like the implementation anyway)
  • synced to cvs

latest again stick out tongue -- JonathanGordon - 14 Aug 2006

  • synced to cvs.
  • added the hold l/r for scroll in the list view. you can disable it by adding the setting "hold_lr_for_scroll_in_list" to a .cfg file and setting it to off (not settable in the menus)

I Attachment Action Size Date Who Comment
action-linus-060731.patchpatch action-linus-060731.patch manage 173.1 K 31 Jul 2006 - 23:28 LinusNielsenFeltzing Linus tiny updates to the actions.diff from 31 jul 14:39
actions.diffdiff actions.diff manage 193.4 K 15 Aug 2006 - 09:25 JonathanGordon commit me!!
Edit | Attach | Print version | History: r50 < r49 < r48 < r47 | Backlinks | View wiki text | More topic actions...
r49 - 15 Aug 2006 - 09:25:13 - JonathanGordon
Copyright by the contributing authors.