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

Default to gatttoolbackend with bluepybackend backup #27673

Closed
wants to merge 1 commit into from

Conversation

helenclarko
Copy link

This change corrects an issue with bluepy disconnecting from devices after some time.
Gatttool is currently unaffected by this issue.

No code change, code has just been rearranged

Description:

Related issue (if applicable): same change has fixed mitemp_bt sensor: #27672

Checklist:

  • [ No] The code change is tested and works locally.
  • [N/A ] Local tests pass with tox. Your PR cannot be merged unless tests pass
  • [ Yes] There is no commented out code in this PR.
  • [ Yes] I have followed the development checklist

If the code does not interact with devices:

  • [yes ] Tests have been added to verify that the new code works.

This switch corrects an issue with bluepy disconnecting from devices after some time. 
Gatttool is currently unaffected by this issue.
@homeassistant
Copy link
Contributor

Hi @helenclarko,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@probot-home-assistant
Copy link

Hey there @Danielhiversen, @ChristianKuehnel, mind taking a look at this pull request as its been labeled with a integration (miflora) you are listed as a codeowner for? Thanks!

@helenclarko
Copy link
Author

I'm new to creating a pull request for Home assistant, please excuse any mistakes that are made.

@ChristianKuehnel
Copy link
Contributor

I would not encourage using gatttool:

  • the tool is deprecated
  • it's not available in hass.io (to my knowledge)
  • it's much slower

I would rather like to have someone understand what is really causing the problem and fix the root cause.

@helenclarko
Copy link
Author

It's available in hassio and although it may be slower it works.

@helenclarko
Copy link
Author

helenclarko commented Oct 15, 2019

@ChristianKuehnel
Could it have anything to do with this issue?

IanHarvey/bluepy#344

@balloob
Copy link
Member

balloob commented Oct 15, 2019

We should not prefer/promote deprecated packages

@sibbl
Copy link

sibbl commented Oct 16, 2019

We should also not have broken components, should we?

While I truly understand the wish to not have "ugly code" in the repo, I'd appreciate if there was any way to have various Mi* bluetooth devices working properly again after 4+ months.

As both ways are in the code currently, would it be an option to have a component config entry to switch between them, defaulting to bluepy?

@ChristianKuehnel
Copy link
Contributor

My point is: Do not implement workarounds. Understand the root cause of the problem and fix it. For whatever reasons miflora is working fine on the 3 machines I'm running it on.

@pvizeli
Copy link
Member

pvizeli commented Oct 16, 2019

@balloob the issue is, that Bluetooth and python are not friends and maybe also not friends with Linux. The new kernel Bluetooth interface is so bad supported that to use Gatttool is in most case the best alternative. The next issue with Bluetooth is that the python library is most older than 5-6 years. Some library was forked under the new name but until now, there is no real stable library.

However, the needed library also depends on the driver blob and Kernel version. So maybe not a bad idea to allow a configuration/options for this because we can't handle that correct for every one automatic.

@ChristianKuehnel
Copy link
Contributor

@pvizeli In that case we should make this configurable from the configuration.yaml? So that users can which Bluetooth library works for their setup?

Or should HomeAssistant offer a generic Bluetooth API and then pick the right implementation underneath that fits the hardware automatically?

@helenclarko
Copy link
Author

helenclarko commented Oct 16, 2019

Guess the question is does bluepy work for anyone at the moment, with this current bug?
If it is hardware related and bluepy simply doesn't work with some hardware, then an option to choose the backend within the configuration.yaml would work fine.

As for Miflora running fine on 3 of your machines, I'm not sure how that helps to solve the issue. Perhaps you can send me one haha?
What I'm getting at is that I guess some hardware doesn't play nice with bluepy and thinking about it now this solution is not the best as it may have been that other hardware wouldn't play nice with gatttool (I dunno). Having this discussion is really what was needed, so thank you for taking the time to look at it.

@ChristianKuehnel
Copy link
Contributor

Can we figure out, which hardware is causing the problem? And then either push bluepy to address this or analyze/fix it ourselves? Or maybe give our users a hint on which hardware to use/avoid.

What I wanted to say with my 3 machines: My problem here is that I cannot reproduce the problem, so I have no clue how to proceed,

@pvizeli
Copy link
Member

pvizeli commented Oct 17, 2019

Like I said, the issue is the Linux Kernel/Bluez interface, which have different interfaces and depend on what the vendor blob use, it work different. The library is only so stable as the Linux Kernel allow to be and the current Bluetooth support is for linux is bad add all. Some vendor/drivers/blobs work fine with new interface but bad with old but that is also happens into other direction.

@frenck frenck mentioned this pull request Dec 5, 2019
4 tasks
@balloob
Copy link
Member

balloob commented Jan 11, 2020

Going to close this. One is not better than the other, so swapping them around just makes it worse for other people.

@balloob balloob closed this Jan 11, 2020
@pvizeli
Copy link
Member

pvizeli commented Jan 11, 2020

The solution would be to make it configurable

@lock lock bot locked and limited conversation to collaborators Jan 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants