New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thermoworks Smoke Sensor #16139

Merged
merged 20 commits into from Oct 8, 2018

Conversation

Projects
None yet
6 participants
@nhorvath
Contributor

nhorvath commented Aug 22, 2018

Description:

Provides access to thermoworks smoke thermometer data.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: thermoworks_smoke
    email: "your email here"
    password: !secret thermoworks_pass
    monitored_conditions:
      - probe1
      - probe2
      - probe1_min
      - probe1_max
      - probe2_min
      - probe2_max

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:

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@wafflebot wafflebot bot added the in progress label Aug 22, 2018

nhorvath added a commit to nhorvath/home-assistant.github.io that referenced this pull request Aug 22, 2018

@nhorvath nhorvath referenced this pull request Aug 22, 2018

Merged

Create documentation for Thermoworks Smoke sensor. #6063

2 of 2 tasks complete
@fabaff

Just some quick comments...

monitored_variables = config.get(CONF_MONITORED_VARIABLES)
excluded = config.get(CONF_EXCLUDE)
mgr = thermoworks_smoke.initialize_app(email, password, True, excluded)

This comment has been minimized.

@fabaff

fabaff Aug 22, 2018

Member

Platform setup should fail if the credentials are not valid and a log entry created.

This comment has been minimized.

@nhorvath

nhorvath Aug 23, 2018

Contributor

I need to handle that in the library. Will update soon.

Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py
def __init__(self, sensor_type, serial, mgr):
"""Initialize the sensor."""
self._name = mgr.name(serial) + ' ' + SENSOR_TYPES[sensor_type]

This comment has been minimized.

@fabaff

fabaff Aug 22, 2018

Member

Please use string formatting.

This comment has been minimized.

@nhorvath

nhorvath Aug 22, 2018

Contributor

I'm sorry but python is not my best language. What's the proper way to do that?

This comment has been minimized.

@nhorvath

nhorvath Aug 23, 2018

Contributor

I figured it out.

Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py
def __init__(self, sensor_type, serial, mgr):
"""Initialize the sensor."""
self._name = mgr.name(serial) + ' ' + SENSOR_TYPES[sensor_type]

This comment has been minimized.

@fabaff

fabaff Aug 22, 2018

Member

Please use string formatting.

This comment has been minimized.

@nhorvath

nhorvath Aug 23, 2018

Contributor

fixed

Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py
Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py Outdated
values = self.mgr.data(self.serial)
# set state from data based on type of sensor
self._state = values.get(self.type, None)

This comment has been minimized.

@fabaff

fabaff Aug 22, 2018

Member

None is the default if .get doesn't get the data.

This comment has been minimized.

@nhorvath

nhorvath Aug 22, 2018

Contributor

will remove.

Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py
Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py
def __init__(self, sensor_type, serial, mgr):
"""Initialize the sensor."""
self._name = "{name} {sensor}".format(name=mgr.name(serial), sensor=SENSOR_TYPES[sensor_type])

This comment has been minimized.

@houndci-bot

houndci-bot Aug 23, 2018

line too long (102 > 79 characters)

This comment has been minimized.

@nhorvath

nhorvath Aug 23, 2018

Contributor

fixed

nhorvath added some commits Aug 23, 2018

Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py
add_devices(dev, True)
except HTTPError as error:
if 'EMAIL_NOT_FOUND' in error.strerror or "INVALID_PASSWORD" in error.strerror:

This comment has been minimized.

@houndci-bot

houndci-bot Aug 23, 2018

line too long (87 > 79 characters)

for serial in mgr.serials():
for variable in monitored_variables:
dev.append(ThermoworksSmokeSensor(variable, serial, mgr))

This comment has been minimized.

@houndci-bot

houndci-bot Aug 23, 2018

blank line contains whitespace

# list of sensor devices
dev = []

This comment has been minimized.

@houndci-bot

houndci-bot Aug 23, 2018

blank line contains whitespace

try:
mgr = thermoworks_smoke.initialize_app(email, password, True, excluded)

This comment has been minimized.

@houndci-bot

houndci-bot Aug 23, 2018

blank line contains whitespace

nhorvath added some commits Aug 23, 2018

Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py Outdated
import thermoworks_smoke
from requests.exceptions import HTTPError
email = config.get(CONF_EMAIL)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 24, 2018

Member

Use dict[key] for required keys and keys with default values.

})
def setup_platform(hass, config, add_devices, discovery_info=None):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 24, 2018

Member

add_devices has been renamed to add_entities. Please update here too.

Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py
Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py
Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py Outdated
@nhorvath

This comment has been minimized.

Contributor

nhorvath commented Aug 24, 2018

@MartinHjelmare and @fabaff when you get a chance can you look at my questions above?

@nhorvath

This comment has been minimized.

Contributor

nhorvath commented Aug 31, 2018

@fabaff where are we at with this? Is it ok in its current state?

@nhorvath

This comment has been minimized.

Contributor

nhorvath commented Sep 27, 2018

@fabaff can you please re-review?

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Sep 27, 2018

There remains requests for changes, regarding sensor type formatting, config validation and state attributes.

Then make sure that the build passes.

@nhorvath

This comment has been minimized.

Contributor

nhorvath commented Sep 27, 2018

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Sep 27, 2018

By using the dictionary we can map whatever home assistant needs to whatever the integration needs. In home assistant we want to have lowercase snakecase sensor type strings that are also used in configuration yaml to select what sensor types to set up.

nhorvath added some commits Sep 29, 2018

exclude firmware from state attrs. rename original_unit to unit_of_mi…
…n_max so it's more clear what it is for.

nhorvath added some commits Sep 30, 2018

# strip probe label and convert to snake_case
key = snakecase(key.replace(self.type, ''))
if key == FIRMWARE:
self.firmware_version = val

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

This isn't used, and we shouldn't use it. Please remove it.

This comment has been minimized.

@nhorvath

nhorvath Sep 30, 2018

Contributor

Yes it is, it populates min and max values as well as wifi strength and battery level.

Show resolved Hide resolved homeassistant/components/sensor/thermoworks_smoke.py Outdated
@MartinHjelmare

Looks good to me. @fabaff can you take a last look?

@nhorvath

This comment has been minimized.

Contributor

nhorvath commented Oct 8, 2018

Is this good to merge? I'd like to get this into a release so I don't have to keep updating the documentation.

@MartinHjelmare MartinHjelmare merged commit 1393766 into home-assistant:dev Oct 8, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 93.517%
Details

@wafflebot wafflebot bot removed the in progress label Oct 8, 2018

frenck added a commit to home-assistant/home-assistant.io that referenced this pull request Oct 9, 2018

Create documentation for Thermoworks Smoke sensor. (#6063)
* Create documentation for Thermoworks Smoke sensor.

Documentation to correspond with home-assistant/home-assistant#16139

* fix configuration and raw tags

* add thermoworks logo

* version number

* Update logo for thermoworks.

* changes from code reviews

* requested changes

* Update with requested changes
@hunterjm

This comment has been minimized.

Contributor

hunterjm commented Oct 19, 2018

@MartinHjelmare @nhorvath - You may want to revert this. Nest Thermostats are broken in the dev branch because pyrebase (a dependency of thermoworks_smoke) has it's own sseclient built in that overwrites files in sseclient-py when installed (namespace collision).

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 19, 2018

@hunterjm please open an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment