-
-
Notifications
You must be signed in to change notification settings - Fork 656
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 mobile network cell sensor #4380
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common/src/main/java/io/homeassistant/companion/android/common/sensors/NetworkSensorManager.kt
Outdated
Show resolved
Hide resolved
common/src/main/java/io/homeassistant/companion/android/common/sensors/NetworkSensorManager.kt
Outdated
Show resolved
Hide resolved
} | ||
|
||
is CellInfoCdma -> { | ||
"cdma:" + cell.cellIdentity.systemId + ":" + cell.cellIdentity.networkId + ":" + cell.cellIdentity.basestationId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would providing latitude and longitude make more sense for this case? noticed that this particular cellIdentity
provides it
https://developer.android.com/reference/kotlin/android/telephony/CellIdentityCdma
too bad none of the other do :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I missed it. It would make things so simple if all of them supported it ...
I can change it, but I will not be able to test it 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we try and keep the state more predictable so its usable, we can always add things to the attributes that describe the state.
I noticed there seems to be a database of cell tower location that can be used with the cell tower identity, pretty neat!
One more thing this PR will definitely need docs but lets hold off on that until we determine how the sensor will work so we can help teach users how to make sense of the data |
yes absolutely ! I am still trying to figure things out :) |
I have a question about how this sensor should update. backgroundFrom what I understand, there is no intent indicating a cell change. QuestionMy question is what to do : keep things simple and using the worker to update the network cell sensor, or implement the callback and explaining in the documentation what the user should do in their phone settings? I am also worried that the callback method will drain a lot of battery, what do you think ? how power hungry is onSensorUpdate(). Does it causes a network request to the ha server every time it is called or only when the value of a sensor changes ? If I am not clear I will try to re explain better, or ask me questions :) |
hmm dont think it will be user friendly to have the phone settings modification in the docs. Also not sure how feasible the data in the sensor would be if we cant update it often. Surprised there is no intent to handle it but it makes sense.
unless you are pushing an update |
Yes slow updates reduce the usefulness of the sensor :( |
how does automate do it? does it look like they get updates as quickly as one would expect? i dont believe they use a foreground service for this right? Personally i dont think its a good idea to walk users through dialer codes in our documentation. They can really mess stuff up there if they dont know what they are doing. |
Hi ! I also tried to use |
Summary
Fixes: #4368 by adding a mobile network cell sensor.
Screenshots
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#1062
Any other notes
I tried to follow the same convention for cell ids as the "Automate" app https://llamalab.com/automate/doc/block/cell_site_near.html . However I lack knowledge to implement it perfectly :- the UTMS network type could not be implemented because there is no correspondingCellInfo
class- for GSM i did not implement theRNC
value because it is not available in theCellInfoGsm
object (in the "Automate" app example it is also empty)I don't know much in mobile networks, someone more knowledgeable in mobile networks could help to improve this.
Currently the sensor is not instantaneously updated.
I couldn't find a way to get updates with intents. I found a way to do it with a callback (https://developer.android.com/reference/kotlin/android/telephony/TelephonyCallback.CellInfoListener) but I am still figuring out how to implement it.
Therefore, this PR is not yet ready but I would happily receive some feedback regarding what I did and ways to do improve the code.
In the emulator the APIs always return the same values for cell towers regardless of the device settings so I tested my code on two physical phones.
I could only test the following scenarios (because of phone settings limitations, availability of network types in my location and my cell plan) :
Thanks to @dshokouhi for the suggestion to use the
CellIdentity
class.