Add climate platform to Mazda integration #75037

bdr99 · 2022-07-12T02:29:09Z

Proposed change

Add a climate entity to the Mazda integration that allows controlling the vehicle's HVAC system. Only works for electric vehicles.

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

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Add documentation for Mazda climate platform home-assistant.io#23365

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:

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

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

homeassistant/components/mazda/climate.py

bdraco · 2022-08-28T21:46:51Z

homeassistant/components/mazda/climate.py

+    client = hass.data[DOMAIN][config_entry.entry_id][DATA_CLIENT]
+    coordinator = hass.data[DOMAIN][config_entry.entry_id][DATA_COORDINATOR]
+    region = hass.data[DOMAIN][config_entry.entry_id][DATA_REGION]


I'd probably unpack this first to reduce the number of dict lookups python has to do.

Ideally we store the data as a dataclass instead of dict so it has better typing

domain_data = hass.data[DOMAIN][config_entry.entry_id]
client = domain_data[DATA_CLIENT]

Thanks, I have updated the code to unpack the dict first.

I agree with your dataclass suggestion. I'm planning to open a general cleanup PR after this PR is merged, and I'll make sure to include that change. By the way, do you know of any other integrations that use a dataclass in this way, so I can use them as an example?

lookin has an example

Thanks. I'll refer to that when I work on the follow-up PR.

homeassistant/components/mazda/climate.py

bdr99 · 2022-09-26T19:31:42Z

Hi @OnFreund, I've addressed your comments above. Would you mind taking another look please? Thanks!

OnFreund

Left a few minor comments, but looks good to me

OnFreund · 2022-09-27T06:12:31Z

homeassistant/components/mazda/climate.py

+    @callback
+    def _handle_coordinator_update(self) -> None:
+        """Update attributes when the coordinator data updates."""
+        self._update_state_attributes()


Can this be pushed to MazdaEntity? That way each platform only needs to implement _update_state_attributes() and not have to worry about calling super().

I like this idea, but since it would affect several platforms, I would like to do this in a separate follow-up PR after this one is merged.

homeassistant/components/mazda/climate.py

bdraco · 2022-12-27T19:23:24Z

homeassistant/components/mazda/climate.py

+
+    async def async_set_temperature(self, **kwargs: Any) -> None:
+        """Set a new target temperature."""
+        if (temperature := kwargs.get(ATTR_TEMPERATURE)) is not None:


This would be cleaner if you reversed the condition and returned early so you could outdent below

I actually did it this way because I couldn't find a way to achieve 100% test coverage otherwise. It doesn't seem possible to create a test scenario where kwargs.get(ATTR_TEMPERATURE) is None, but I need to check that case in order for the type checker to pass.

We can make a service call for set_temperature that passes low and high target temps instead of ATTR_TEMPERATURE.

bdraco · 2022-12-27T19:24:57Z

homeassistant/components/mazda/climate.py

+    entry_data = hass.data[DOMAIN][config_entry.entry_id]
+    client = entry_data[DATA_CLIENT]
+    coordinator = entry_data[DATA_COORDINATOR]
+    region = entry_data[DATA_REGION]


This would be a bit cleaner if entry_data was a @dataclass

I agree. We already discussed this previously here: #75037 (comment). I plan to implement this in a follow-up PR after this one is merged.

bdraco · 2022-12-27T19:35:26Z

@bdr99 Can you take care of the unit enum?

bdr99 · 2022-12-27T19:56:55Z

@bdraco Thanks for approving. The unit enums are fixed.

bdr99 · 2022-12-27T19:57:37Z

There seems to be an issue where the CI checks are failing globally across all PRs right now.

bdraco · 2022-12-27T19:58:41Z

Probably wait an hour for the rate limit to clear than

git commit -m 'empty' -n --allow-empty

and push

bdr99 · 2022-12-27T20:13:27Z

@bdraco The CI checks passed now. Good idea with the empty commit, I didn't know about --allow-empty.

MartinHjelmare · 2022-12-28T21:25:20Z

homeassistant/components/mazda/climate.py

+        if self.data["hvacSetting"]["temperatureUnit"] == "F":
+            self._attr_precision = PRECISION_WHOLE
+            self._attr_temperature_unit = UnitOfTemperature.FAHRENHEIT
+            self._attr_min_temp = 61.0
+            self._attr_max_temp = 83.0
+        else:
+            self._attr_precision = PRECISION_HALVES
+            self._attr_temperature_unit = UnitOfTemperature.CELSIUS


Why do we need these two cases if the API always reports the temperature data in Celsius? The climate integration will convert between C and F for all the temperature attributes based on the climate entity temperature_unit attribute and the user globally configured temperature unit.

The idea is that the platform shouldn't need to bother with converting.

MartinHjelmare · 2022-12-28T21:41:03Z

homeassistant/components/mazda/climate.py

+        elif hvac_mode == HVACMode.OFF:
+            await self.client.turn_off_hvac(self.vehicle_id)
+
+        self._handle_coordinator_update()


It's a bit weird to call this method when there's no coordinator update. I'd create a different helper that we can call that calls self._update_state_attributes and self.async_write_ha_state.

Add climate platform to Mazda integration

8bb3706

homeassistant added the cla-signed label Jul 12, 2022

project-bot bot added this to Needs review in Dev Jul 12, 2022

probot-home-assistant bot added has-tests integration: mazda new-feature new-platform by-code-owner labels Jul 12, 2022

project-bot bot moved this from Needs review to By Code Owner in Dev Jul 12, 2022

bdr99 mentioned this pull request Jul 12, 2022

Add documentation for Mazda climate platform home-assistant/home-assistant.io#23365

Merged

8 tasks

frenck added the smash Indicator this PR is close to finish for merging or closing label Jul 25, 2022

OnFreund reviewed Aug 11, 2022

View reviewed changes

homeassistant/components/mazda/climate.py Outdated Show resolved Hide resolved

homeassistant/components/mazda/climate.py Outdated Show resolved Hide resolved

homeassistant/components/mazda/climate.py Outdated Show resolved Hide resolved

bdraco reviewed Aug 28, 2022

View reviewed changes

bdr99 added 2 commits August 30, 2022 19:28

Refactor to store state values in entity

8100781

Unpack entry data in async_setup_entry

c699f8f

bdr99 requested a review from OnFreund September 4, 2022 01:48

bdr99 mentioned this pull request Sep 4, 2022

mazda odometer #76421

Closed

OnFreund suggested changes Sep 16, 2022

View reviewed changes

Dev automation moved this from By Code Owner to Review in progress Sep 16, 2022

bdr99 added 5 commits September 16, 2022 18:53

Convert properties to attributes

1e3799d

Implement

f3c26f8

Import constants from climate root

8b09d2c

Initialize state attributes in constructor

376d73d

Fix docstring

2628b5e

bdr99 requested a review from OnFreund September 26, 2022 19:31

OnFreund approved these changes Sep 27, 2022

View reviewed changes

Extract some common logic

d81e9e4

OnFreund reviewed Sep 27, 2022

View reviewed changes

homeassistant/components/mazda/climate.py Show resolved Hide resolved

bdr99 requested a review from bdraco November 8, 2022 21:40

bdraco reviewed Dec 27, 2022

View reviewed changes

bdraco approved these changes Dec 27, 2022

View reviewed changes

Dev automation moved this from Review in progress to Reviewer approved Dec 27, 2022

Merge branch 'dev' into mazda_climate_platform

8b335a4

bdr99 and others added 4 commits December 27, 2022 14:43

Fix pylint issues

7f4cc96

Format with black

cfde695

Small change to re-run CI

ca8ca9b

Merge branch 'dev' into mazda_climate_platform

3b915b7

empty

9dbf9d9

bdraco merged commit f5c5615 into home-assistant:dev Dec 27, 2022

Dev automation moved this from Reviewer approved to Done Dec 27, 2022

github-actions bot locked and limited conversation to collaborators Dec 28, 2022

MartinHjelmare reviewed Dec 28, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add climate platform to Mazda integration #75037

Add climate platform to Mazda integration #75037

bdr99 commented Jul 12, 2022

bdraco Aug 28, 2022

bdr99 Aug 30, 2022

bdraco Aug 30, 2022

bdr99 Aug 30, 2022

bdr99 commented Sep 26, 2022

OnFreund left a comment

OnFreund Sep 27, 2022

bdr99 Sep 27, 2022

bdraco Dec 27, 2022

bdr99 Dec 27, 2022 •

edited

MartinHjelmare Dec 28, 2022 •

edited

bdraco Dec 27, 2022

bdr99 Dec 27, 2022 •

edited

bdraco commented Dec 27, 2022

bdr99 commented Dec 27, 2022

bdr99 commented Dec 27, 2022

bdraco commented Dec 27, 2022

bdr99 commented Dec 27, 2022

MartinHjelmare Dec 28, 2022

MartinHjelmare Dec 28, 2022

Add climate platform to Mazda integration #75037

Add climate platform to Mazda integration #75037

Conversation

bdr99 commented Jul 12, 2022

Proposed change

Type of change

Additional information

Checklist

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

bdr99 commented Sep 26, 2022

OnFreund left a 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

bdr99 Dec 27, 2022 • edited

Choose a reason for hiding this comment

MartinHjelmare Dec 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdr99 Dec 27, 2022 • edited

Choose a reason for hiding this comment

bdraco commented Dec 27, 2022

bdr99 commented Dec 27, 2022

bdr99 commented Dec 27, 2022

bdraco commented Dec 27, 2022

bdr99 commented Dec 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdr99 Dec 27, 2022 •

edited

MartinHjelmare Dec 28, 2022 •

edited

bdr99 Dec 27, 2022 •

edited