Add min_temp and max_temp to MQTT climate device by PhilRW · Pull Request #14690 · home-assistant/core

PhilRW · 2018-05-30T00:35:12Z

Description:

min_temp and max_temp now work with the MQTT climate device.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5455

Example entry for `configuration.yaml` (if applicable):

climate:
  - platform: mqtt
    name: Study
    min_temp: 40
    max_temp: 90
    current_temperature_topic: /sensors/hvac_study/current_temp
    temperature_command_topic: /sensors/hvac_study/target_temp

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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

Documentation added/updated in home-assistant.github.io

houndci-bot · 2018-05-30T00:36:01Z

+
+        state = self.hass.states.get(ENTITY_CLIMATE)
+        self.assertEqual(35, state.attributes.get('max_temp'))
+


blank line at end of file

dgomes

I understand your clever use of the super class, but maybe we can improve the super class in this PR defining 7 and 35 as constants and using these constants as the defaults for config.get()

PhilRW · 2018-05-30T16:51:09Z

I wish I could say it was my cleverness, but I was simply doing some copypasta from the generic thermostat class. Any suggestions are welcome.

PhilRW · 2018-05-30T16:52:48Z

Perhaps let's open a separate ticket/PR and fix the superclass issue (and refactor subclasses) in that?

PhilRW · 2018-05-31T16:41:43Z

@dgomes Perhaps something like this?

dgomes · 2018-05-31T16:53:36Z

Would replace the super().min_temp with the imported DEFAULT_MIN_HUMITIDY.

This would make things more explicit

PhilRW · 2018-05-31T17:03:59Z

Cool; I'll take a crack at it once this one gets merged.

dgomes · 2018-05-31T17:11:08Z

I see things the other way round.

The other PR should come first, then you rebase this one according to the changes, then this gets approved.

PhilRW · 2018-05-31T17:30:26Z

OK, see #14721

OttoWinter · 2018-05-31T21:10:31Z

+        """Return the minimum temperature."""
+        # pylint: disable=no-member
+        if self._min_temp:
+            return self._min_temp


So if the parent entity is converting the temperature to the internal temp_unit, I think we should do so here too, right?

yeah, good point. missed that logic

We should probably also fix whatever bugs were introduced in #14721 since I didn't consider that.

Already went ahead in #14730

