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

Revert "Add dynamic generation of device triggers from keypad buttons #80797" #82605

Closed
wants to merge 3 commits into from

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Nov 24, 2022

Proposed change

I tried to figure out a way to rework this but it since #80797 requires the integration to be setup to attach the trigger its not going to work if the integration isn't setup in time or is in a failed/retry state.

I think we are going to have to rework that design to rely only what is in the device registry when attaching triggers.

I hate to do a revert of #80797 but since it causes pico remotes to no longer function if the bridge isn't available right away at startup or ends up in retry at startup as reported in #82495 & #81999 I don't think we have another option.

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
  • 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 @swails, @danaues, mind taking a look at this pull request as it has been labeled with an integration (lutron_caseta) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of lutron_caseta can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign lutron_caseta Removes the current integration label and assignees on the issue, add the integration domain after the command.

@bdraco
Copy link
Member Author

bdraco commented Nov 24, 2022

@danaues Sorry about doing a revert. Let me know if you have see another way.

@danaues
Copy link
Contributor

danaues commented Nov 25, 2022

@bdraco,

Is there an async call that we can await , so it happens right when the integration is set up?

    _async_register_bridge_device(hass, entry_id, bridge_device, bridge)
    keypad_data = _async_setup_keypads(hass, entry_id, bridge, bridge_device)

@danaues
Copy link
Contributor

danaues commented Nov 25, 2022

Have you tested with the revert to confirm that it fixes the issue? I don't see anything that should make this slower that the original. Not waiting on any extra calls from the bridge, just looping through some dicts.

@bdraco
Copy link
Member Author

bdraco commented Nov 25, 2022

@bdraco,

Is there an async call that we can await , so it happens right when the integration is set up?


    _async_register_bridge_device(hass, entry_id, bridge_device, bridge)

    keypad_data = _async_setup_keypads(hass, entry_id, bridge, bridge_device)

That would work unless the integration fails to setup in which case it would deadlock setting up automations. We could add a timeout but that gets us back to the same failure state where the integration isn't setup yet.

@bdraco
Copy link
Member Author

bdraco commented Nov 25, 2022

Have you tested with the revert to confirm that it fixes the issue?

Yes. I added an explicit test as well to make sure it can still attach triggers if the integration is slow to setup, failed, or retrying by unloading the integration and making sure the triggers still attach

I don't see anything that should make this slower that the original. Not waiting on any extra calls from the bridge, just looping through some dicts.

The original didn't require the integration to be setup to attach the triggers which is the core issue here. We can't be sure it will be completed setup at attach time

@bdraco
Copy link
Member Author

bdraco commented Nov 25, 2022

I was digging into the device triggers to see if we can do some type of late attach but I haven't finished going through it yet because of the holiday

@bdraco
Copy link
Member Author

bdraco commented Nov 25, 2022

The validation can handle the config entry not being loaded yet

async def async_validate_trigger_config(

But attaching the trigger can't because thats the point where the automation is being setup

async def async_attach_trigger(

"parent_keypad": "9",
},
"1372": {
"button_name": "Kitchen " "Pendants",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit of a problem since if you rename the button the trigger breaks

@elupus
Copy link
Contributor

elupus commented Nov 26, 2022

We do late attach in the new PluggableActions

@bdraco
Copy link
Member Author

bdraco commented Nov 26, 2022

We do late attach in the new PluggableActions

This one is getting replaced with #82714

@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants