Add SensorPush Cloud integration #121890

sstallion · 2024-07-13T06:03:40Z

Proposed change

This PR adds cloud integration for SensorPush devices. It communicates with the publicly available Cloud API using the sensorpush-api Python package. Care was taken to ensure that presented devices appeared the same as those created by the existing SensorPush integration. A G1 WiFi Gateway is required to make use of the Cloud API.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (you're welcome!)
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: Add SensorPush Cloud integration home-assistant.io#33735

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
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io

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.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant

Hi @sstallion

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

home-assistant · 2024-07-13T06:03:44Z

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

Learn more about our pull request process.

Stale

DCSBL

It's been a while since I last left a review, so please take all of this with a grain of salt. I may be incorrect, so don't take it all as absolute truth.

homeassistant/components/sensorpush_cloud/__init__.py

homeassistant/components/sensorpush_cloud/const.py

homeassistant/components/sensorpush_cloud/sensor.py

homeassistant/components/sensorpush_cloud/strings.json

sstallion · 2024-07-18T15:24:18Z

It's been a while since I last left a review, so please take all of this with a grain of salt. I may be incorrect, so don't take it all as absolute truth.

No worries, this is my first integration - I appreciate another set of eyes!

sstallion · 2024-07-21T18:49:08Z

Ping! (Keeping PR from going stale)

joostlek · 2024-07-21T19:22:42Z

homeassistant/components/sensorpush_cloud/api.py

+def api_call[**_P, _R](
+    func: Callable[Concatenate[SensorPushCloudApi, _P], _R],
+) -> Callable[Concatenate[SensorPushCloudApi, _P], _R]:
+    """Decorate API calls to handle SensorPush Cloud exceptions."""
+
+    @wraps(func)
+    def _api_call(self: SensorPushCloudApi, *args: _P.args, **kwargs: _P.kwargs) -> _R:
+        LOGGER.debug(f"API call to {func} with args={args}, kwargs={kwargs}")
+        retries: int = 0
+        while True:
+            try:
+                result = func(self, *args, **kwargs)
+            except Exception as e:
+                # Force reauthorization if an exception occurs to avoid
+                # authorization failures after temporary outages.
+                self.deadline = dt_util.now()
+
+                # The SensorPush Cloud API suffers from frequent exceptions;
+                # requests are retried before raising an error.
+                if retries < REQUEST_RETRIES:
+                    retries = retries + 1
+                    continue
+
+                LOGGER.debug(f"API call to {func} failed after {retries} retries")
+                if isinstance(e, ApiException):
+                    # API exceptions provide a JSON-encoded message in the
+                    # body; otherwise, fall back to the general behavior.
+                    try:
+                        data = json.loads(e.body)
+                        raise SensorPushCloudError(data["message"]) from e
+                    except Exception:  # noqa: BLE001
+                        pass
+                raise SensorPushCloudError from e
+            else:
+                LOGGER.debug(f"API call to {func} succeeded after {retries} retries")
+                return result
+
+    return _api_call


This sounds like something thats api specific, should we keep it in the library?

Unfortunately the API is a generated Swagger client, which doesn't allow the user to customize generated output beyond the package name etc. I'm not the biggest fan of this approach either, but it seemed the cleanest method to interface with the API without leaking details.

Did you generate the openapi?

I always have a huge aversion against those libraries as they always come with deficits and the integration code has to make up for that

I did. I was originally going to write a custom API library for it, but after reading the SensorPush developer documentation, they seemed to favor using Swagger for generating API clients. There wasn't a huge amount of code to integrate the two, so it seemed like a fair trade-off.

Personally I always like the freedom and the efficiency you can add over autogenerating

I agree with Joost, that this code should be part of the library

joostlek · 2024-07-21T19:23:37Z

homeassistant/components/sensorpush_cloud/api.py

+    async def async_authorize(self) -> None:
+        """Sign in and request an authorization code."""
+        return await self.hass.async_add_executor_job(self.authorize)


How can we make everything async from the getgo?

This is a tough one. My preference is for everything to be async as well, however generated Swagger clients have a broad set of compatibility constraints with little support for async. My hope was to at least quarantine blocking calls and then rely on the executor to at least give the appearance of supporting async calls.

joostlek · 2024-07-21T19:28:40Z

homeassistant/components/sensorpush_cloud/config_flow.py

+                    vol.Required(CONF_DEVICE_IDS, default=[]): SelectSelector(
+                        SelectSelectorConfig(options=options, multiple=True)


Why do we select devices and not just add everything?

I'm glad you asked! SensorPush lumps all devices under a single login regardless of locality. For my use case, I have sensors in two different houses, each running their own Home Assistant instance. I need the ability to select specific sensors for each space.

We usually add everything and have the user disable stuff they don't need

Gotcha. Having that part of the config flow does make it easier to filter device data at runtime. I can remove it if you wish, but it will increase the amount of data that is downloaded each update.

To be consistent throughout the Home Assistant, all devices should be added, and the user has the possibility to disable it via the UI.
Please remove the option to select specific devices

joostlek · 2024-07-21T19:29:03Z

homeassistant/components/sensorpush_cloud/config_flow.py

+        if user_input is not None:
+            email, password = user_input[CONF_EMAIL], user_input[CONF_PASSWORD]
+            self.api = SensorPushCloudApi(self.hass, email, password)
+            await self.async_set_unique_id(email, raise_on_progress=False)


Can you change the email?

Nope, this is a fixed part of the SensorPush API used for login and cannot be changed as far as I can tell.

What happens if you need to change your email address? Do you need to create a new account?

I just double checked and it looks like you can change the email using the dashboard. In this case, could it make sense to have a follow-up PR that supports the reauth config flow and allow the user to change both email and password?

joostlek · 2024-07-21T19:30:16Z

homeassistant/components/sensorpush_cloud/coordinator.py

+                data[device_id] = {
+                    ATTR_ALTITUDE: sample.altitude,
+                    ATTR_ATMOSPHERIC_PRESSURE: sample.barometric_pressure,
+                    ATTR_BATTERY_VOLTAGE: sensor["battery_voltage"],
+                    ATTR_DEWPOINT: sample.dewpoint,
+                    ATTR_HUMIDITY: sample.humidity,
+                    ATTR_LAST_UPDATE: sample.observed,
+                    ATTR_SIGNAL_STRENGTH: sensor["rssi"],
+                    ATTR_TEMPERATURE: sample.temperature,
+                    ATTR_VAPOR_PRESSURE: sample.vpd,
+                }


Why dont we just leave the sample in the Coordinator data and have the sensor platform figure out what data it wants to show

We could do that, however it would complicate the sensor code a bit. I was hoping to keep a minimum subset of data resident rather than the entire set of supported devices in addition to the sample data. The SensorPush API isn't well designed and you can see the effects here. Fetching sensor data cannot be filtered, so we're always grabbing every sensor even if they aren't being used by Home Assistant.

homeassistant/components/sensorpush_cloud/sensor.py

sstallion · 2024-07-21T20:33:13Z

Thanks for the review @joostlek! I wanted to check if there was anything preventing this PR from merging. The integration has been working well for some time now and I wanted to double check before I made major changes to the config flow. I also wasn't sure if you wanted me to create a custom API library versus using the existing Swagger client or if that was personal preference. Thanks again!

joostlek · 2024-07-23T10:46:12Z

We probably still have a few rounds to go. But I kinda dislike all the things in the integration that are needed to complete the library. We'd rather have a more complete library. But it's not a blocker as this isn't the only one, but I still wanted to express my opinions on that :)

sstallion · 2024-07-23T15:34:21Z

We probably still have a few rounds to go. But I kinda dislike all the things in the integration that are needed to complete the library. We'd rather have a more complete library. But it's not a blocker as this isn't the only one, but I still wanted to express my opinions on that :)

I understand - I'm not the biggest fan of this approach either. I don't have a good feeling for when/if SensorPush will make changes to the API (the most recent update I can find was from a few weeks ago). Once the API settles I can put together something that would be much nicer to integrate.

With respect to the remaining issues to address, what do you see as blockers to merge the PR?

joostlek · 2024-07-23T21:03:29Z

I did ask around for you. Is it possible to create the openapi as an inner folder in the dependency and just have a public facing facade with nice and neat functions?

/Mylib
         | _api/ (autogen)
         | __init__.py
         | client.py (wrap api)

sstallion · 2024-07-23T21:10:22Z

I did ask around for you. Is it possible to create the openapi as an inner folder in the dependency and just have a public facing facade with nice and neat functions?

Thanks! Not really - Swagger generates an entire setuptools-based package for Python clients. I've also released this to PyPI before opening this PR, which might make it a little tough to back out.

The only other alternative I can think of is to create a new package which depends on the Swagger client and presents the same interface as api.py. The only consumer of this new package would be Home Assistant, which doesn't seem quite right since it would create a cyclic dependency (eg. this library would have to rely on both hass and sensorpush_api to do its job unless we want to use the generic Python executor)

joostlek · 2024-07-23T21:45:53Z

I think there are more ways of generating them, I've also seen them async

joostlek · 2024-07-23T21:47:02Z

Cc @mj23000, you know more about this I think

edenhaus · 2024-07-24T10:11:18Z

homeassistant/components/sensorpush_cloud/api.py

+        while True:
+            try:
+                result = func(self, *args, **kwargs)
+            except Exception as e:


Catching the rare Exception outside the config flow is not allowed. Please adopt the code to catch only specific ones

edenhaus · 2024-07-24T10:11:52Z

homeassistant/components/sensorpush_cloud/api.py

+def api_call[**_P, _R](
+    func: Callable[Concatenate[SensorPushCloudApi, _P], _R],
+) -> Callable[Concatenate[SensorPushCloudApi, _P], _R]:
+    """Decorate API calls to handle SensorPush Cloud exceptions."""
+
+    @wraps(func)
+    def _api_call(self: SensorPushCloudApi, *args: _P.args, **kwargs: _P.kwargs) -> _R:
+        LOGGER.debug(f"API call to {func} with args={args}, kwargs={kwargs}")
+        retries: int = 0
+        while True:
+            try:
+                result = func(self, *args, **kwargs)
+            except Exception as e:
+                # Force reauthorization if an exception occurs to avoid
+                # authorization failures after temporary outages.
+                self.deadline = dt_util.now()
+
+                # The SensorPush Cloud API suffers from frequent exceptions;
+                # requests are retried before raising an error.
+                if retries < REQUEST_RETRIES:
+                    retries = retries + 1
+                    continue
+
+                LOGGER.debug(f"API call to {func} failed after {retries} retries")
+                if isinstance(e, ApiException):
+                    # API exceptions provide a JSON-encoded message in the
+                    # body; otherwise, fall back to the general behavior.
+                    try:
+                        data = json.loads(e.body)
+                        raise SensorPushCloudError(data["message"]) from e
+                    except Exception:  # noqa: BLE001
+                        pass
+                raise SensorPushCloudError from e
+            else:
+                LOGGER.debug(f"API call to {func} succeeded after {retries} retries")
+                return result
+
+    return _api_call


I agree with Joost, that this code should be part of the library

edenhaus · 2024-07-24T10:12:40Z

homeassistant/components/sensorpush_cloud/api.py

+        # Generated Swagger clients install default logging handlers that
+        # conflict with Home Assistant; remove before making API calls.
+        self.configuration = Configuration()
+        for logger in self.configuration.logger.values():
+            logger.removeHandler(self.configuration.logger_stream_handler)


Please fix this directly in the code gen tools. It looks hacky and is a workaround, which should be avoided

edenhaus · 2024-07-24T10:15:21Z

homeassistant/components/sensorpush_cloud/config_flow.py

+                    vol.Required(CONF_DEVICE_IDS, default=[]): SelectSelector(
+                        SelectSelectorConfig(options=options, multiple=True)


To be consistent throughout the Home Assistant, all devices should be added, and the user has the possibility to disable it via the UI.
Please remove the option to select specific devices

edenhaus · 2024-07-24T10:16:49Z

homeassistant/components/sensorpush_cloud/config_flow.py

+        if user_input is not None:
+            email, password = user_input[CONF_EMAIL], user_input[CONF_PASSWORD]
+            self.api = SensorPushCloudApi(self.hass, email, password)
+            await self.async_set_unique_id(email, raise_on_progress=False)


What happens if you need to change your email address? Do you need to create a new account?

edenhaus · 2024-07-24T10:18:23Z

homeassistant/components/sensorpush_cloud/coordinator.py

+                    ATTR_TEMPERATURE: sample.temperature,
+                    ATTR_VAPOR_PRESSURE: sample.vpd,
+                }
+        except Exception:  # noqa: BLE001


Please catch only specific exceptions

edenhaus · 2024-07-24T10:25:11Z

What is the difference between the local and cloud sensor push integration regarding functionality?
What features will be offered only by the local one and which ones only by the cloud one?

mj23000 · 2024-07-24T12:36:35Z

Are you using swagger-codegen for the API generation? We use openapi-generator , which seems to be the more up to date OpenAPI generator.
An async Python client can easily be generated by using the python-pydantic-v1(As Home Assistant still uses pydantic v1 for now) generator and the asyncio backend. Getting everything to work as expected can be a bit of a pain, which results in our generation being pretty complicated as we modify many of the generated files.

joostlek · 2024-07-24T12:40:06Z

But I personally do not allow any more Pydantic V1 integrationd without shims for v2

mj23000 · 2024-07-24T12:52:01Z

But I personally do not allow any more Pydantic V1 integrationd without shims for v2

Yeah exactly, which is part of what makes our generation complicated. It looks like the API client isn't too big to manually add the shims, so you can probably avoid too much work.

joostlek · 2024-07-24T12:54:02Z

Thanks for giving the extra info @mj23000, much appreciated!

sstallion · 2024-07-24T15:21:42Z

What is the difference between the local and cloud sensor push integration regarding functionality? What features will be offered only by the local one and which ones only by the cloud one?

There should be no difference between the two; even the polling intervals are the same. At the moment, it does not appear that the BLE version of the integration provides every possible sensor but that's likely just a limitation in the sensorpush-ble library.

sstallion · 2024-07-24T15:23:19Z

Are you using swagger-codegen for the API generation? We use openapi-generator , which seems to be the more up to date OpenAPI generator. An async Python client can easily be generated by using the python-pydantic-v1(As Home Assistant still uses pydantic v1 for now) generator and the asyncio backend. Getting everything to work as expected can be a bit of a pain, which results in our generation being pretty complicated as we modify many of the generated files.

Gotcha. Is there an exemplar I could use to pattern this integration after?

sstallion · 2024-07-24T15:28:21Z

@joostlek It sounds like the generated sensorpush-api library is a no-go for integration. Since it sounds like there are better generation tools available that library will need an overhaul. With respect to adding this new library, should this be added as an internal package since it will need modification or does this still need to be an external library published to PyPI?

joostlek · 2024-07-24T15:32:09Z

I mean, I think you got the idea now that it should not be just generate and publish and call it a day.

We still require all device specific logic to be its own library, so that's a must.

mj23000 · 2024-07-24T15:46:13Z

Are you using swagger-codegen for the API generation? We use openapi-generator , which seems to be the more up to date OpenAPI generator. An async Python client can easily be generated by using the python-pydantic-v1(As Home Assistant still uses pydantic v1 for now) generator and the asyncio backend. Getting everything to work as expected can be a bit of a pain, which results in our generation being pretty complicated as we modify many of the generated files.

Gotcha. Is there an exemplar I could use to pattern this integration after?

Yes, the mozart-open-api Python client can be used as an example (And is used in the bang_olufsen integration). This client is modified in various ways, so the output of the OpenAPI-generator will be a bit different, but not majorly.

To make Pydantic v1/v2 work, ensure that the Pydantic v1 imports are wrapped in a try catch like this:

try:
    from pydantic.v1 import Field, StrictInt, StrictStr
except ImportError:
    from pydantic import Field, StrictInt, StrictStr

This may unfortunately break some typing checks.

sstallion · 2024-08-01T01:20:52Z

Heads up, I'm still working on this though I'm somewhat slowed by travel for work. I should have something ready for review late next week.

home-assistant bot previously requested changes Jul 13, 2024

View reviewed changes

home-assistant bot added cla-needed core has-tests new-integration integration: sensorpush_cloud labels Jul 13, 2024

home-assistant bot marked this pull request as draft July 13, 2024 06:03

home-assistant bot added cla-recheck cla-signed and removed cla-recheck cla-needed labels Jul 13, 2024

sstallion force-pushed the sstallion/sensorpush_cloud branch 2 times, most recently from 62d93ba to 13e0239 Compare July 13, 2024 16:09

This was referenced Jul 13, 2024

Add SensorPush Cloud integration home-assistant/home-assistant.io#33735

Open

Add SensorPush Cloud integration home-assistant/brands#5667

Open

sstallion marked this pull request as ready for review July 13, 2024 16:58

sstallion force-pushed the sstallion/sensorpush_cloud branch 3 times, most recently from e66eb2f to f8a8e68 Compare July 17, 2024 23:12

DCSBL reviewed Jul 18, 2024

View reviewed changes

sstallion force-pushed the sstallion/sensorpush_cloud branch 3 times, most recently from 5be9bd1 to 7b48ff6 Compare July 18, 2024 20:59

sstallion force-pushed the sstallion/sensorpush_cloud branch from 7b48ff6 to a7470d4 Compare July 21, 2024 18:49

joostlek requested changes Jul 21, 2024

View reviewed changes

home-assistant bot marked this pull request as draft July 21, 2024 19:31

sstallion force-pushed the sstallion/sensorpush_cloud branch from a7470d4 to 8794274 Compare July 21, 2024 19:51

sstallion marked this pull request as ready for review July 21, 2024 19:58

home-assistant bot requested a review from joostlek July 21, 2024 19:58

sstallion force-pushed the sstallion/sensorpush_cloud branch from 8794274 to ecb9b64 Compare July 21, 2024 19:59

Add SensorPush Cloud integration

d8cc277

sstallion force-pushed the sstallion/sensorpush_cloud branch from ecb9b64 to d8cc277 Compare July 22, 2024 02:10

edenhaus requested changes Jul 24, 2024

View reviewed changes

home-assistant bot marked this pull request as draft July 24, 2024 10:20

		vol.Required(CONF_DEVICE_IDS, default=[]): SelectSelector(
		SelectSelectorConfig(options=options, multiple=True)

Add SensorPush Cloud integration #121890

Are you sure you want to change the base?

Add SensorPush Cloud integration #121890

Conversation

sstallion commented Jul 13, 2024 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot left a comment

Choose a reason for hiding this comment

home-assistant bot commented Jul 13, 2024

DCSBL left a comment

Choose a reason for hiding this comment

sstallion commented Jul 18, 2024

sstallion commented Jul 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstallion Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstallion Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstallion Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstallion commented Jul 21, 2024

joostlek commented Jul 23, 2024

sstallion commented Jul 23, 2024 • edited Loading

joostlek commented Jul 23, 2024

sstallion commented Jul 23, 2024 • edited Loading

joostlek commented Jul 23, 2024

joostlek commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edenhaus commented Jul 24, 2024

mj23000 commented Jul 24, 2024

joostlek commented Jul 24, 2024

mj23000 commented Jul 24, 2024 • edited Loading

joostlek commented Jul 24, 2024

sstallion commented Jul 24, 2024

sstallion commented Jul 24, 2024

sstallion commented Jul 24, 2024

joostlek commented Jul 24, 2024

mj23000 commented Jul 24, 2024

sstallion commented Aug 1, 2024

sstallion commented Jul 13, 2024 •

edited

Loading

sstallion Jul 21, 2024 •

edited

Loading

sstallion Jul 21, 2024 •

edited

Loading

sstallion Jul 21, 2024 •

edited

Loading

sstallion commented Jul 23, 2024 •

edited

Loading

sstallion commented Jul 23, 2024 •

edited

Loading

mj23000 commented Jul 24, 2024 •

edited

Loading