This is really confusing thanks to the lack of a consistent use of units cross platforms :(

home-assistant/architecture#10

OttoWinter · 2018-05-31T21:15:00Z

    vol.Optional(CONF_PAYLOAD_ON, default="ON"): cv.string,
    vol.Optional(CONF_PAYLOAD_OFF, default="OFF"): cv.string,
+
+    vol.Optional(CONF_MIN_TEMP): vol.Coerce(float),


So I think an even cleaner solution would be to write

vol.Optional(CONF_MIN_TEMP, default=DEFAULT_MIN_TEMP): vol.Coerce(float),

The later, you could just write:

@property def min_temp(self): """Return the minimum temperature.""" return self._min_temp

The voluptuous schema is where usually all default values should reside in (when there are config schemas). Having the default values in code usually leads to bugs one way or another.

Banged my head against this too.

DEFAULT_MIN_TEMP is in CELCIUS, CONF_MIN_TEMP might not be... using the default=DEFAULT_MIN_TEMP leaves an innuendo on what is the unit of the temperature voluptuous returns :(

Then again if I revert it to super().min_temp it will do the conversion according to convert_temperature() and return that value. Or implement that logic in each subclass?

OttoWinter · 2018-05-31T21:18:19Z

+        assert setup_component(self.hass, climate.DOMAIN, DEFAULT_CONFIG)
+
+        state = self.hass.states.get(ENTITY_CLIMATE)
+        self.assertEqual(7, state.attributes.get('min_temp'))


So these test cases are not really testing much. The tests on lines 56-57 already do test the defaults. It would, however be good that the configuration schema would be tested with something like

assert setup_component(self.hass, climate.DOMAIN, { # ... 'min_temp': 10.0, }) state = self.hass.states.get(ENTITY_CLIMATE) self.assertEqual(10.0, state.attributes.get('min_temp'))

houndci-bot · 2018-06-01T15:38:08Z

+    def max_temp(self):
+        """Return the maximum temperature."""
+        # pylint: disable=no-member
+        return self._max_temp


no newline at end of file

houndci-bot · 2018-06-01T15:38:09Z

 import homeassistant.helpers.config_validation as cv
 from homeassistant.components.fan import (SPEED_LOW, SPEED_MEDIUM,
                                          SPEED_HIGH)
+from homeassistant.util.temperature import convert as convert_temperature


'homeassistant.util.temperature.convert as convert_temperature' imported but unused

houndci-bot · 2018-06-01T15:38:09Z

+        max_temp = state.attributes.get('max_temp')
+
+        self.assertIsInstance(max_temp, float)
+        self.assertEqual(60, max_temp)


no newline at end of file

houndci-bot · 2018-06-01T15:38:09Z

+    def max_temp(self):
+        """Return the maximum temperature."""
+        # pylint: disable=no-member
+        return self._max_temp


no newline at end of file

houndci-bot · 2018-06-01T15:38:09Z

 import homeassistant.helpers.config_validation as cv
 from homeassistant.components.fan import (SPEED_LOW, SPEED_MEDIUM,
                                          SPEED_HIGH)
+from homeassistant.util.temperature import convert as convert_temperature


'homeassistant.util.temperature.convert as convert_temperature' imported but unused

houndci-bot · 2018-06-01T15:38:09Z

+        max_temp = state.attributes.get('max_temp')
+
+        self.assertIsInstance(max_temp, float)
+        self.assertEqual(60, max_temp)


no newline at end of file

* Add min_temp and max_temp to MQTT climate device * Add unit tests * Remove blank line * Fix unit tests & temp return values * PEP-8 fixes * Remove unused import

homeassistant added platform: climate.mqtt cla-signed labels May 30, 2018

houndci-bot reviewed May 30, 2018

View reviewed changes

PhilRW mentioned this pull request May 30, 2018

Update documentation for MQTT climate device home-assistant/home-assistant.io#5455

Merged

2 tasks

dgomes approved these changes May 30, 2018

View reviewed changes

OttoWinter reviewed May 31, 2018

View reviewed changes

dgomes mentioned this pull request May 31, 2018

Fix unit conversion #14730

Merged

8 tasks

PhilRW requested review from a team, andrey-git and cdce8p as code owners June 1, 2018 15:37

homeassistant added the cla-signed label Jun 1, 2018

Philip Rosenberg-Watt added 4 commits June 1, 2018 09:37

Add min_temp and max_temp to MQTT climate device

a2ab953

Add unit tests

a549726

Remove blank line

98ca199

Fix unit tests & temp return values

a03e617

houndci-bot reviewed Jun 1, 2018

View reviewed changes

PhilRW force-pushed the climate-mqtt branch from a1132e3 to a03e617 Compare June 1, 2018 15:38

homeassistant added the cla-signed label Jun 1, 2018

Philip Rosenberg-Watt added 2 commits June 1, 2018 09:38

PEP-8 fixes

810383c

Remove unused import

0663a65

cdce8p removed their request for review June 1, 2018 19:31

dgomes added the Ready for review label Jun 7, 2018

balloob approved these changes Jun 7, 2018

View reviewed changes

balloob merged commit bb4d177 into home-assistant:dev Jun 7, 2018

ghost removed the Ready for review label Jun 7, 2018

balloob mentioned this pull request Jun 22, 2018

0.72 #15088

Merged

home-assistant locked and limited conversation to collaborators Sep 5, 2018

ghost added integration: mqtt and removed platform: climate.mqtt labels Mar 21, 2019


		state = self.hass.states.get(ENTITY_CLIMATE)
		self.assertEqual(35, state.attributes.get('max_temp'))

Uh oh!

Conversation

PhilRW commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgomes left a comment

Choose a reason for hiding this comment

Uh oh!

PhilRW commented May 30, 2018

Uh oh!

PhilRW commented May 30, 2018

Uh oh!

PhilRW commented May 31, 2018

Uh oh!

dgomes commented May 31, 2018

Uh oh!

PhilRW commented May 31, 2018

Uh oh!

dgomes commented May 31, 2018

Uh oh!

PhilRW commented May 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgomes May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

PhilRW commented May 30, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

dgomes May 31, 2018 •

edited

Loading