-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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 Starlink state sensors #133184
base: dev
Are you sure you want to change the base?
Add Starlink state sensors #133184
Conversation
Hey there @boswelja, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
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.
What about an icon for the status?
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
b42b4b2
to
644b654
Compare
Added icon 'mdi:satellite-uplink' to the state sensor, because it looks like a Starlink dish :) |
5fa3c4c
to
8a65b5f
Compare
8a65b5f
to
ff62f3c
Compare
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.
Change
key="connected"
to
key="connectivity"
too. 👍
Edit: Name connected
w/ a state connected
is just off.
ff62f3c
to
c26b259
Compare
Random idea, but should I change the 'obstructed' binary sensor to be a diagnostic, like the other problem sensors, now that there's a catch-all 'connected' one (which would include obstructions)? |
Good point, I’m for it. |
StarlinkBinarySensorEntityDescription( | ||
key="connection", | ||
device_class=BinarySensorDeviceClass.CONNECTIVITY, | ||
value_fn=lambda data: (data.status["state"] == "CONNECTED"), |
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.
value_fn=lambda data: (data.status["state"] == "CONNECTED"), | |
value_fn=lambda data: data.status["state"] == "CONNECTED", |
What are the other states? Would it make sense to have it be an enum sensor instead?
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.
This sensor is specifically binary because the raw state can change with a Starlink software update, so no enum sensor in this PR - see this thread above for more.
Proposed change
Add detailed state sensors for Starlink.
The current integration has no way of representing Starlink being disconnected from the Internet for reasons other than obstruction. For example, if a network outage occurs, there is no sensor to indicate that it is not working.
This PR proposes to add the State (sensor.starlink_state) and Connected (binary_sensor.starlink_connected) sensors back. 'State' was originally present in the original HACS Starlink integration, and the 'Connected' sensor is a binary representation of the former.
This is my first 'real' PR, so feedback is very welcome.
Type of change
Additional information
Checklist
ruff format homeassistant tests
)(not sure how or if needed in this case?)
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
.To help with the load of incoming pull requests: