Adding a discoverable Samsung Syncthru Printer sensor platform by nielstron · Pull Request #13134 · home-assistant/core

nielstron · 2018-03-12T16:40:01Z

Description:

Pull request in netdisco: home-assistant-libs/netdisco#174

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

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

  - platform: syncthru
    resource: http://my.printer.address
    monitored_conditions:
        - toner_black

Checklist:

The code change is tested and works locally.

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

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

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

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

fabaff · 2018-03-12T18:23:55Z

Successor of #12529.

fabaff

Also, nielstron/pysyncthru#1 needs to be addressed.

fabaff · 2018-03-12T18:24:52Z

homeassistant/components/sensor/syncthru.py

+    devices = [SyncThruMain(hass, printer, name)]
+
+    for key in printer.tonerStatus(filter_supported=True):
+        if 'toner_' + str(key) in monitored:


Please use string formatting.

fabaff · 2018-03-12T18:27:41Z

homeassistant/components/sensor/syncthru.py

+        monitored = config.get(CONF_MONITORED_CONDITIONS)
+
+    # Main device, always added
+    printer = SyncThru(host)


You need to catch the TypeError here.

fabaff · 2018-03-12T18:29:16Z

homeassistant/components/sensor/syncthru.py

+
+    def __init__(self, hass, syncthru, name):
+        """Initialize the sensor."""
+        self._hass = hass


fabaff · 2018-03-12T18:29:19Z

homeassistant/components/sensor/syncthru.py

+        self._hass = hass
+        self.syncthru = syncthru
+        self._attributes = {}
+        self._state = STATE_UNKNOWN


Set the state to None and let HA handle unknown states.

fabaff · 2018-03-12T18:37:46Z

homeassistant/components/sensor/syncthru.py

@@ -0,0 +1,244 @@
+"""Connect to a Samsung Printer via SyncThru web interface."""


Please align he style of the file header with the other platforms.

fabaff · 2018-03-12T18:40:41Z

homeassistant/components/sensor/syncthru.py

+            devices.append(SyncThruOutputTray(hass, printer, name, key))
+
+    add_devices(devices, True)
+    return True


Not needed.

fabaff · 2018-03-12T18:42:34Z

homeassistant/components/sensor/syncthru.py

+
+
+class SyncThruMain(SyncThruSensor):
+    """Implementaion of the main sensor, monitoring the general state."""


Typo: Implementaion

MartinHjelmare · 2018-03-13T07:55:51Z

homeassistant/components/sensor/syncthru.py

+        # Test if the discovered device actually is a syncthru printer
+        if not test_syncthru(host):
+            _LOGGER.error("No SyncThru Printer found at %s", host)
+            return False


Just return.

MartinHjelmare · 2018-03-13T07:58:23Z

homeassistant/components/sensor/syncthru.py

+    @property
+    def device_state_attributes(self):
+        """Return the state attributes of the device."""
+        self._attributes[CONF_FRIENDLY_NAME] = self._friendly_name


Remove this.

Friendly name is only for users to set. Platforms can set the name property. If platform wants to have different default friendly name from entity_id, the platform sets the entity_id too.

MartinHjelmare · 2018-03-13T07:58:43Z

homeassistant/components/sensor/syncthru.py

+        self._attributes = {}
+        self._state = STATE_UNKNOWN
+        self._name = name
+        self._friendly_name = name


Remove this.

MartinHjelmare · 2018-03-13T08:02:07Z

homeassistant/components/sensor/syncthru.py

+        self._state = self.syncthru.deviceStatus()
+
+        if self.syncthru.isOnline():
+            self._friendly_name = self.syncthru.model()


See above about friendly name.

MartinHjelmare · 2018-03-13T08:03:42Z

homeassistant/components/sensor/syncthru.py

+            self._attributes = self.syncthru.tonerStatus(
+                ).get(self._color, {})
+            self._state = self._attributes.get(
+                'remaining', STATE_UNKNOWN)


Don't use STATE_UNKNOWN. Use None, which is the default value returned if key is missing in the dict.

houndci-bot · 2018-03-14T14:23:23Z

homeassistant/components/sensor/syncthru.py

too many blank lines (2)

houndci-bot · 2018-03-14T14:23:23Z

homeassistant/components/sensor/syncthru.py

too many blank lines (3)

houndci-bot · 2018-03-14T14:23:23Z

homeassistant/components/sensor/syncthru.py

'homeassistant.const.CONF_FRIENDLY_NAME' imported but unused

MartinHjelmare · 2018-03-15T06:43:11Z

homeassistant/components/sensor/syncthru.py

Please limit this to the exception you expect.

MartinHjelmare · 2018-03-15T06:43:46Z

homeassistant/components/sensor/syncthru.py

Unnecessary return.

MartinHjelmare · 2018-03-15T06:44:16Z

homeassistant/components/sensor/syncthru.py

Don't pass in hass. It will be set on the entity when it has been added to home assistant.

MartinHjelmare · 2018-03-15T06:50:41Z

homeassistant/components/sensor/syncthru.py

Output tray sensor.

MartinHjelmare · 2018-03-15T06:51:35Z

homeassistant/components/sensor/syncthru.py

Drum sensor.

MartinHjelmare · 2018-03-15T06:53:43Z

homeassistant/components/sensor/syncthru.py

Should this be removed? It's called below too if the printer is online.

houndci-bot · 2018-03-15T12:08:04Z

homeassistant/components/sensor/syncthru.py

unexpected indentation

houndci-bot · 2018-03-15T12:08:04Z

homeassistant/components/sensor/syncthru.py

expected 2 blank lines after class or function definition, found 0
missing whitespace around operator
missing whitespace after ':'

houndci-bot · 2018-03-15T12:08:04Z

homeassistant/components/sensor/syncthru.py

unexpected indentation

houndci-bot · 2018-03-15T12:08:04Z

homeassistant/components/sensor/syncthru.py

expected 2 blank lines after class or function definition, found 0
missing whitespace around operator

houndci-bot · 2018-03-15T12:08:04Z

homeassistant/components/sensor/syncthru.py

unexpected indentation

houndci-bot · 2018-03-15T12:08:04Z

homeassistant/components/sensor/syncthru.py

expected 2 blank lines after class or function definition, found 0
IndentationError: unexpected unindent
missing whitespace around bitwise or shift operator
missing whitespace around operator

houndci-bot · 2018-03-15T12:08:05Z

homeassistant/components/sensor/syncthru.py

IndentationError: unindent does not match any outer indentation level

pvizeli · 2018-03-17T08:50:30Z

Please rebase this

Adds a component based on pyblnet, that hooks up the blnet to home assistant

Now the friendly_name (and for digital values the mode) is set in the state_attributes whereas the name is defined as "blnet_(analog|digital)_{sensornumber}" so you can reliably add them to groups.

MartinHjelmare · 2018-03-17T17:56:30Z

@nielstron is this ready to merge?

nielstron · 2018-03-17T23:12:35Z

Yes this is ready for merging on my opinion. The home-assistant.io PR has no logo but I doubt it will be allowed by Samsung to use their logo. could also be added in a later request.

MartinHjelmare · 2018-03-18T00:06:27Z

.coveragerc

    homeassistant/components/sensor/pushbullet.py
    homeassistant/components/sensor/pvoutput.py
    homeassistant/components/sensor/pyload.py
+    homeassistant/components/sensor/syncthru.py


Please sort 🔤.

MartinHjelmare

Thanks!

MartinHjelmare · 2018-03-18T10:32:26Z

Can be merged when build passes.

Diaoul · 2018-04-09T22:04:43Z

I have added my printer with this component using the hostname however the discovery adds a second device with the ip address. Should I fill an issue for that or is this inherent to the discovery stuff?

nielstron requested a review from andrey-git as a code owner March 12, 2018 16:40

homeassistant added integration: discovery new-platform platform: sensor.syncthru cla-signed labels Mar 12, 2018

nielstron changed the title ~~Adding a discoverable Samsung Syncthru Printer component~~ Adding a discoverable Samsung Syncthru Printer sensor platform Mar 12, 2018

fabaff reviewed Mar 12, 2018

View reviewed changes

MartinHjelmare requested changes Mar 13, 2018

View reviewed changes

homeassistant added the cla-signed label Mar 14, 2018

houndci-bot reviewed Mar 14, 2018

View reviewed changes

MartinHjelmare requested changes Mar 15, 2018

View reviewed changes

nielstron requested review from a team, Danielhiversen and cdce8p as code owners March 15, 2018 12:07

homeassistant added the cla-signed label Mar 15, 2018

houndci-bot reviewed Mar 15, 2018

View reviewed changes

cdce8p removed their request for review March 15, 2018 12:09

nielstron force-pushed the pysyncthru branch from bf5996e to 2f92d0f Compare March 15, 2018 12:11

homeassistant added the cla-signed label Mar 15, 2018

pvizeli approved these changes Mar 17, 2018

View reviewed changes

MartinHjelmare approved these changes Mar 17, 2018

View reviewed changes

nielstron added 7 commits March 17, 2018 13:59

Added a simple component to support the BLNET

8569121

Adds a component based on pyblnet, that hooks up the blnet to home assistant

Adds support for custimzation of blnet sensor devices

f5adcd9

Setting up blnet as a platfrom

d376c45

Updated use of state_attributes

e73807b

Now the friendly_name (and for digital values the mode) is set in the state_attributes whereas the name is defined as "blnet_(analog|digital)_{sensornumber}" so you can reliably add them to groups.

Added support for the SyncThru printer web service

cd6e443

Added pysyncthru to the requirements

8a34303

Changed to Dependencis, import inside setup_platform

4102cf6

nielstron added 11 commits March 17, 2018 14:01

Added pysyncthru to the requirements

d9ed437

Removed Blnet components

9203510

Pinned requirements and added to coveragerc

fbbc6e5

Fixed indentation

f08c411

Fixed format

4f2663a

Pinned pysyncthru in requirements_all, having trouble with friendly_name

f5053e3

ran gen_reqirements_all.py

acfcce6

Updated requirements_all

a4ba8c2

Renamed sensor objects, removed passing of hass entity

7fe451a

Removed merge artifacts

3b251c2

Reset syncthru to newest state

b892338

nielstron force-pushed the pysyncthru branch from b670471 to b892338 Compare March 17, 2018 13:08

homeassistant added the cla-signed label Mar 17, 2018

nielstron added 3 commits March 17, 2018 14:11

Merge branch 'dev' into pysyncthru

72898a0

Updated requirements_all

d940746

switched to using the newest version of pysyncthru

d0291ab

MartinHjelmare reviewed Mar 18, 2018

View reviewed changes

Sorted coveragerc

00cb12d

homeassistant added the cla-signed label Mar 18, 2018

MartinHjelmare approved these changes Mar 18, 2018

View reviewed changes

balloob merged commit 6b05948 into home-assistant:dev Mar 18, 2018

nielstron mentioned this pull request Mar 19, 2018

Syncthru Printer description home-assistant/home-assistant.io#4707

Merged

2 tasks

balloob mentioned this pull request Mar 30, 2018

0.66.0 #13554

Merged

home-assistant locked and limited conversation to collaborators Jul 26, 2018

ghost added integration: syncthru and removed platform: sensor.syncthru labels Mar 21, 2019

		@@ -0,0 +1,244 @@
		"""Connect to a Samsung Printer via SyncThru web interface."""



		class SyncThruMain(SyncThruSensor):
		"""Implementaion of the main sensor, monitoring the general state."""

Uh oh!

Conversation

nielstron commented Mar 12, 2018

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

fabaff commented Mar 12, 2018

Uh oh!

fabaff left a comment

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!

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!

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!

MartinHjelmare Mar 15, 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!

pvizeli commented Mar 17, 2018

Uh oh!

MartinHjelmare commented Mar 17, 2018

Uh oh!

nielstron commented Mar 17, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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

MartinHjelmare Mar 15, 2018 •

edited

Loading

nielstron commented Mar 17, 2018 via email •

edited

Loading