Skip to content

Create ISY auxiliary sensors as sensor entities instead of attributes#71254

Merged
balloob merged 1 commit intohome-assistant:devfrom
bdraco:isy_sensors
May 3, 2022
Merged

Create ISY auxiliary sensors as sensor entities instead of attributes#71254
balloob merged 1 commit intohome-assistant:devfrom
bdraco:isy_sensors

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented May 3, 2022

Breaking change

The auxiliary sensors for each insteon device are now
their own sensor entity instead of an attribute on
the parent entity which makes them easier to find and allows
attributes to be de-duplicated in the database.

Screen Shot 2022-05-03 at 11 42 59

General purpose, custom control, and redundant sensors are not enabled
by default. If they are desired, they can be enabled in the UI

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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
  • 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.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

The auxiliary sensors for each insteon device are now
their own sensor entity instead of an attribute on
the parent entity.
@probot-home-assistant
Copy link
Copy Markdown

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

@balloob balloob merged commit 8d40d9d into home-assistant:dev May 3, 2022
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented May 3, 2022

Thanks.

@shbatm
Copy link
Copy Markdown
Contributor

shbatm commented May 3, 2022

@bdraco Would it be better to mark RR (Ramp Rate) and OL (On Level) as Config Sensors? And ST can be ignored--that will be your main device state.

@shbatm
Copy link
Copy Markdown
Contributor

shbatm commented May 3, 2022

Also--your ignore method looks like it will only ignore GV, but for node servers and Z-Wave these are usually followed by a number e.g. GV1.

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented May 3, 2022

Would it be better to mark RR (Ramp Rate) and OL (On Level) as Config Sensors?

I think this would be a good addition. However, I we should still disable them by default most users probably won't need them.

And ST can be ignored--that will be your main device state.

I don't think this one is actually ever exposed, at least it isn't on my devices.

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented May 3, 2022

Also--your ignore method looks like it will only ignore GV, but for node servers and Z-Wave these are usually followed by a number e.g. GV1.

The code uses control.startswith so it should get ignored

@shbatm
Copy link
Copy Markdown
Contributor

shbatm commented May 3, 2022

The code uses control.startswith so it should get ignored

Ah, missed that. I thought I read it as includes.

Last thing -- how does this handle the attributes that are populated on a state change, that don't exist from initial query. For example ERR will only be in the aux_properties if the device has an error during communications.

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented May 3, 2022

Last thing -- how does this handle the attributes that are populated on a state change, that don't exist from initial query. For example ERR will only be in the aux_properties if the device has an error during communications.

It is not going to create them. I could see that one still being useful as EntityCategory.DIAGNOSTIC. I can add a special case to still create it. Are there any others ?

@shbatm
Copy link
Copy Markdown
Contributor

shbatm commented May 3, 2022

ERR and BUSY are probably the two that would come in without ever being picked up from /nodes or /status that we should worry about and maybe create by default as Diagnostic? Error comes in as integer (# of errors), pretty sure BUSY comes in a 0 or 1 and could be binary.

I have no idea about Node Servers--I don't know if they can change their definitions mid-stream or not. Users should reload the integration after any node/scene/node server changes anyways.

Thermostats thow properties in late, but I think those are picked up in the /status call we added in v3; and climate.py reads the nodes aux properties separately from the main entity, so worst case is you don't get separate sensors from your set points, but they're available under the climate entity.

Also, a nit-pick, but would it be better to reference the constants from the module rather than redefine in sensor.py?

And last (for outside of this PR): you reminded me that we should add a "Beep" button for devices down the road.

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented May 3, 2022

I think ERR makes sense to add. BUSY is likely not so useful, but I'll add it if you think someone will have a use case for it.

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented May 3, 2022

I'll add the constants for the ones we have. There are a few missing

@github-actions github-actions bot locked and limited conversation to collaborators May 4, 2022
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.

4 participants