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

Allow ibeacons with no name but a allowlisted minor #104419

Closed

Conversation

chatziko
Copy link
Contributor

@chatziko chatziko commented Nov 23, 2023

Proposed change

This is an attempt to workaround #80357. The ibeacon integration ignores beacons with empty/default name, to avoid some users getting lots of "garbage" devices.
The problem with this, however, is that several "software" beacons do not allow the user to set a name.

Most notably, this is the case of the HA android companion app, setting a name seems to be techincally challenging. As a consequence, users get the totally unexpected behaviour that HA does not recognize beacons from its own app! (Other android beacon simulator apps have the same issue.)

Whitelisting UUIDs could be a solution, but it's not an ideal user experience. Each user would need to find and configure their UUID, which could in principle be automated, but with further complications.

So the workaround proposed by this PR is to whitelist a list of magic minor values instead (currently set to 40004). Any beacon with minor 40004 is accepted, even if its name is empty.

Then, the companion app can be configured to have 40004 as the default minor (and document its meaning).

Advantages:

  • Simple solution
  • Completely transparent to the user (after changing the default value in the companion app)
  • Can be used with any software or hardware beacon that allows changing the minor (very common)
  • Can be easily combined with other solutions (eg UUID whitelist) in the future

Drawbacks:

  • A bit hacky solution, with hard-coded whitelist
  • The user is no longer free to choose its own minor value (but the major can still be used)
  • Small risk of still getting some garbage devices that coincidentally use 40004 (in my limited testing I didn't see any).

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

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

Code owner commands

Code owners of ibeacon can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign ibeacon Removes the current integration label and assignees on the pull request, 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 pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@bdraco
Copy link
Member

bdraco commented Nov 23, 2023

I think I'd be ok with this if the android app merges a change to make this the default value.

Also please adjust white list to allow list in the PR description and code

@chatziko chatziko force-pushed the ibeacon-whitelisted-minor branch from c6f2100 to d013f0d Compare November 23, 2023 16:11
@chatziko
Copy link
Contributor Author

I think I'd be ok with this if the android app merges a change to make this the default value.

Cool, I'll prepare a PR, pretty confident they'll merge it.

Also please adjust white list to allow list in the PR description and code

Done.

@chatziko
Copy link
Contributor Author

chatziko commented Nov 23, 2023

Small improvement: since the old companion defaults are Major = 100, Minor = 1, which are not that likely to exist elsewhere, when an ignored beacon has these values we create a persistent notification with instructions for fixing the problem.

@chatziko chatziko force-pushed the ibeacon-whitelisted-minor branch 2 times, most recently from 02203b7 to 01df260 Compare November 23, 2023 19:07
@chatziko chatziko force-pushed the ibeacon-whitelisted-minor branch from 01df260 to 6e43007 Compare November 23, 2023 22:01
@bdraco bdraco changed the title Allow ibeacons with no name but a whitelisted minor Allow ibeacons with no name but a allowlisted minor Nov 24, 2023
@bdraco
Copy link
Member

bdraco commented Nov 24, 2023

Im kinda worried there will be requests for now values. I think it would be better to add the allow list as an option flow so they can add and remove specific ones. It could default to the one used in the android app

@chatziko
Copy link
Contributor Author

Im kinda worried there will be requests for now values. I think it would be better to add the allow list as an option flow so they can add and remove specific ones. It could default to the one used in the android app

If the list is user configurable, wouldn't it make more sense to whitelist UUIDs instead of minor values? It's more precise and more flexible. The point of the minor whitelist was to avoid user configuration.

A cleaner solution if we accept user configuration:

  • have a user-configurable UUID whitelist
  • no hard-coded minor whitelist
  • When we detect that a beacon with the companion app's default minor/major is ignored, show a repair to the user which automatically adds the UUID to the whitelist.

@dshokouhi
Copy link
Member

Hey guys we have just merged home-assistant/android#4027 which sets the default to the new minor value. This will be part of the beta that goes out tonight to the play store.

This means that any user who already had the sensor enabled will need to adjust their values to match whats allowed in this PR.

In order to change the values users can do so in the app settings directly or remotely using a notification command

  • When we detect that a beacon with the companion app's default minor/major is ignored, show a repair to the user which automatically adds the UUID to the whitelist.

out of all the options mentioned I think this will be the most user friendly. Look for the sensor ID ble_transmitter when the state is set to Transmitting and from there you can get the ID to add to the allow list from the sensor attributes. That may be the cleanest solution IMO.

image

@chatziko
Copy link
Contributor Author

@bdraco was suggesting to make the minor-whitelist user configurable (cause users will ask for more options), and my point was that if we add a user-configurable option, it should better be a UUID-whitelist. But this would take some time to implement & test.

Given that the android change is already merged and will be released soon, maybe we can start with the current PR as-is, with no user configurable options. If it's not sufficient and users ask for more options, we can then implement a user-configurable UUID whitelist (including the auto repair feature).

@bdraco
Copy link
Member

bdraco commented Nov 25, 2023

I'd rather do the allow list once instead of having to do a breaking change in a second PR to change the implemention

@chatziko
Copy link
Contributor Author

Makes sense, maybe I can find some time to implement it this weekend (can't say for sure).

@bdraco bdraco marked this pull request as draft November 25, 2023 21:16
@chatziko chatziko mentioned this pull request Nov 30, 2023
20 tasks
@chatziko
Copy link
Contributor Author

I made a new PR (#104790) to keep things clean. I think it turned out pretty neat, note that it's completely transparent for companion users.

@swann05
Copy link

swann05 commented Dec 17, 2023

Hi, may I point that it solves the problem inside home assistant with its own companion app. What about beacons without names that cannot change minor (i supposed it exists) and, in my case, advertising a name with HA app to an outside device (such as espresense or nuki_hub) ?
Is it about the library ? as mentioned by sorien here : #80357 (comment)

Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Feb 15, 2024
@chatziko
Copy link
Contributor Author

Closing this PR, which was abandoned in favor of #104790 (now merged and released).

@chatziko chatziko closed this Feb 15, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2024
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.

iBeacons without names are not seen by the integration
4 participants