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

Kegtron BT matcher too generic #134081

Open
patman15 opened this issue Dec 27, 2024 · 7 comments
Open

Kegtron BT matcher too generic #134081

patman15 opened this issue Dec 27, 2024 · 7 comments
Assignees

Comments

@patman15
Copy link
Contributor

The problem

Hi! I got a issue report for my custom integration that support Ective battery BMS. Unfortunately, those batteries also use the manufacturer ID 65535. According to this thread the number is actually reserved, although I could not find the source in the BT specification.

The very general filtering for manufacturer ID 65535 thus can easily cause a conflict with other devices. For reasons I do not really understand (see additional information below) this in turn can cause a Invalid Flow Specified exception together with a second integration that uses that number (alone). The minimal side-effect that is reproducible is that I need to have kegtron-ble as requirement for my integration tests, since the advertisements of the Ective battery trigger also Kegtron, which I'd like to avoid to have.

I therefore suggest to add "local_name": "Kegtron*", to the manifest as the specification states "Kegtron xxxxxx" (where xxxxxxx = MAC address suffix to be part of the advertisement Type = 0x09 (Complete local name).

I was about to suggest a PR, but then I saw in the tests for this integration, that the test advertisements do not follow that specified format, so I'm not sure how real devices advertise. Since I do not own such a device, I cannot clarify, could you please do so and comment on restricting the BT matcher more?

What version of Home Assistant Core has the issue?

core-2024.12.0.dev0

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant Core

Integration causing the issue

kegtron

Link to integration documentation on our website

https://www.home-assistant.io/integrations/kegtron/

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

Documentation says the devices should provide a local_name "Kegtron xxxxxx" (where xxxxxxx = MAC address suffix)

I could not reproduce the issue the user reported (HA core, devcontainer, or HA OS), as HA anytime would install kegtron-ble for me and the flow would work. The message "invalid flow specified" only occurs if kegtron-ble is missing, which is reproducible in tests without that requirement installed via PIP.

@home-assistant
Copy link

Hey there @Ernst79, mind taking a look at this issue as it has been labeled with an integration (kegtron) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of kegtron can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Renames the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign kegtron Removes the current integration label and assignees on the issue, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

(message by CodeOwnersMention)


kegtron documentation
kegtron source
(message by IssueLinks)

@Ernst79
Copy link
Contributor

Ernst79 commented Dec 27, 2024

If I remember correct, these Kegtron devices send two type of advertisements. One has the name in it, but the second one does not. This is also explained in the docs. So, filtering on “Kegtron*” will cause it to miss the second type of advertisements. And the second one, is the one with the relevant data in the 13FFFFFF part. Kegtron is using the manufacturer uuid FFFF. I don’t think that is an official UUID, they just use it. As your link explains, it should be for testing purposes.

I’m not sure how to make ithe filtering more restricted. I thought the port name was editable, so can be anything. The only thing we might be able to add is filtering on the length of the advertisement, as it is fixed at 31 bytes.

@patman15
Copy link
Contributor Author

Hey! Thanks for the quick response, highly appreciated!

If I remember correct, these Kegtron devices send two type of advertisements. One has the name in it, but the second one does not. This is also explained in the docs. So, filtering on “Kegtron*” will cause it to miss the second type of advertisements.

According to the explanation for Bleak, which is used by HA, the two messages should be combined, so filtering would not make a difference if I correctly understand. If you want me to test that with a fake device based on ESP32, I can do, if you even consider a change to your code. 🙏

The only thing we might be able to add is filtering on the length of the advertisement, as it is fixed at 31 bytes.

That is not feasible as the BT matcher does not support length filtering.

@Ernst79
Copy link
Contributor

Ernst79 commented Dec 27, 2024

I don’t have a Kegtron device myself, so I can’t check if it sends both messages in one advertisement. But I don’t think it does. I also don’t know how to combine the two messages

@patman15
Copy link
Contributor Author

I also don’t know how to combine the two messages

Well my point is (based on the links I referenced), you don't have to do so. The underlying libraries will do that for you, thus the case that you will miss one messages cannot occur. They are first combined and then matched, since the first contains the name and the second the data, you will get one with both elements.

@Ernst79
Copy link
Contributor

Ernst79 commented Dec 28, 2024

I don’t have a Kegtron device myself, so we should find someone who has a Kegtron device to check this, I guess.

@patman15
Copy link
Contributor Author

I don’t have a Kegtron device myself, so we should find someone who has a Kegtron device to check this, I guess.

Hmm, maybe @alanquillin can help, his code
https://github.com/alanquillin/kegtron-v1-api-proxy/blob/87317e26863b0907c198bb71ed3f95b3b2cc4cf8/src/scan.py#L111
supports my suspicion as well that you can filter for the name start and manufacturer ID in parallel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants