Page MenuHomePhabricator

bluez5 support - a new module with gadget etc.
ClosedPublic

Authored by raster on May 9 2018, 9:55 PM.

Details

Summary

bluez4 support is now basically dead. nothing ships it anymore. bluez5
is a new api that is rather different so new code. also a new gui with
more complete features etc.

not everything is done as i'd like. need:

  1. many more icons for device types (60-70 maybe?)
  2. a few specific custom icons for some action buttons (like

pair/unpair)

  1. icons for group headers
  2. gadget status - the gagdte itself displays zero status. it's a

button to display a popup. that's all.

  1. still ye olde gadcon gadget as that is what i use etc.

Diff Detail

Repository
rE core/enlightenment
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
raster created this revision.May 9 2018, 9:55 PM
raster requested review of this revision.May 9 2018, 9:55 PM
zmike added a comment.May 10 2018, 8:55 AM

I think this is a good feature, and one which has been requested for a long time, so thanks for taking the time to work on it.

Before going deeper into review, I'd like to ask for more details on why you chose to use the obsolete gadcon API over either the gadget API or sandbox infrastructure. While you mention "that is what i use etc.", this is not consistent with the development direction of the community/project for the past 2+ years. In fact, all gadcon gadgets have since gained gadget equivalents, and there are a number of gadgets which either were not written to include gadcon implementations or have since removed them.

At this time, I'm opposed to adding any new gadgets in-tree which implement only the gadcon API as this infrastructure has been unmaintained and inactive for years despite having numerous known bugs. It seems that the sandbox infrastructure would be better suited as this gadget does not require access to enlightenment internals and it would simplify the gadget code considerably. I'm sure @stephenmhouston would be willing to help out with this since he's done considerable work in this area.

I'd like to ask for more details on why you chose to use the obsolete gadcon API

i use shelves. still. they are the stable still public and supported api. they work and work well and i want and need something that works for me now rock solidly. i also started with the bluez4 code and stripped it down to just the gadget then built on top thus that's how it started. 95% or so of the code has nothing to do with gadcon/gadgets so its shareable between both. it can be added. i'm not against that at all and am willing to do it myself, but i already had a few people ask to share this and use it and play with it etc. and people who just plain need it, so the sooner it goes in, the better. new gadget support can be added no problem. but i don't see that as necessary right now. i have tested this across several devices and think i have gotten this good but i'm not totally sure if it works for others as well. sharing will determine that and i'm pretty sure those people asking me to share it are still on shelves too. :)

despite having numerous known bugs

i spend my days and nights and weeks without seeing them. it's stable and very usable. i have tried bryces and found them to be ... not so great and littered with far more bugs than shelves (from usability bugs to visual snags to segfaults). i'll leave it at that.

It seems that the sandbox infrastructure would be better suited

absolutely not. that would add a fat memory footprint overhead for something that runs all day long doing very little other than waiting for agent requests sometimes from bluez and the odd occasional paring or unpairing of some device. consuming all the overhead of a process (which is multiple mb's) for essentially doing nothing all day offends me to no end. it's a massive waste of resources. any functionality that is similar (network/wireless controls for example) is in the same boat. same for clocks, temp, cpu etc. gauges etc.

simplify the gadget code considerably

it wouldn't simplify really worth talking about. it'd remove some code from the providing of the gadget and gadcon iface handling, but then add extra code to re-do the popups that gadcon could do with creating ctxpopups and placing (minus ~5-10 lines on one gadcon side, plus ~10 lines on the popup side). we'd be about even given just the rest of the code related to popup (not its content) and gadcon.

so all in all i can add the gadget support, but i'd rather not iterate over this to do it. it's simple enough to read e.g. time module code to see how it works. i'll put it in once i can also land some other fixes i now also have pending because i have to be on a branch for this.

I don't mind helping you convert it to the E gadget. Just let me know if you want the help.

@stephenmhouston - thanks. i'll be happy to ask for advice, but it's the kind of thing i like to do. i haven't done it before, but i've read the relevant code, so if i run into problems i'll definitely ping. but i'm happy to do this as it is ultimately something that needs doing but wasn't my priority (getting bz5 to work and work well is and was and to dot my i's and cross my t's). there's lots of other things that still need doing like my points in the log and it needs wide testing.

zmike added a comment.May 14 2018, 9:23 AM

I've taken some time to do a moderately deep review of this. Overall it looks good, though I've found some problem areas which can be improved.

I'm still opposed to merging this gadget to master until either a comparable e_gadget implementation exists or a sandbox port is created. You've indicated some dissatisfaction with sandboxing--which is understandable since there is still work pending which addresses the points you raised--so I'll be anticipating the e_gadget version either in another submitted patch using git-phab or merged into this patch. Until that time, this seems like a fine candidate for a feature branch testing.

It seems that bluez4 is dead. Is there any point in keeping the corresponding module or will you be following up with another patch to remove it?

I don't see 4. in your list of remaining issues as a blocker to eventually merging this, but I think a task should be created for it (and possibly the rest of the items) to ensure that it isn't forgotten since this is a significant feature.

Thanks again for taking the time to tackle this, as it is definitely a much-requested feature.

src/modules/bluez5/bz.c
4

Another symbol which requires EINTERN.

29

A comment to explain the difference in timings here will avoid having someone "clean up" this code later by removing one of the cases.

69

Given that all these callbacks are set exactly once upon init and are always set with the same function, it seems like it would simplify the code and make it more readable if instead bz_init just set a bool variable to enable calls while the agent was active?

src/modules/bluez5/bz.h
59

I would appreciate it if you could namespace all these defines with e.g., BLUEZ5_ to make the code more immediately recognizable at a glance.

178

Module-local symbols must be marked with EINTERN to prevent possible symbol collision.

181

These function names should also be namespaced.

src/modules/bluez5/bz_agent.c
113

Errors should go to stderr, not stdout.

131

Perhaps it would be best to use a more descriptive path here, e.g., /org/bluez/enlightenment_agent to improve dbus debugging.

Also it would help readability and consistency if this string could be a define or static variable somewhere local within the module.

269

nit: This method is actually UnregisterAgent.

src/modules/bluez5/bz_obj.c
246

I'm of the opinion that these macros make the below code considerably harder to read for no gain; eldbus tracks pending requests internally and should cancel them automatically when the proxy is freed--the only reason that I can see for the macro use?

323

This reads a bit confusingly at first since there is a similar case on L300. A comment or consolidation into the original block (using strncmp) could help make this easier to read.

597

This mechanism appears to be broken. Consider the following:

  1. obj_table has a free callback which calls obj_unref
  2. This hash entry is only deleted here or from obj_shutdown
  3. If the hash entry is deleted here, then it means this function will be called recursively, resulting in a double free on L626

Recommend either using eina_hash_set to avoid the free callback for this case or reworking the mechanism.

603

See above comment regarding eldbus managing pending calls internally.

622

This signal handler should be automatically destroyed on L613; attempting to destroy it again here is an invalid memory access.

680

nit: Variables should be declared at the top of the scope.

743

There should be no need to track this pending call or the signal handlers below since they are internally managed by the proxy.

src/modules/bluez5/e_mod_agent.c
48

Ideally this should be an elm_popup instead of an internal window. A popup acquires and maintains focus more reliably than a window on all display systems, and it is also consistent with all gadgets created within the past few years.

Example code can be found here: https://git.enlightenment.org/core/enlightenment.git/tree/src/modules/wireless/wireless.c#n867

201

It may be worth using <hilight> for the device name instead of quotes since this would be consistent with highlighting used in similar cases.

220

Same as above for <hilight> tags.

238

Same as above for <hilight> tags.

260

Same as above for <hilight> tags.

280

Same as above for <hilight> tags.

298

Same as above for <hilight> tags.

src/modules/bluez5/e_mod_main.c
9

This must be EINTERN to avoid symbol collision.

src/modules/bluez5/e_mod_main.h
31

This and similar symbols must be EINTERN to avoid symbol collision.

src/modules/bluez5/e_mod_popup.c
357

evas_object_evas_get(base) will always return e_comp->evas

480

This and all other occurrences of Elm_Widget_Item should be Elm_Object_Item since this is using the legacy API.

src/modules/bluez5/e_mod_util.c
262

nit: Should be Unknown

zmike requested changes to this revision.May 14 2018, 9:23 AM
This revision now requires changes to proceed.May 14 2018, 9:23 AM

I'm still opposed to merging this gadget to master until either a comparable e_gadget implementation exists

does it matter? what is the value? there is no "we must keep clean history" policy here. i can add it later. i agreed to adding the e gadget support. you don't trust me to add it? this is heavily hurting my workflow right now and really getting under my skin. don't forget... i founded this project, wrote more code than most and you now want to treat me like i'm untrusted. i'm being courteous with a heads up on this.

or a sandbox port is created

"hell no". in fact notice here to you specifically: any e modules or gadgets i find using external process sandboxes will be reverted or nuked unless there is an incredibly good reason for an external process (e.g. a needs to sandbox for privacy/security or something else justifiable when push comes to shove). i will not have enlightenment become a bloated memory swamp of processes. i worked hard over many years to ensure e is not bloated. i will not see it undone.

i'm pushing back really hard here because it seems you didn't get the message earlier when this appeared... the memory will bloat per process by multiple mb and quicklaunch will never solve this. threads+fork are incompatible. read https://thorstenball.com/blog/2014/10/13/why-threads-cant-fork/ ... not to mention fd's being a nightmare. and by fork i mean fork + keep running, not fork + then exec soon after. it's not going to solve it. you haven't explained how i or the above article are so wrong and that it will. you'll remove all threads from efl? something else? the more threads - the worse. in fact just 1 already calls it quits on this (and we have multiple in efl now as core features). you've made your point about quicklaunch saving the world. it's not going to for starters for the reasons above.

You've indicated some dissatisfaction with sandboxing

it's not dissatisfaction. it's outright unbridled and vehement opposition to it for many reasons i have given. the work pending won't address the points raised - see above. threads are going to only be used more and more to get stalls out of the main loop and to make use of more cores and it's not solvable without effectively neutering the gains of sharing dirty pages (you have far fewer dirty pages and few if any shared data structs like modules), so it's still bloated.

It seems that bluez4 is dead. Is there any point in keeping the corresponding module or will you be following up with another patch to remove it?

undecided. i don't know if anyone happens to use it still, and the voices of discontent from users who got upgraded to bluez5 and didn't want it would be the indicator. at this point i see it as orthogonal but mildly related.

I don't see 4. in your list of remaining issues as a blocker to eventually merging

don't take that todo list as things i must to do merge. they are just things i must do and iw as planning on doing over time when this lands. i don't intend to create any tasks. i know what i have to do.

Thanks again for taking the time to tackle this, as it is definitely a much-requested feature.

you're welcome. so far i'm not thrilled with this review process. it's pitting a stick in my spokes. i'll address what i say i will, but i want to get on with doing things.

src/modules/bluez5/bz.c
4

see above. it doesn't. or won't once i fix the dlopens...

29

sure.

69

actually this is how i designed it. i clearly split the back-end and gui with core protocol and associated logic being isolated out via callbacks. this makes it easier to maintain and re-use.

src/modules/bluez5/bz.h
59

they are private to the module. they never go outside. i guess i can change them though i fail to see the value.

178

good point. actually the issue is with e_module.c ... it shouldn't use RTLD_GLOBAL ... :) because all the module init/shutdown functions then pollute global namespace. so actually... i'll solve that instead.

181

i can namespace if u want...

src/modules/bluez5/bz_agent.c
113

actually it should go to gui... with a dialog. i just haven't done all of the dotting i's and crossing t's yet of this as its a low priority item vs other things still to do. so this is basically a placeholder until i do that.

131

well the above is what the bluetoothctl client uses, so i mimic'd. it works. i can change it.

and i disagree on the #defines or variables. whenever i read code that does this (dbus), it annoys me because i then have to jump around to find the actual dbus string used, which is what is important. so from the point of view of someone who just spent a while reading bluez code (bluez4 module, bluez code itself)... it's better with strings inline. they aren't going to "change in 1 place" as they are tided to the api.

269

indeed it is.

src/modules/bluez5/bz_obj.c
246

i guess that will clean out some code...

323

no /org/bluez is different from /org/bluez/xxx - yes. i could use strncmp. i would have to count the chars then and more likely get it wrong, so this was a safer bet. i will comment it though

597

just should flag it before the has del here, not after... oddly i never had this happen - it worked perfectly for me. when it was a stand-alone test app and as a module.

622

well oddly enough the magic checks don't complain...

680

well technically in c99 you don't have to, but yes, it's nicer to find all your vars there.

743

ok.

src/modules/bluez5/e_mod_agent.c
48

actually this is temporary. i plan to merge these into the gadget popup... i am not happy with the dialogs popping up. i just dislike the look and usability. if i put it into the popup with the other content then its grouped and you know its a bluetooth thing.

consider this just a stage of development

201

as above - i plan to move this around into the popup and make it different. i also dislike the look and usability of it. this is a perfect example where the request should be in the list of devices so the item that is requesting actually has the request inside of it...

src/modules/bluez5/e_mod_main.c
9

as above

src/modules/bluez5/e_mod_main.h
31

as above. they dont have to be if we loaded local which is what we should have done anyway...

src/modules/bluez5/e_mod_popup.c
480

good point

src/modules/bluez5/e_mod_util.c
262

indeed. typo. but oddly my copy has the typo fixed ... somehow it didnt make it into this diff

This revision was not accepted when it landed; it landed in state Needs Revision.May 18 2018, 5:49 AM
This revision was automatically updated to reflect the committed changes.