Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encoder support #422

Closed
poelstra opened this issue Sep 22, 2018 · 13 comments
Closed

Encoder support #422

poelstra opened this issue Sep 22, 2018 · 13 comments

Comments

@poelstra
Copy link

Hi! Thanks for your excellent graphics library!

I'm using it to implement a GUI for a room thermostat on an SSD1331 display (96x64 px) in combination with an encoder (turn + push). I noticed a few other issues (#365, #261) about this subject, and switched to dev-5.2 to try it out.

It took me a while to figure out that I had to assign the input driver to a group, but it works now 😄
I then played a bit with lv_test_group_1().

A few things that 'surprised' we were:

  1. I would expect registering the encoder driver would 'just work', just like e.g. the mouse driver does (including navigating in e.g. popup windows)
  2. I expected a short-click to also allow the slider to move
  3. Focus/edit mode of a slider on 100% (with Night theme) are invisible
  4. Rollers 'open' on focus, which suggests that further rotating would choose another element

(I hope the terse wording doesn't come across as offending, I certainly appreciate the effort that went into this already!)

Some random ideas (just to throw them out there)

  1. I'm used to e.g. creating Windows GUI's with Delphi (long time ago...), where keyboard focus was handled using e.g. a TabIndex property on each control. When set to a positive value, this would indicate the focus order.
  2. For many controls, a single click would probably suffice to 'enter' them, and a long-press could function as a generic way to 'exit' them. E.g. for controls like a slider a short or long click would exit them, for button groups a first short click enters the group, further short clicks operate the buttons, and a long-press exits the group).
  3. Maybe (also) restyle the knob on focus/edit? And/or provide a bit of 'glow' around the object?
  4. Suggest to keep the roller 'closed' on focus, and only open in edit mode.

I already experimented a bit in the code myself, and am more than happy to help out with both implementation and testing. Curious what your thoughts on this are, though!

@kisvegabor
Copy link
Member

Hi Martin,

Thank you for the suggestions and remarks! They very reasonable! I never used LittelvGL with an encoder controller GUI so I didn't have first-hand experience with its usability.

I try to release v5.2 soon and I'd like to add your great suggestions to it. So I implemented some of them. See below. Please test the updates and give some feedback!

  1. I would expect registering the encoder driver would 'just work', just like e.g. the mouse driver does (including navigating in e.g. popup windows)

When v5.2 will be released I'm planning to add a working example to the documentation which you can just copy-paste to your code.

  1. I expected a short-click to also allow the slider to move

Why not!? Added.

  1. Focus/edit mode of a slider on 100% (with Night theme) are invisible

Good perception! I fixed it. Now the border is drawn above the indicator.

  1. Rollers 'open' on focus, which suggests that further rotating would choose another element

If you mean Drop down list then I agree, it's a little bit confusing. I also updated it.

Random ideas

  1. I'm used to e.g. creating Windows GUI's with Delphi (long time ago...), where keyboard focus was handled using e.g. a TabIndex property on each control. When set to a positive value, this would indicate the focus order.

As probably you noticed in LittelvGL the order of creation of objects determines the "tab" order. I agree, in some cases, a "tabindex" property might be useful. However, it has some memory overheads... Do you think it's really useful for usually small embedded GUI's or just mentioned it?

  1. For many controls, a single click would probably suffice to 'enter' them, and a long-press could function as a generic way to 'exit' them. E.g. for controls like a slider a short or long click would exit them, for button groups a first short click enters the group, further short clicks operate the buttons, and a long-press exits the group).

Good idea. The navigate->edit can be short and/or long enter too. Only edit->naviagate needs to be long enter. I added it as well.

3 Maybe (also) restyle the knob on focus/edit? And/or provide a bit of 'glow' around the object?

Now only one style can be modified to have a focused appearance. It seems a littlebit complicated to add more (arbitrary styles) to update. Adding a glow seems difficult too because it would increase the size of the object and it's not supported now.

  1. Suggest to keep the roller 'closed' on focus, and only open in edit mode.

It also sounds reasonable. Added.

@poelstra
Copy link
Author

Wow, that's fast! 👍

  1. I would expect registering the encoder driver would 'just work', just like e.g. the mouse driver does (including navigating in e.g. popup windows)

When v5.2 will be released I'm planning to add a working example to the documentation which you can just copy-paste to your code.

That would indeed be helpful.

I meant something different, though (I now see I wasn't very clear ;)). I meant that it would be nice if just registering a keypad driver would be sufficient, i.e. that it wouldn't be necessary to assign it to a group, and to add individual objects to that group.

  1. I expected a short-click to also allow the slider to move

Why not!? Added.

Heh, again my mistake in unclarity... I meant to have short-click enter (and exit) edit mode of the slider, such that I can move it after a short-click (and get back to navigate mode after another short-click).
(As I mentioned in Random idea 2.)

  1. Focus/edit mode of a slider on 100% (with Night theme) are invisible

Good perception! I fixed it. Now the border is drawn above the indicator.

Cool, thanks! Much clearer now!

  1. Rollers 'open' on focus, which suggests that further rotating would choose another element

If you mean Drop down list then I agree, it's a little bit confusing. I also updated it.

Ah yes, I meant ddlist. Open on edit-mode is OK now, but I would propose to also leave edit mode again after selecting an item (with short-click), e.g.:

  • Rotate to navigate to ddlist, it becomes focused (orange), but stays closed
  • Short-click to start edit mode (opens, and becomes green)
  • Rotate to select other item in the ddlist
  • Short-click to select the item and go back to navigate mode (closes, and becomes orange)
  • Rotate to select next control on form, etc.

Random ideas

As probably you noticed in LittelvGL the order of creation of objects determines the "tab" order. I agree, in some cases, a "tabindex" property might be useful. However, it has some memory overheads... Do you think it's really useful for usually small embedded GUI's or just mentioned it?

The reason why I mentioned it was the thought whether the concept of groups is really necessary:

  • It seems the existing parent/child relationships of objects (together with signals such as LV_SIGNAL_GET_EDITABLE and possibly a future signal LV_SIGNAL_GET_FOCUSABLE) are enough to achieve the same effect.
  • It appears that it was only added to support keypad/encoder navigation.

Advantages:

  • No need to assign input driver to a group
  • No need to separately add objects to a group
    • GUIs made by people using just a mouse will automatically also work for people only having a keypad
    • Recursive structures (like a popup window) are nicely solved (no need to 'manually' add e.g. its buttons to the main group)
  • Controls can change their 'behaviour' (e.g. style) based on focus/edit state, without overhead, see below.

One would loose the possibility of having multiple encoders controlling different parts of the GUI. Not sure if this is a use-case, though. (I could imagine someone wanting to assign an encoder to a single dedicated control, but that wouldn't need navigate/edit mode anyway?)

I'll try to experiment a bit to see if this idea indeed might work.

  1. For many controls, a single click would probably suffice to 'enter' them, and a long-press could function as a generic way to 'exit' them. (...)

Good idea. The navigate->edit can be short and/or long enter too. Only edit->naviagate needs to be long enter. I added it as well.

I think that in most cases, edit->navigate is possible with short-click too (e.g. slider, ddlist, roller, ...).
Only elements that have 'children' which have their own concept of focus/edit (e.g. kb, list, ...) , would need a long-click as 'edit->navigate'.

  1. Maybe (also) restyle the knob on focus/edit? And/or provide a bit of 'glow' around the object?

Now only one style can be modified to have a focused appearance. It seems a littlebit complicated to add more (arbitrary styles) to update. Adding a glow seems difficult too because it would increase the size of the object and it's not supported now.

Fair enough :)

Hmmm, just a thought: if the focus/edit concept indeed becomes based on the object structure, there can only be one focused element in the app (instead of multiple, as possible with multiple groups). This would also enable any object to simply check "Am I the focused (or editing) object?" (i.e. without adding extra state in each object) and if so select the appropriate style from its theme.

This would also remove the need for style_mod_def and style_mod_edit_def.

E.g.:

struct {
        lv_style_t *bg;
        lv_style_t *bg_focus_and_edit;
        lv_style_t *indic;
        lv_style_t *knob;
        lv_style_t *knob_focus;
        lv_style_t *knob_edit;
    } slider;

(In this example, e.g. the background would have the same style for both focus and edit, and only the knob would change to indicate edit mode.)

  1. Suggest to keep the roller 'closed' on focus, and only open in edit mode.

It also sounds reasonable. Added.

Nice, thanks!


Encoder input driver

FWIW, here's the code I'm using for my encoder input:

bool input_read_encoder(lv_indev_data_t *data) {
  static int16_t diff = 0;
  diff += encoder.getValue();

  static uint32_t key = 0;
  static lv_indev_state_t state = LV_INDEV_STATE_REL;

  ClickEncoder::Button button = encoder.getButton();
  if (button != ClickEncoder::Open) {
    // If button is pressed, process that first, and ignore
    // turns of the encoder while pressed
    key = LV_GROUP_KEY_ENTER;
    state = LV_INDEV_STATE_PR;
    diff = 0;
  } else if (state == LV_INDEV_STATE_PR) {
    // Otherwise, first emit a release event before
    // processing remaining ticks of the knob
    state = LV_INDEV_STATE_REL;
  } else if (diff > 0) {
    // Emit one right 'click'
    diff--;
    key = LV_GROUP_KEY_RIGHT;
    state = LV_INDEV_STATE_PR;
  } else if (diff < 0) {
    // Emit one left 'click'
    diff++;
    key = LV_GROUP_KEY_LEFT;
    state = LV_INDEV_STATE_PR;
  }

  data->state = state;
  data->key = key;

  // Keep coming back until all encoder pulses are processed
  return diff != 0;
}

The library I use (ClickEncoder) supports 'acceleration' (nice for e.g. sliders), which means that in a single read of the input driver, the value may have gone up multiple ticks. I am emulating multiple key down/up events to pass these on to lvgl.

It might be handy to provide some sort of wrapper (or even a dedicated encoder input driver type?) such that people only have to pass number of ticks and current state of the button to make it work?

@kisvegabor
Copy link
Member

  1. Short-Long click to Enter-Exit
    I see and agree. As you mention it would be really useful to exit from edit mode on a short click for some objects. The question is which object types are affected. I think:
  • slider
  • ddlist
  • roller

I updated the objects above to work this way.

  1. Default group
    I think the most simple solution would be to add a define in lv_conf.h to create a default group and add focusable objects in their create function automatically. What do you think?

  2. Complex focus
    My only concern about adding extra "..._focus" and "..._edit" styles is their memory overhead.
    Earlier we talked about it here: Enhancement: More flexibility for focused object style #277
    What do you think?

  3. LV_INDEV_TYPE_ENCODER
    I'm convinced. An interface like "people only have to pass number of ticks and current state of the button to make it work" would be really different from the current LV_INPUT_TYPE_KEYPAD. Maybe an encoder type would replace lv_group_enable_edit. KEYPADs can use the groups normally (only orange) and ENCODER can use both navigate and edit modes.
    I added the ENCODER type and tested with this read function:

static bool enc_read(lv_indev_data_t * data)
{
    static int32_t last_diff = 0;
    data->enc_diff = diff - last_diff;;
    data->state = btn_state;

    last_diff = diff;

    return false;
}

Can you test it?

Thank you for sharing your code and mentioning ClickEncoder. I didn't know this library but it seems great!

@kisvegabor
Copy link
Member

I've just pushed a minor fix.
You encoder driver is great and made testing very easy!

@poelstra
Copy link
Author

Cool! I also made the same fix on my end already :)

