Page MenuHomePhabricator

Ecore_Input: define data type for joysticks
ClosedPublic

Authored by kimcinoo on Oct 14 2014, 5:34 AM.

Details

Summary

This adds support for joysticks for ecore_input

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes

Add conditional compilation for libudev.

raster requested changes to this revision.May 27 2015, 8:55 PM
raster edited edge metadata.
raster added inline comments.
src/lib/ecore_input/Ecore_Input.h
440

so one last q - why have joystick be a separate init./shutdown - why not just always init as part of ecore_event? why have these at all? basically we'll end up always calling these from elm anyway... so why make them separate anyway?

This revision now requires changes to proceed.May 27 2015, 8:55 PM
kimcinoo added inline comments.May 31 2015, 10:54 PM
src/lib/ecore_input/Ecore_Input.h
440

I thought the joystick event is for specific users such as game developers, so I would like to make these init, and shutdown APIs be used only if it is really necessary.
I was not sure this could be part of ecore_event.

jpeg added a comment.Aug 5 2015, 9:47 PM

@kimcinoo could you update this patch (rebase on git master) and maybe merge the init/shutdown with the rest of ecore_input? Alternatively, I wonder if we could just initialize automagically when the first joystick event is registered.

Question:

  • How does an application enumerate the already plugged-in joysticks?

I believe apps will first initialize the joystick module, and then register the callback? Which makes it impossible to enumerate them at startup time?

src/lib/ecore_input/ecore_input_joystick.c
283

code style: userData is not following EFL conventions. data or user_data are fine
same remark for fdHandler (should just be fd)

414

parenthesis here, or separate code in two lines (code style)

jpeg requested changes to this revision.Aug 5 2015, 9:47 PM
jpeg edited edge metadata.
kimcinoo updated this revision to Diff 6899.Aug 26 2015, 6:32 AM
kimcinoo edited edge metadata.
kimcinoo removed rEFL core/efl as the repository for this revision.
  1. Merge with ecore_input
  2. Follow EFL convention

This patch handles joystick which is connected before ecore input initiation.

is everyone happy with this now?

I don't know much about joysticks (internals of them that is), but looking at this code makes me wonder...

Can't we be using Eeze here instead of udev directly ?? Eeze can already monitor, etc, etc. Should cover what is needed here..

I am also concerned wrt the ecore_main_fd_handlers being added here ... For the case of libinput, I would imagine that these will NOT be needed (as libinput should send us events).

I'm not going to block this getting accepted...but I'd like to get a response on the above Qs/Issues before it lands if possible

It is not possilbe to use eeze here, becuase eeze depends on ecore. So I referred to eeze implementation when I made this patch set using udev.
I'm not good at the libinput. If the libinput is enough to handle this kind of thing, why does not the eeze use the libinput?
And, I'm not sure what the issue is, If the ecore_main_fd_handlers uses same fd.

jpeg added a subscriber: stefan.Jan 5 2016, 6:36 PM

Something important to know about this patch is that it's already used in Tizen, providing a public (ie. stable) API.
I guess we should merge this patch upstream. Maybe even for 1.17? (ping @stefan)

See: https://developer.tizen.org/development/api-references/native-application?redirect=https%3A//developer.tizen.org/dev-guide/2.4.0/org.tizen.native.mobile.apireference/group__Ecore__Input__Group.html

Of course the doc is slightly wrong as it says since EFL 1.15

zmike added a subscriber: zmike.Jan 5 2016, 6:51 PM
In D1538#56589, @jpeg wrote:

Something important to know about this patch is that it's already used in Tizen, providing a public (ie. stable) API.
I guess we should merge this patch upstream. Maybe even for 1.17? (ping @stefan)

See: https://developer.tizen.org/development/api-references/native-application?redirect=https%3A//developer.tizen.org/dev-guide/2.4.0/org.tizen.native.mobile.apireference/group__Ecore__Input__Group.html

Of course the doc is slightly wrong as it says since EFL 1.15

I'm strongly opposed to merging anything for this reason as it sets a dangerous precedent wherein we lose control of our codebase whenever Tizen decides to apply a patch which provides api functions.

jpeg added a comment.Jan 5 2016, 8:19 PM
In D1538#56592, @zmike wrote:
In D1538#56589, @jpeg wrote:

Something important to know about this patch is that it's already used in Tizen, providing a public (ie. stable) API.

I'm strongly opposed to merging anything for this reason as it sets a dangerous precedent wherein we lose control of our codebase whenever Tizen decides to apply a patch which provides api functions.

Yeah, I agree with you, you raise a very valid point. I wanted to let others know about the current situation, that's all. We don't need to rush this for 1.17.

Also, for this specific patch, even though it's marked as rejected, I see no strong objection against it. Probably mostly because no one really knows for sure if this API is correct (ie. future-proof) or not.

zmike added a comment.Jan 6 2016, 8:41 AM

It is not possilbe to use eeze here, becuase eeze depends on ecore. So I referred to eeze implementation when I made this patch set using udev.
I'm not good at the libinput. If the libinput is enough to handle this kind of thing, why does not the eeze use the libinput?
And, I'm not sure what the issue is, If the ecore_main_fd_handlers uses same fd.

I don't see why this is a valid objection to using eeze?

Eeze doesn't use libinput because it was written a number of years before libinput was created.

ok. so one one is complaining about API anymore. the rest is internals/implementation.

of course it would be possible to use eeze here. eeze depends on ecore. ecore_input depends on ecore. this is perfectly possible to so. remember ecore != ecore_input - different libs.

so the point of using eeze is valid. this isn't using libinput - its going to udev/device directly to it does need to use fd handlers to glue events into the mainloop.

so i think this would be good to use eeze - it would mov the udev handling over to there where it already is done.

It seems that the configuration of Eeze should exist between Ecore and Ecore_Input, doesn't it?
Is changing configuration order acceptable?

yeah - the order can change there. that's fine.

kimcinoo updated this revision to Diff 8124.Jan 12 2016, 11:40 PM
kimcinoo edited edge metadata.
kimcinoo changed the visibility from "All Users" to "Public (No Login Required)".

The patch comes with eeze. :]

jpeg added a comment.Jan 13 2016, 5:21 PM

I don't know eeze but this patch seems otherwise OK.
The @since needs to be 1.18

jpeg retitled this revision from [Ecore_Input] define data type for the joystick to Ecore_Input: define data type for joysticks.Jan 13 2016, 5:34 PM
jpeg updated this object.
jpeg edited the test plan for this revision. (Show Details)
jpeg added a project: efl.
jpeg edited subscribers, added: stefan_schmidt; removed: stefan.
jpeg accepted this revision.Jan 13 2016, 5:36 PM
jpeg edited edge metadata.

Looks good.
@since needs to be changed to 1.18 if we merge it during the next round.

zmike requested changes to this revision.Jan 13 2016, 7:02 PM
zmike added a reviewer: zmike.

This needs a number of changes. I'd like to see a lot of those enums renamed to be shorter and slightly less ambiguous.

src/lib/ecore_input/Ecore_Input.h
141

This enum should probably be Ecore_Joystick_Button with corresponding member name changes.

161

This enum should probably be Ecore_Joystick_Axis with corresponding member name changes.

179

I'd like to see this renamed as Ecore_Event_Joystick_Type with corresponding enum member name changes.

src/lib/ecore_input/ecore_input.c
62

If failure is handled in the joystick code, failure should probably also be handled here.

src/lib/ecore_input/ecore_input_joystick.c
47

system_path should probably be type Eina_Stringshare.

59

This should be calloc to avoid uninitialized struct values in fallthrough cases below.

77

This should be calloc to avoid uninitialized struct values in fallthrough cases below.

190

Return type should be on separate line.

236

This leaks devnode.

241

Leak.

248

Leak.

253

eina_stringshare_ref()

260

Need eina_stringshare_del(devnode); at the end of scope to avoid leak.

270

Needs FOREACH_SAFE to avoid corruption during list deletion.

272

These strings will be stringshared, so the comparison can be simplified to either if (syspath == ji->system_path) or if (eina_streq(syspath, ji->system_path)).

280

break after finding the joystick?

299

eina_stringshare_del(syspath); at the end of this scope.

316

Declare variables at top of scope.

This revision now requires changes to proceed.Jan 13 2016, 7:02 PM
thiepha requested changes to this revision.Jan 20 2016, 11:32 PM
thiepha added a reviewer: thiepha.
thiepha added a subscriber: thiepha.

This will make efl fail to build with --disable-libeeze; adding eeze checking would be necessary.

I love this kind of comments. Thank you for valuable comments. I will update soon.

src/lib/ecore_input/Ecore_Input.h
141

I'd like to keep this, because the Ecore_Event_Joystick_Button is used under the ECORE_EVENT_JOYSTICK.

161

I'd like to keep this, because the Ecore_Event_Joystick_Axis is used under the ECORE_EVENT_JOYSTICK.

179

I'd like to keep this, because the Ecore_Event_Joystick_Event_Type is used under the ECORE_EVENT_JOYSTICK.

src/lib/ecore_input/ecore_input.c
62

I'd like to keep this, because the ecore_input_joystick_init could be failed if the eeze is not enabled.

kimcinoo updated this revision to Diff 8230.EditedJan 25 2016, 3:53 PM
kimcinoo edited edge metadata.

Enhance patch set based on valuable comments of zmike, and thiepha.

zmike requested changes to this revision.Jan 25 2016, 4:00 PM
zmike edited edge metadata.
zmike added inline comments.
src/lib/ecore_input/ecore_input_joystick.c
265

This also leaks devnode.

This revision now requires changes to proceed.Jan 25 2016, 4:00 PM
kimcinoo updated this revision to Diff 8257.Jan 27 2016, 10:21 PM
kimcinoo edited edge metadata.

Fix memory leak caused by eina_stringshare_add of eeze_udev_syspath_get_devpath.

zmike accepted this revision.Feb 1 2016, 8:54 AM
zmike edited edge metadata.

Eeze usage looks good.

thiepha accepted this revision.Feb 1 2016, 4:47 PM
thiepha edited edge metadata.
kimcinoo updated this revision to Diff 9125.May 19 2016, 10:22 PM
kimcinoo edited edge metadata.

Using "/dev/input/js" not "/dev/input/event" because user does not have read permission of "/dev/input/event".

raster added a comment.Jun 3 2016, 2:57 AM

ok. so we're looking good then right? are we happy with this? it seems so - i just want to have some time for objections - if everyone is happy let's merge in the next week or so

jpeg added a comment.Jun 12 2016, 11:00 PM

The patch looks good (I guess?) but I wonder.
Do we need the ecore events and ecore APIs? Following recent changes in EFL 1.18 input events at evas level are now eo events, and their data is itself an eo object (Efl.Event stuff). Do we really need those new APIs at ecore level? Or can we forward the core events to the Window and propagate them to apps the same way as key events?

i would just go with this in the ecore_input level and treat it as internal glue right now. :) we will expose in future as eo events on objects with callbacks etc.

jpeg accepted this revision.Jun 13 2016, 3:29 AM
jpeg edited edge metadata.

Very well then, let's merge this in!

Closed by commit rEFLf70be6eb28a2: Ecore_Input: define data type for joysticks (authored by kimcinoo, committed by Jean-Philippe Andre <jp.andre@samsung.com>). · Explain WhyJun 13 2016, 4:29 AM
This revision was automatically updated to reflect the committed changes.