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

Refactor communicators + work of mac os (based on #72) #90

Closed
wants to merge 3 commits into from
Closed

Refactor communicators + work of mac os (based on #72) #90

wants to merge 3 commits into from

Conversation

sergioisidoro
Copy link
Contributor

I wanted to contribute, didn't know exactly where to start.

I picked up #72 made some refactoring, and fixed the tests. Also added some work for tox, so we can test multiple python versions.

Tried to handle the Keyboard interrupt better on Mac os (it would stay a zombie process blocking the device)

@ttu
Copy link
Owner

ttu commented Jan 3, 2020

Thanks for the great work!

I have kept the Bleson communication in an own branch as it still is in the early alpha stage and hasn't been updated in a year. It is not yet working correctly in all the cases. I have been checking some other cross platform libraries, but haven't found one yet.

This package also needs to support Python 2.

This PR could be separated to multiple PRs. First we could make the fixes and changes to the master branch and then update the Bleson branch and make it optional to either use Bluez or Bleson. How does that plan sound? Also then if we find some better cross platform library, adding it would be pretty easy.

PRs could be for example:

  • RunFlag
  • Adaptors (dummy and nix_hci)
  • Code cleanup
  • Tox (update also developer_notes.md)
  • Bleson option when using Python 3
    • Own Adaptor
    • Different DataFormats convert_data functions needed for Bleson and Bluez

I really appreciate all the work you have done and hope that you still have interest to split this to smaller pieces. Unfortunately my time is pretty limited at the moment, but I can try to help the best I can.

@sergioisidoro
Copy link
Contributor Author

sergioisidoro commented Jan 3, 2020

Hi there !
Yes, that sounds good.

Actually that was my initial idea, to make a PR just for the refactoring (and the first commit is quite easy to cherry-pick) but then I got into the rabbit hole of the bleson because I'm running on mac os.

I'll break this one down, and if you need some help I can lend a hand.
This is a really interesting project.

@sergioisidoro
Copy link
Contributor Author

Ok, here's the first one:
#91

@sergioisidoro
Copy link
Contributor Author

Second one:
#92

@sergioisidoro
Copy link
Contributor Author

Btw, I just came across this project. Could be interesting to get in touch
https://github.com/hulttis

@ttu
Copy link
Owner

ttu commented Jan 5, 2020

@hulttis project looks interesting. Thanks for the tip!

Besides Bleson this is another cross platform BLE project I have been following https://github.com/hbldh/bleak. I still doesn't have macOS support, but when it does, it might be a worth to check out.

@sergioisidoro
Copy link
Contributor Author

This can be closed in favor of #91 #92
I don't think the keyboard interrupt has been implemented on any other PR, but that can be added later to the draft PR -- and there's an issue for it already anyway -- #80

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.

2 participants