core icon indicating copy to clipboard operation
core copied to clipboard

Add a new "Ambient Weather Network" integration

Open thomaskistler opened this issue 2 years ago • 7 comments

Breaking change

Proposed change

This pull request establishes support for a new integration with the Ambient Weather Network. Similar to the current Ambient Station integration, this integration gathers sensor data from individual weather stations. However, in contrast to the Ambient Station integration, which enables only owners to fetch data solely from their owned stations, the new integration directly pulls data from https://ambientwether.net/ without requiring an API key, albeit with a reduced dataset (e.g., excluding indoor sensors). Furthermore, this integration enables the creation of a virtual weather station that combines data from various physical weather stations.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [x] 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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/30347

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] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [x] 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:

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

To help with the load of incoming pull requests:

thomaskistler avatar Dec 15 '23 04:12 thomaskistler

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

Code owner commands

Code owners of ambient_station can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign ambient_station Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Dec 15 '23 04:12 home-assistant[bot]

Putting this to draft as CI isn't passing. I don't really understand why this shouldn't extend the already existing integration instead of making a completely new one?

gjohansson-ST avatar Dec 22 '23 16:12 gjohansson-ST

I had a conversation with @bachya (owner of ambient-station) and we concluded that this probably needs to be a separate integration. For one, the onboarding process is completely different. ambient-station requires you to own a station and you onboard with your personal API key. It reports data that is only relevant to you (e.g., indoor humidity and indoor temperature). ambient-network is like Weather Underground. You pick one or a set of stations near you and it reports aggregated data from those stations. It only reports non-personal data (e.g., no indoor sensors).

We can of course combine them, but then the onboarding process would look something like this:

  1. Do you currently own a personal station (yes, no)
  2. If yes, enter your API key and move to step 4
  3. If no: a. pick the area you live in b. pick the stations you want to monitor
  4. Pick an HA name for your station

In addition, they pull data from two different APIs. ambient-station uses a web socket to fetch data and ambient-network uses a coordinator to pull data from an restful API.

thomaskistler avatar Dec 22 '23 16:12 thomaskistler

(Converastion that @thomaskistler mentions is here.)

I agree. Different onboarding, different transport, different data, etc. all lead me to think that including everything in one integration is unwieldy. I went through something similar with airvisual and airvisual_pro: started out as a single integration, became a mess of different logic, and so I put them as separate integrations underneath the same "brand."

bachya avatar Dec 22 '23 16:12 bachya

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 Dec 30 '23 23:12 home-assistant[bot]

I'll have another look after my workout. Feel free to send me a message on discord

joostlek avatar Jan 27 '24 07:01 joostlek

@joostlek and @gjohansson-ST, I left some clarifying questions for you.

thomaskistler avatar Feb 06 '24 15:02 thomaskistler

Please rebase this PR to bring all the latest dev changes in

gjohansson-ST avatar Mar 23 '24 17:03 gjohansson-ST

Rebased and addressed all the feedback.

thomaskistler avatar Mar 23 '24 18:03 thomaskistler

@thomaskistler I think we need a brand for Ambient Weather, which should point to the ambient station and ambient network integrations.

emontnemery avatar Mar 25 '24 11:03 emontnemery

According to the PR description:

this integration enables the creation of a virtual weather station that combines data from various physical weather stations.

Where does that happen in the code? The config flow requires the user to pick a single weather station as I understand it?

emontnemery avatar Mar 25 '24 11:03 emontnemery

I will change the PR description. It used to be supported but previous reviewers didn't like that capability so it was removed.

thomaskistler avatar Mar 25 '24 14:03 thomaskistler