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

Add option to transmit BLE on Home SSID only #4277

Merged
merged 5 commits into from Mar 22, 2024

Conversation

leccelecce
Copy link
Contributor

Summary

Sensors -> Bluetooth Sensors -> BLE Transmitter

Added a toggle called "Transmit on Home SSID only". When enabled, beacon transmission will only occur if the device is connected to a Home Network WiFi SSID.

Closes #2867

Screenshots

n/a

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#1046

Any other notes

This is my first commit to an Android app, please be gentle!
I have only been able to test this on an AVD as I can't get USB or Wifi debugging working on my Samsung S23 (I believe because it has corporate apps installed). However the AVD allowed me to toggle Bluetooth and Wifi and looked to behave correctly.

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @leccelecce

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!

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@leccelecce
Copy link
Contributor Author

I've now managed to test this on my Samsung S23 Ultra by installing the debug APK created and can confirm that it's working for me. This includes also testing that switching from one "home" SSID to a "non-home" SSID disables transmission, as well as turning WiFi off entirely.

@dshokouhi
Copy link
Member

@leccelecce have you tested this against multiple servers?

@leccelecce
Copy link
Contributor Author

No, I don't have that setup at home currently. I've not looked into it before but can do so if required?

My reading of server Manager.getServer() is that it always returns the currently active server. Is your point that someone could change the currently active server, and that would need to retrigger the home network check?

@dshokouhi
Copy link
Member

Is your point that someone could change the currently active server, and that would need to retrigger the home network check?

No the point is that a user may have multiple servers setup with different BSSID and SSID configured so we need to test that it properly turns on for all servers. Sensor settings impact all servers the user can have added. BSSID and SSID is configured on a per server basis so by looking at just the currently active server ID is not correct as a user may not have their server active and the check will skip it.

@leccelecce
Copy link
Contributor Author

So I'm still trying to get my head around the multiple server concept. Say a user has:

Server A: Home 1 (using SSID: Wifi X)
Server B: Home 2 (using SSID: Wifi Y)

Are you saying that even if Server A is active, we should accept Wifi Y at Home 2 as valid for transmitting the beacon?

I had assumed that if you connected to Wifi Y, the app would automatically make Server B the active server, but presumably not?

@jpelgrom
Copy link
Member

jpelgrom commented Mar 18, 2024

The whole 'active' concept only exists for what you're viewing in the app - the dashboard. Everywhere else (sensors, widgets, ...) all servers are supposed to be 'active' and stay updated.

Are you saying that even if Server A is active, we should accept Wifi Y at Home 2 as valid for transmitting the beacon?

Reading @dshokouhi's comment and in my opinion, yes. If any server is seeing home WiFi, it should be enabled. Sensor values are not linked to an individual server.

@leccelecce
Copy link
Contributor Author

OK, looked around the codebase and updated as follows which hopefully addresses this

private fun isPermittedOnThisWifiNetwork(context: Context): Boolean {
        return serverManager(context).defaultServers.any { it.connection.isHomeWifiSsid() }
    }

@leccelecce
Copy link
Contributor Author

Oh my god, the more I look at Kotlin the more I love it - just discovered the expression body syntax. Just going to simplify this a little more...there

@dshokouhi
Copy link
Member

We should also add a line about this setting in the sensor description as we mention all other settings there.

Also not sure but should we make the setting title match the setting the user defines the SSIDs under to make it more apparent?

@leccelecce
Copy link
Contributor Author

Done and done

Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with 2 servers each connected to their own network and it respects the setting, even connecting to a 3rd non-home network leaves the transmitter disabled.

Thank you very much for the contribution :)

@leccelecce
Copy link
Contributor Author

My pleasure, the code changes have turned out to be very simple but needed some more understanding, so thank you for the help in getting to the right solution.

I can see the last Companion App release was 2024.1.5 on Jan 30th; I can't find any information on the release cadence but looks like it is approximately every 2 months? Are the "pre-release" releases on Github all to the beta channel?

@dshokouhi
Copy link
Member

I can see the last Companion App release was 2024.1.5 on Jan 30th; I can't find any information on the release cadence but looks like it is approximately every 2 months? Are the "pre-release" releases on Github all to the beta channel?

We currently do not have a set schedule for releasing to production. Every saturday all merged changes get pushed to the beta channel in the play store, after that is when we decide to cut a release. We usually like to let features and fixes get tested by beta testers before pushing changes to production.

@leccelecce
Copy link
Contributor Author

Noted, many thanks! Will enroll in the beta programme next week :-)

@dshokouhi dshokouhi merged commit b23b183 into home-assistant:master Mar 22, 2024
4 checks passed
@leccelecce leccelecce deleted the ble-transmit-wifi-only branch March 22, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BLE transmitter: auto enable/disable depending on home wifi SSID
3 participants