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

feat(indev) Add crown support to pointer input device #5057

Merged
merged 14 commits into from
Mar 19, 2024
Merged

Conversation

kisvegabor
Copy link
Member

Description of the feature or fix

It nicely integrates the the typical watch crown features with the pointer input device. To test it with SDL change LV_SDL_MOUSEWHEEL_MODE to 1.

cc @XuNeo @bjsylvia

Checkpoints

Be sure the following conventions are followed:

  • Follow the Styling guide
  • Prefer enums instead of macros. If inevitable to use defines export them with LV_EXPORT_CONST_INT(defined_value) right after the define.
  • In function arguments prefer type name[] declaration for array parameters instead of type * name
  • Use typed pointers instead of void * pointers
  • Do not malloc into a static or global variables. Instead declare the variable in lv_global_t structure in lv_global.h and mark the variable with (LV_GLOBAL_DEFAULT()->variable) when it's used. See a detailed description here.
  • Widget constructor must follow the lv_<widget_name>_create(lv_obj_t * parent) pattern.
  • Widget members function must start with lv_<module_name> and should receive lv_obj_t * as first argument which is a pointer to widget object itself.
  • structs should be used via an API and not modified directly via their elements.
  • struct APIs should follow the widgets' conventions. That is to receive a pointer to the struct as the first argument, and the prefix of the struct name should be used as the prefix of the function name too (e.g. lv_disp_set_default(lv_disp_t * disp))
  • Functions and structs which are not part of the public API must begin with underscore in order to mark them as "private".
  • Arguments must be named in H files too.
  • To register and use callbacks one of the following needs to be followed (see a detailed description here):
    • For both the registration function and the callback pass a pointer to a struct as the first argument. The struct must contain void * user_data field.
    • The last argument of the registration function must be void * user_data and the same user_data needs to be passed as the last argument of the callback.
    • Callback types not following these conventions should end with xcb_t.

Copy link
Collaborator

@XuNeo XuNeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks really clean. I'll try it later. Thank you.

src/indev/lv_indev.c Outdated Show resolved Hide resolved
@bjsylvia
Copy link
Contributor

@kisvegabor I used this patch to run on a Mac environment, but there are crashes related to scrolling animation when using the scroll wheel. I will try it on Ubuntu later.

@kisvegabor
Copy link
Member Author

I'm using on Linux as well, but SDL should be the same on both places.

@bjsylvia
Copy link
Contributor

bjsylvia commented Dec 24, 2023

@kisvegabor I try to run it on Ubuntu and it works well.
On the Mac system, there is no crash after compiling without relying on Xcode Clang.

Regarding implementation, there is a suggestion: for the operation of the digital crown, LVGL only need to report the corresponding event device type and scroll event, without the need for conversion to key events.

If the upper layer application or component needs to respond to such scroll events to change button status or adjust progress, it should be handled by the upper layer application logic.
For example, on Google WearOS it notifies the scroll event from a rotary encoder device :
https://developer.android.com/training/wearables/user-input/rotary-input
https://developer.android.com/reference/kotlin/androidx/compose/ui/input/rotary/RotaryScrollEvent

While on Apple WatchOS it reports digital crown rotation event :
https://developer.apple.com/documentation/swiftui/digitalcrownevent

@kisvegabor
Copy link
Member Author

You can get the event device type by lv_indev_get_type(lv_event_get_indev(e)) but I'm not sure if we should use a scroll event here for two reasons:

  1. We don't have a scroll event now 🙂 LV_EVENT_SCROLLED is sent when the widget was already scrolled just to notify anyone who is interested in it
  2. It's not really a scroll but an increment or decrement.

What would be the advantage of approaching from the direction of scroll? I understand that it used in Android, however we haven't followed Android's approach so far and selectively picking a few things might cause conflicts.

@lvgl-bot
Copy link

lvgl-bot commented Jan 9, 2024

We need some feedback on this pull request.

Now we mark this as "stale" because there was no activity here for 14 days.

Remove the "stale" label or comment else this will be closed in 7 days.

@lvgl-bot lvgl-bot added the stale label Jan 9, 2024
@XuNeo
Copy link
Collaborator

XuNeo commented Jan 9, 2024

From the meeting conclusion, using a 32bit number for key event together with its repeat count can eliminate the loop to send key event one by one.

However, the crown or rotary encoder has its own feature differs with keyboard. For example the sensitivity when scrolling list and roller.

For roller, press keyboard LEFT or RIGHT should focus next item. However, using rotary encoder, we may say rotate 10 steps will trigger a refocus to next item in roller.

Android and IOS have treated rotary encoder differently than keyboard, they must have reasons. It would be better we investigate the usage further.

@kisvegabor
Copy link
Member Author

As the feature doesn't any API we don't need to merge it for v9. It means we have some more time think about it.

The sensitivity of the roller is a great example for why we need to treat it separately. Where shall we store this sensitivity value? As a style property? Or a normal widget property?

Probably we need a similar sensitivity for scrolling too.

@lvgl-bot lvgl-bot removed the stale label Jan 10, 2024
@lvgl-bot
Copy link

We need some feedback on this pull request.

Now we mark this as "stale" because there was no activity here for 14 days.

Remove the "stale" label or comment else this will be closed in 7 days.

@lvgl-bot lvgl-bot added the stale label Jan 24, 2024
@XuNeo
Copy link
Collaborator

XuNeo commented Jan 24, 2024

Not stale.

@kisvegabor
Copy link
Member Author

In summary what we should do is

  • having a new LV_EVENT_ROTARY_SCROLL and the ticks parameter.
  • send this event from LV_INDEV_ENCODER too
  • handle LV_EVENT_ROTARY_SCROLL in the existing widgets
  • add a sensitivity style property (1 by default for roller, dropdown, etc)
  • make the scroll size depend on the ticks and the sensitivity of the widget

I think it's a good idea to merge this functionality into the pointer input device type as they share the focused object and the crown is usually used in together with a touch pad.

@kisvegabor
Copy link
Member Author

I added the ROTARY event. Finally added these to lv_indev_t:

/**< Rotary diff count will be divided by this value when widgets are adjusted*/
int32_t rotary_adjust_divider;

/**< Rotary diff count will be multiples by this value when scrolling to get scroll throw size*/
int32_t rotary_scroll_sensitvity;

Copy link
Collaborator

@XuNeo XuNeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Gabor. I just have few comments want to confirm.

@@ -1314,6 +1328,36 @@ static void indev_proc_release(lv_indev_t * indev)
}
}

static void indev_proc_pointer_diff(lv_indev_t * indev)
{
lv_obj_t * obj = indev->pointer.last_pressed;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lv_obj_t * obj = indev->pointer.last_pressed;
Should the user manually click the screen when a new page is loaded, in order to make the rotary works by default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about it too. What about setting it automatically to the screen when a new screen is loaded?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about searching a scrollable obj instead if last_pressed is NULL?

src/indev/lv_indev.c Outdated Show resolved Hide resolved
@kisvegabor kisvegabor force-pushed the feat/crown branch 2 times, most recently from d36b52e to 9e276ca Compare February 5, 2024 17:01
@kisvegabor
Copy link
Member Author

I've slightly reworked it:

  • Configured the base sensitivity in indev->rotary_sensitvity with 1/256 unit
  • Configure the per widget sensitivity with the rotary_sensitvity style property with 1/256 unit again. For scrollable widgets you can set it to a larger value (set dpi / 4 in the default theme), but it's 256 by default, so slider, dropdown, etc are stepping one.

The final diff is calculated like:

  uint32_t indev_sensitivity = indev->rotary_sensitvity;
  uint32_t obj_sensitivity = lv_obj_get_style_rotary_sensitivity(indev_obj_act, 0);
  int32_t diff = (int32_t)((int32_t)indev->pointer.diff * indev_sensitivity * obj_sensitivity + 32768) >> 16;

@lvgl-bot
Copy link

We need some feedback on this pull request.

Now we mark this as "stale" because there was no activity here for 14 days.

Remove the "stale" label or comment else this will be closed in 7 days.

src/core/lv_obj_style_gen.c Show resolved Hide resolved
if(event->wheel.y > 0) dsc->diff--;
#else
indev_dev->diff = -event->wheel.y;
#endif /*LV_SDL_MOUSEWHEEL_MODE*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#endif /*LV_SDL_MOUSEWHEEL_MODE*/
#endif /* __EMSCRIPTEN__ */

src/themes/default/lv_theme_default.c Show resolved Hide resolved
@@ -35,6 +35,9 @@ typedef struct {
int16_t last_x;
int16_t last_y;
bool left_button_down;
#if LV_SDL_MOUSEWHEEL_MODE == 1
Copy link
Collaborator

@PGNetHun PGNetHun Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using "constants" (in our case new SD_MOUSEWHEEL_MODE_ENCODER and SDL_MOUSEWHEEL_MODE_CROWN macros) is better and more descriptive than repeating the values 0 and 1 in files.
Now we know what 0 and 1 mean, but later we will forget, and have to remember, or investigate what they mean.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I'll fix these.

XuNeo
XuNeo previously approved these changes Mar 14, 2024
Copy link
Collaborator

@XuNeo XuNeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Gabor! All looks good to me.

@PGNetHun PGNetHun self-requested a review March 14, 2024 16:07
@kisvegabor
Copy link
Member Author

I've updated the docs and added LV_SDL_MOUSEWHEEL_MODE_ENCODER/CROWN as @PGNetHun suggested. enums can't be used here as the enum values are not resolved at the pre-processing stage so they can't be used in #ifs.

src/drivers/sdl/lv_sdl_window.c Outdated Show resolved Hide resolved
src/drivers/sdl/lv_sdl_mousewheel.h Outdated Show resolved Hide resolved
@PGNetHun PGNetHun changed the title Add crown support to pointer input device feat(indev) Add crown support to pointer input device Mar 15, 2024
@kisvegabor
Copy link
Member Author

@PGNetHun
Fixed, thanks!

@XuNeo XuNeo merged commit 54f9003 into master Mar 19, 2024
16 checks passed
@kisvegabor kisvegabor deleted the feat/crown branch March 19, 2024 07:20
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

Successfully merging this pull request may close these issues.

None yet

6 participants