I also already implemented the ENCODER type driver yesterday, in much the same way as you did, so it seems we're aligned ;)


I do have a few questions on the implementation of indev_encoder_proc, if you don't mind:

            focused->signal_func(focused, LV_SIGNAL_FOCUS, NULL);      /*Focus again. Some object do something on navigate->edit change*/

Would it make sense to have dedicated signals like LV_SIGNAL_ENTER and LV_SIGNAL_LEAVE for entering and leaving edit mode? (It may be a bit confusing otherwise?)

In the "Release happened" part, I noticed that it would still send LV_GROUP_KEY_ENTER to the control, even after a long press. Therefore, I added a big if inside of it, like so:

    /*Release happened*/
    else if(data->state == LV_INDEV_STATE_REL && i->proc.last_state == LV_INDEV_STATE_PR)
    {
        if (i->proc.long_pr_sent == 0) { /* Short-click release */
            lv_obj_t * focused = lv_group_get_focused(i->group);
            bool editable = false;
            focused->signal_func(focused, LV_SIGNAL_GET_EDITABLE, &editable);

            /*The button was released on a non-editable object. Just send enter*/
            if (!editable) {
                lv_group_send_data(i->group, LV_GROUP_KEY_ENTER);
            }
            /*An object is being edited and the button is released. Just send enter */
            else if (i->group->editing) {
                lv_group_send_data(i->group, LV_GROUP_KEY_ENTER);
            }
            /*If the focused object is editable and now in navigate mode then enter edit mode*/
            else if(editable && !i->group->editing && !i->proc.long_pr_sent) {
                i->group->editing = i->group->editing ? 0 : 1;
                focused->signal_func(focused, LV_SIGNAL_FOCUS, NULL);      /*Focus again. Some object do something on navigate->edit change*/
                LV_LOG_INFO("Edit mode changed (edit)");
                lv_obj_invalidate(focused);
            }
        }

        if(i->proc.reset_query) return;     /*The object might be deleted in `focus_cb` or due to any other user event*/

        i->proc.pr_timestamp = 0;
        i->proc.long_pr_sent = 0;
    }

However, it seems that then (e.g.) button no longer 'clears' its long_pr_action_executed.
Is this intended behaviour? I would expect either LONG_PRESS or ENTER, not both.


Default group

I think the most simple solution would be to add a define in lv_conf.h to create a default group and add focusable objects in their create function automatically. What do you think?

That would work too, I suppose.

I was experimenting a bit with achieving the same without groups, e.g. having an indev_focus_next() iterating to the next focusable sibling etc.

My idea was that a short-click would then:

  • enter edit mode (if editable), or
  • 'decend' into its children (if any, e.g. for kb, list), or
  • send ENTER

And that long-press would navigate back to the parent, i.e. behave somewhat like an Escape key or Android's back button.

That would probably also work very nice for e.g. a (modal dialog) popup, where long-press would 'cancel' the dialog.

Not sure how to handle things like long-press on a button in that case, though. I suppose it could still work, but only when there is no 'parent' to navigate to, such as when the button is on the 'home screen' (i.e. not inside a dialog or something).

I'll experiment a bit with this if you don't mind, and if it appears feasible, I'll put up a PR for further discussion (not necessarily for direct merging).

  1. Complex focus
    My only concern about adding extra "..._focus" and "..._edit" styles is their memory overhead.
    Earlier we talked about it here: Enhancement: More flexibility for focused object style #277
    What do you think?

I was under the impression that the necessary styles would only be stored on the theme, so all instances of an object type (e.g. sliders) would reference the same style data. In that case, the overhead is very minimal? (e.g. just 2 extra lv_style_t on the theme for a slider, which would be shared by all sliders)?

  1. LV_INDEV_TYPE_ENCODER

Perfect! It was exactly what I was thinking of :)

I want to do a bit more testing, but results so far are very nice, thanks!

@poelstra
Copy link
Author

I did some more testing (using lv_test_group_1()) and I think it's a big improvement! 👍

Some smaller things I found:

  • ddlist selected item has a nice animation when opening, but then 'jumps' to reposition the selected item in the middle. Probably the animation should try to keep the selected item in the middle all the time, instead of moving it to the top.
  • btnm, keyboard, mbox and list all 'highlight' their first button on focus (orange), but clicking the encoder will not press that button, but just enter edit mode. I think it's more clear if that button is only highlighted when edit mode is active.
  • page doesn't scroll on edit (I think that's a known issue)
  • if the page only contains non-focusable/editable objects, it may make sense for a single click to exit edit mode again.

I wonder how navigation would work with 'nested' controls, e.g. putting sliders on the tab controls or a page. Didn't try that yet.

@kisvegabor
Copy link
Member

  1. LV_SIGNAL_ENTER and LV_SIGNAL_LEAVE
    Sounds reasonable. However, only a few object types are affected only with encoders. So maybe it's not worth to create new signals for this few cases. I suggest to decide it later. If something comes to light which we can't solve (or just very difficultly) with current API we can add the new signals in the next release.

  2. Long press on release
    It is the intended behaviour because the object itself should know to do nothing (or something) when a release comes after long press. For example, the button won't call the click action if long press action is already executed. If an object doesn't care with the long press then it should accept the click even if the press lasted for seconds. I think the GUIs on PC and mobile works this way as well.

  3. Default group - Hierarchical navigation
    Interesting idea! For example, if you have a page which needs to be scrolled and you have some objects on it your suggested solution would make the navigation very simple.
    I can imagine an interface like (just thinking loudly):

  • lv_group can have an attribute to change lv_group_focus_next/prev to iterate among all the objects or only the siblings circularly.
  • Functions like lv_gorup_focus_parent and lv_group_focus_child can be used to step on level.

Only one thing is not clear: If you have a page with some objects and you are now focusing on the page you can do 3 things:

  1. Focus on the next sibling (maybe an other page). It's clear: turn the encoder or press TAB

  2. Focus on the first child. Clear, press enter or push the encoder.

  3. Scroll the page. The keyboard arrows can scroll it but what to do with encoder? Maybe the user needs to assign a focus callback to every child of the page an call lv_page_focus to scroll the page to show the object is being focused.

  4. Complex focus styles
    Storing the extra styles in the theme makes sense. However, it needs a lot of updates and test so I think it should part of the next release. We already had a long discussion about reworking the styles system and themes. The conclusion was to remove default styles and create a very light weighted default theme instead. In this default theme, we will have the opportunity to add the focused styles too. Here is the discussion: [v6.0] Rework style system to use padding and margin better and remove init. values #302
    What do you think?

  5. Test results
    I will check them soon!

@poelstra
Copy link
Author

  1. LV_SIGNAL_ENTER and LV_SIGNAL_LEAVE
    (...) we can add the new signals in the next release.

Sure! Still experimenting anyway, and I'd like to create some actual application objects first to see what I run into in practice :)

  1. Long press on release

Ah, so it's more of a 'release' event than an actual 'click' event then. That makes sense. Maybe naming can be updated to better reflect this in a future release?

Complex focus styles, #302

Ah, yes, that seems like a good idea! It certainly shouldn't delay the 5.2 release.

Hierarchical navigation

Yes, exactly!

So 'entering' an object (short-clicking the encoder) will either call lv_group_focus_child() (if it has a focusable child like a slider), or enter edit mode (if it's editable), or just send a KEY_ENTER to the focused object.

Long-click will then always try to lv_group_focus_parent(), and if that returns false (e.g. no parent to go to, or somehow prohibited by the current object), then it will just send LONG_PRESS to the focused object.

I'm not sure on how to best implement the 'scroll into view' when focusing on an object on a page, I did not dive into the code of e.g. page yet. I would not be a big fan of people having to assign manual callbacks though. Maybe the current parent (i.e. of which we're navigating the children) should also be sent a signal whenever its children change focus. So e.g. the page could then do a lv_page_scroll_into_view(obj) as necessary?

I suppose the same holds for things like btnm.

@kisvegabor
Copy link
Member

kisvegabor commented Sep 26, 2018

Tests:

  • ddlist animation: I checked it and it really jumps when the animation is ready. Probably it's possible to animate the position of the list as well when opens/closes. However, it's a little bit difficult to properly calculate the values of the animation when the page size is changing.

  • button highlight in navigate mode: It's truly confusing, I fixed them

  • page scroll: it was because the encoder sent LEFT/RIGHT keys and the page needs UP/DOWN to scroll vertically. I fixed it by scrolling vertically on LEFT/RIGHT if the scrollable part is not wide enough to scroll horizontally.

  • leave page on short click: true, maybe the clearest way is to add a function to lv_page to determine what how to exit from edit mode. This way the derived objects (like lv_list) could easily set the page's behaviour.

Hierarchical navigation:
I agree some automated method would be better.

For clarity, I suggest using this topic for fixing the issue of the currently implemented things and create new issues for improvements planned for next releases.
I added a reminder in #302 about storing focused styles in themes.

@poelstra
Copy link
Author

  • ddlist animation: ok

  • button highlight: thanks!

  • page scroll: ah, cool!

  • leave page on short click: yeah, something like that. It may be a good idea to create a more elaborate test case first. E.g. having multiple pages with and without editable controls, a dialog with some controls that pops up when clicking a button and then needs to be closed again, etc. Because I can imagine that some things will indeed be very control-specific (thus needing e.g. a signal), and others can better be handled 'globally' (e.g. by the lv_group_focus_ functions).

Did you push these changes already?

BTW, I just found out that the current encoder driver crashes when there are no controls to be focused.
I think we need to sprinkle a few more if (focused != NULL) ... etc :)

I agree with creating separate issues for the remaining 'new' stuff (hierarchical navigation and ddlist animation). I can do that later.

@kisvegabor
Copy link
Member

Upps, I forget to push :) Pushed now.

Leave page edit on short click:
As I see there are two cases:

  1. An object is derived from lv_page. E.g. list, text area, roller, ddlist etc. In this case, I think it's fine to control the behaviour in the lv_..._create() function explicitly.
  2. The page is a general container and you can create random objects on it (buttons, labels, lists etc). E.g. the content area of the window or tab view. In this case, the child objects can be added to the group one-by-one (or automatically to the default group) and the page can leave edit mode on short click.

So if the default mode is "short click exit" then it will work in general cases. If the object type needs something else it can change the behaviour. What do you think?

The crash:
Do you mean when no objects are added to the group? I tested it and didn't crash for me. Anyway, added some if(focus) {...}

@poelstra
Copy link
Author

Hierarchical nav: adding the children themselves to the group seems awkward (even if it happens automatically), because e.g. the childen of the main page would be in the same list as the children of the popup dialog. So things like focus_next() have to somehow skip a whole bunch of controls if they want to stay in 'sibling mode'. Whereas the list of available siblings is already available as the children of the control (e.g. popup window).

I'm actually writing some application code right now (using the already excellent lvgl :)), but if I have some more time in the coming weeks I'll see if I can come up with a good 'demo app' to have something to shoot on.

Crash: yes, group was 'empty', and encoder called focused->signal_func(focused, LV_SIGNAL_GET_EDITABLE, &editable); in indev_encoder_proc(), which crashed.
You added it to the same spot as I did locally, so that should fix it. Thanks!

@kisvegabor
Copy link
Member

Okay, I'm waiting for the demo! Thank you! :) It would be easier to speak about a concrete thing.

As the main issue (better Encoder support) seems to be implemented successfully I think this issue can be closed.
If you have the demo app please open a new issue for the "Hierarchical navigation".

Thank you very much for the contribution! It was really nice to work with you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants