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

Migrate all the D-Bus calls to GDBus. #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tommie-lie
Copy link

Disclaimer: This pull-request does not "fix" an "issue" but is rather a proposal and is a 1:1 migration. I plan on expanding on that idea, creating proper DBusProxy subclasses, somehow resect the requirement for a non-user-controlled mainloop and thus improve the integratability of gatt-python for my own projects. If you think this migration is rubbish, just close the pull-request ;-)

--

Given that the python-dbus interface only receives fixes and is considered by
its author and maintainer as un-pythonic and is not recommended by him, it seems
more logical to use another, more recommendable interface.

As gatt-python already makes use of PyGI and GLib, it makes sense to use GDBus which
comes as a part of GLib.

This also effectively removes the python-dbus dependency.

@tommie-lie tommie-lie force-pushed the gdbus branch 2 times, most recently from 9468b3c to 8252600 Compare November 4, 2017 14:03
@larsblumberg
Copy link
Contributor

I'm the original author of this library and I find your PR very promising. As it is removing the dependency of dbus and using GDBus instead, I consider this PR a good step forward.
Unfortunately I don't have suitable hardware to test your PR as I'm not working with team Senic at the moment. Maybe someone from @getsenic can give the PR a try? It should make their build process slightly simpler.

@mtiutiu
Copy link

mtiutiu commented Nov 10, 2017

@tommie-lie This is really great. Tested your PR using real hardware and I fixed some issues - seems all well now with some minor exceptions but not critical for the applications. I created another PR on your fork(I created a PR for this PR basically 😄 ).

@larsblumberg @tommie-lie
What was tested:

  1. Discovery - works
  2. Connecting - works
  3. Notifications - works

Given that the python-dbus interface only receives fixes and is considered by
its author and maintainer as un-pythonic and is not recommended by him, it seems
more logical to use another, more recommendable interface.

As gatt-python already makes use of PyGI and GLib, it makes sense to use GDBus which
comes as a part of GLib.

This also effectively removes the python-dbus dependency.
@tommie-lie
Copy link
Author

Sorry for having been silent for so long, but I had some quite busy weeks and did not work on my Bluetooth pet project.

I'm a bit puzzled as to why my code ever worked with the issues found by @mtiutiu but they are fixed now and I found some other placed where the error-handling was wrong. I also rebased the commit to the latest master (and adapted the new DeviceManager.remove_all_devices method).

@mtiutiu I sincerely apologize for not giving credit to you in the git history but I found it rather hard to integrate your PR (see there for more info). Please let me know if you are okay with this or if I should add you to the commit message.

@mtiutiu
Copy link

mtiutiu commented Jan 2, 2018

@tommie-lie You can merge your changes I don't need any credits for this. I'm glad I could be of help.

@larsblumberg
Copy link
Contributor

@TheMeaningfulEngineer Where you guys able to test this PR? It should make your build process easier as it makes the necessity for the dbus lib obsolete.

@TheMeaningfulEngineer
Copy link

Hey @larsblumberg
given the current load, and that it's a feature improvement I'd say somewhere in March.

And thanks @tommie-lie and @mtiutiu, sorry for not being very responsive at the moment.

@larsblumberg
Copy link
Contributor

While I wouldn't consider the dbus replacement a feature (from the user perspective) I understand that you got more important things to do in the moment. I'm looking forward to seeing those changes implemented at some point as it eases the overall build process, not only for the Senic Hub.

@TheMeaningfulEngineer
Copy link

Hey peeps,
got a chance to test the PR with our current stack.
Unfortunately it broke it and I can't spend the time atm to debug it,
will check it out in the soonest available time slot.

Where it fails is on the wrapper library that abstracts the gatt and allows
us to think in terms of functionalities of the device.
@larsblumberg
Do feel free to rephrase, that how I understood the stack :)

  File "/home/root/nuimo-linux-python/nuimo/nuimo.py", line 38, in start_discovery                                                                                                                          
    super().start_discovery(service_uuids=Controller.SERVICE_UUIDS)                                                                                                                                         
  File "/home/root/gatt-python/gatt/gatt_linux.py", line 142, in start_discovery                                                                                                                            
    self._adapter.SetDiscoveryFilter('(a{sv})', discovery_filter)                                                                                                                                           
  File "/usr/lib/python3.5/site-packages/gi/overrides/Gio.py", line 157, in __call__                                                                                                                        
    arg_variant = GLib.Variant(signature, tuple(args))                                                                                                                                                      
  File "/usr/lib/python3.5/site-packages/gi/overrides/GLib.py", line 243, in __new__                                                                                                                        
    (v, rest_format, _) = creator._create(format_string, [value])                                                                                                                                           
  File "/usr/lib/python3.5/site-packages/gi/overrides/GLib.py", line 132, in _create                                                                                                                        
    return self._create_tuple(format, args)                                                                                                                                                                 
  File "/usr/lib/python3.5/site-packages/gi/overrides/GLib.py", line 167, in _create_tuple                                                                                                                  
    (v, format, _) = self._create(format, args[0][i:])                                                                                                                                                      
  File "/usr/lib/python3.5/site-packages/gi/overrides/GLib.py", line 135, in _create                                                                                                                        
    return self._create_dict(format, args)                                                                                                                                                                  
  File "/usr/lib/python3.5/site-packages/gi/overrides/GLib.py", line 194, in _create_dict                                                                                                                   
    (val_v, rest_format, _) = self._create(rest_format, [v])                                                                                                                                                
  File "/usr/lib/python3.5/site-packages/gi/overrides/GLib.py", line 126, in _create                                                                                                                        
    v = constructor(args[0])                                                                                                                                                                                
TypeError: argument value: Expected GLib.Variant, but got list

@Snevzor
Copy link

Snevzor commented Mar 21, 2019

@mtiutiu could you please elaborate on what issues were fixed for you?

@mtiutiu
Copy link

mtiutiu commented Mar 21, 2019

@Snevzor I'm sorry but this thing is too old for me now to remember all the details. The initial problem was due to some incompatible methods signature and/or class constructors. But to be honest I don't remember which and the "why". This PR cleaned those up and made my custom application a bit more stable. Initially I had to do some hacks to overcome some related issues - sorry I can't remember those either.

I'm sorry for not being too helpful here but I really don't remember the details and I don't have the time and luxury to search in my old messy code for that application (although it seems to work even now as it is 😄 )

@Snevzor
Copy link

Snevzor commented Mar 21, 2019

@mtiutiu no worries! Thanks for responding anyhow.

@larsblumberg
Copy link
Contributor

I would like to continue the work on this PR.

@mtiutiu Is your PR to this PR already included?

@mtiutiu
Copy link

mtiutiu commented Mar 25, 2019

@larsblumberg
As far as I know it should but @tommie-lie knows better as he worked more on it.

@rngtng
Copy link

rngtng commented Mar 9, 2020

Any updates on this? I’d love to see this merged and especially removing the dependency. Gave me headaches. Anything I can help with?

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.

6 participants