core icon indicating copy to clipboard operation
core copied to clipboard

Add config flow to frontier_silicon

Open wlcrs opened this issue 3 years ago β€’ 10 comments

Breaking change

Frontier Silicon integration moves to configuration via Config Flow. Users need to remove their existing integration from configuration.yaml .

Proposed change

Update to FSAPI 0.2.0, which is a large overhaul of the previous implementation. (cfr: https://github.com/wlcrs/python-afsapi/compare/0.0.4...0.2.0 )

It does not longer interfere with the use of the UNDOK application to control the radio.

This PR is the first in a series that will add new functionality to this integration (oa. selecting presets). As per the instructions I'm splitting these out in multiple PR's to ease the process.

Type of change

  • [x] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [ ] New feature (which adds functionality to an existing integration)
  • [x] Breaking change (fix/feature causing existing functionality to break)
  • [x] 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

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] The code has been formatted using Black (black --fast homeassistant tests)
  • [x] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

  • [x] Documentation added/updated for www.home-assistant.io (cfr. https://github.com/home-assistant/home-assistant.io/pull/21238 )

If the code communicates with devices, web services, or third-party tools:

  • [x] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [x] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [x] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [x] Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • [] No score or internal
  • [x] πŸ₯ˆ Silver
  • [ ] πŸ₯‡ Gold
  • [ ] πŸ† Platinum

To help with the load of incoming pull requests:

wlcrs avatar Jan 18 '22 12:01 wlcrs

Tested this with a Philips TAPR802/12 radio and it works without any problems.

TopdRob avatar Feb 09 '22 23:02 TopdRob

I've split the library upgrade from the config_flow implementation into a separate PR: #69371 . I've marked this PR as a draft while waiting for the library upgrade PR to be approved.

wlcrs avatar Apr 05 '22 20:04 wlcrs

@wlcrs #69371 has been merged and shipped, I guess this one needs to be rebased now? Could you take a look? Thanks! πŸ‘

../Frenck

frenck avatar Jun 23 '22 21:06 frenck

This is quite a heavy PR to review as migrating to config_flow means that besides defining async_step_user I need to migrate 2 other flows:

  • discovery via SSDP (async_step_ssdp, async_step_unignore)
  • legacy configuration (async_step_import)

As the device requires a PIN, and the user can change this PIN, I also need to implement async_step_reauth.

To make things a bit more complex: when the device is not discovered via SSDP, I can only retrieve a suitable unique_id to check against after successfully logging in on the device. This means that I've have to implement this check on multiple places.

To make it a bit more digestible, I've made this diagram to show what is going on:

  • how the different steps in the flow call each other
  • where in the flow I am successfully logged in
  • when I check for the unique_id
flowchart TD
    async_step_import --> li3{{Logged In}} --> r3_check([Check radio_id as unique_id]) --> _create_entry
    async_step_user --> _async_step_device_config_if_needed
    async_step_ssdp --> udp_check([Check UDN as unique_id]) --> _async_step_device_config_if_needed
    pin_correct{DEFAULT_PIN is correct?}
    pin_correct-- YES --> li2{{Logged In}} --> r2_check([Check radio_id as unique_id, if no UDN]) --> async_confirm 
    pin_correct-- NO --> async_step_device_config
    _async_step_device_config_if_needed --> pin_correct
    async_step_device_config -- User enters PIN --> li1{{Logged In}} --> r1_check([Check radio_id as unique_id, if no UDN]) --> _create_entry
    async_confirm --> _create_entry

    async_step_unignore --> async_step_ssdp
    async_step_reauth --> async_step_device_config

    style li1 fill:yellowgreen,color:white;
    style li2 fill:yellowgreen,color:white;
    style li3 fill:yellowgreen,color:white;

    style r1_check fill:orange,color:white;
    style r2_check fill:orange,color:white;
    style r3_check fill:orange,color:white;
    style udp_check fill:orange,color:white;
            

wlcrs avatar Jun 29 '22 09:06 wlcrs

To ease the review, you could keep just import and user flows (stash ssdp and reauth for a future PR)

epenet avatar Aug 23 '22 10:08 epenet

As requested by @epenet , I've stashed the ssdp, reauth and unignore flows for now. Still accessible in this branch: https://github.com/wlcrs/core/tree/frontier_silicon_config_flow .

I guess that we'll re-introduce them into this PR once the review for the first two flows is complete? Otherwise we'll introduce a regression into HA.

wlcrs avatar Aug 24 '22 05:08 wlcrs

Otherwise we'll introduce a regression into HA.

I assumed that since the config flow is new these were new features. If it introduces a regression feel free to bring them back.

epenet avatar Aug 24 '22 05:08 epenet

I re-introduced the SSDP flow, as that was previously handled by the deprecated(?) discovery component.

To the future reviewer(s): you can review those changes separately in https://github.com/home-assistant/core/pull/64365/commits/1fa59e2bc4703c96a3189373026d337bed2dcf7a if that's easier for you.

wlcrs avatar Aug 25 '22 06:08 wlcrs

Why is this still pending?

TopdRob avatar Oct 13 '22 06:10 TopdRob

Good question. I've slimmed down the PR as much as I could without introducing regressions, and provided a flowchart to explain how everything interconnects. Nothing more that I can do I'm afraid. A core developer has to step in and process this PR.

I've once again removed a merge conflict that had appeared.

wlcrs avatar Oct 13 '22 10:10 wlcrs

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

Learn more about our pull request process.

home-assistant[bot] avatar Mar 06 '23 11:03 home-assistant[bot]

Hi @epenet , thank you for your feedback on this PR in the last few days. Can you please finalize your review so that we have ample time to add the other config flows in 2023.4 release? I've already used your feedback from this PR to improve the code in the other flows (+their tests), so it should be smooth sailing.

wlcrs avatar Mar 09 '23 08:03 wlcrs

Thanks @wlcrs πŸ‘ And thanks @Kane610 for the second opinionπŸ‘

epenet avatar Mar 10 '23 09:03 epenet