Add kwargs to send_magic_packet() service individually #37387

stshontikidis · 2020-07-02T22:37:31Z

Port kwarg was always being passed even if the config returned None
which breaks in the 3rd party lib. You probably don't ever want to send
just a port but the library supports it so both are optional and not
dependent on one another.

Breaking change

Proposed change

Port kwarg was always being passed even if the config returned None
which breaks in the 3rd party lib. You probably don't ever want to send
just a port but the library supports it so both are optional and not
dependent on one another.

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)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

# Example configuration.yaml
wake_on_lan:

Additional information

This PR fixes or closes issue: fixes service wake_on_lan.send_magic_packet breaks (for some scenarios) #37107
This PR is related to issue:
Link to documentation pull request:

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.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

Port kwarg was always being passed even if the config returned None which breaks in the 3rd party lib. You probably don't ever want to send just a port but the library supports it so both are optional and not dependent on one another.

homeassistant · 2020-07-02T22:37:33Z

Hi @stshontikidis,

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!

stshontikidis · 2020-07-03T18:24:33Z

Was able to fully test locally after working through a FE issue unrelated to this change.

Logs on branch with fix

2020-07-03 14:14:50 INFO (MainThread) [homeassistant.components.wake_on_lan] Send magic packet to mac aa:bb:cc:dd:ee:ff (broadcast: 192.168.1.254, port: None)
2020-07-03 14:15:04 INFO (MainThread) [homeassistant.components.wake_on_lan] Send magic packet to mac aa:bb:cc:dd:ee:ff (broadcast: None, port: 9)

Current dev without fix. Note that sending just the port logs ok but I am very certain it does not actually send the service call with the port if you look at how the logging is being setup, it always logs with the kwargs but it is hitting the service call with only the positional args since it only checks for a broadcast address.

2020-07-03 14:21:01 INFO (MainThread) [homeassistant.components.wake_on_lan] Send magic packet to mac aa:bb:cc:dd:ee:ff (broadcast: None, port: 9)
2020-07-03 14:21:58 INFO (MainThread) [homeassistant.components.wake_on_lan] Send magic packet to mac aa:bb:cc:dd:ee:ff (broadcast: None, port: 2)
2020-07-03 14:22:16 INFO (MainThread) [homeassistant.components.wake_on_lan] Send magic packet to mac aa:bb:cc:dd:ee:ff (broadcast: 192.168.1.254, port: None)
2020-07-03 14:22:16 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection.4523651408] an integer is required (got type NoneType)
Traceback (most recent call last):
  File "/Users/savatshontikidis/sandbox/home-assistant/core/homeassistant/components/websocket_api/commands.py", line 130, in handle_call_service
    connection.context(msg),
  File "/Users/savatshontikidis/sandbox/home-assistant/core/homeassistant/core.py", line 1269, in async_call
    task.result()
  File "/Users/savatshontikidis/sandbox/home-assistant/core/homeassistant/core.py", line 1304, in _execute_service
    await handler.func(service_call)
  File "/Users/savatshontikidis/sandbox/home-assistant/core/homeassistant/components/wake_on_lan/__init__.py", line 46, in send_magic_packet
    port=broadcast_port,
  File "/usr/local/Cellar/python/3.7.7/Frameworks/Python.framework/Versions/3.7/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/savatshontikidis/sandbox/home-assistant/core/venv/lib/python3.7/site-packages/wakeonlan.py", line 79, in send_magic_packet
    sock.connect((ip, port))
TypeError: an integer is required (got type NoneType)

balloob

Looks good!

lollo78 · 2020-07-12T16:14:47Z

Wol stopped working for me after 0.112 upgrade and 0.112.4 doesn't fix the issue.

stshontikidis · 2020-07-13T14:09:48Z

@lollo78 how are you using WOL? Can you provide any config and logs? I am seeing no issue with using broadcast address with no broadcast port after the 112.4 update.

lollo78 · 2020-07-13T20:55:40Z

@stshontikidis
Hi, I haven't changed my config. Before the update WOL was working fine for Samsung TV, PC and Synology NAS.
Now is "broken".

In configuration.yaml:

# WOL Support
wake_on_lan:

#Samsung TV
samsungtv:
  - host: !secret samsungtv_host
    turn_on_action:
      - service: wake_on_lan.send_magic_packet
        data:
          mac: !secret samsungtv_mac

In includes/switches.yaml

#NAS WOL
  - platform: wake_on_lan
    mac: !secret nas_mac
    name: "Synology NAS"
    host: !secret nas_host
    turn_off:
      service: shell_command.turn_off_nas

In packages/pc_stanzetta.yaml:

switch:
#PC STANZETTA WOL

  - platform: wake_on_lan
    mac: !secret pc_stanzetta_mac
    name: "PC Stanzetta WOL"
    host: !secret pc_stanzetta_host
    turn_off:
      service: hassio.addon_stdin
      data:
        addon: core_rpc_shutdown
        input: turn_off_pc_stanzetta

I see this in log, but I don't know if is related:

Logger: homeassistant.components.switch
Source: __main__.py:356
Integration: Interruttore (documentation, issues)
First occurred: 12:09:15 (3 occurrences)
Last logged: 12:09:18

Setup of switch platform wake_on_lan is taking over 10 seconds.
Setup of switch platform broadlink is taking over 10 seconds.
Setup of switch platform mqtt is taking over 10 seconds.

stshontikidis · 2020-07-13T21:24:47Z

@lollo78
I am actually surprised you are seeing issues, the issue this PR solves is when you were using wake_on_lan with broadcast_address configured but no broadcast_port. From your config it does not look like you ever define a broadcast_address. Do you have the logger configured? if so you can call the service logger.set_level from the developer tools/services tab and pass

homeassistant.components.wake_on_lan: debug
homeassistant.components.websocket_api: info

then fire off the wake on lan switches that are not working and let us know the output. logs should be in config/home-assistant.log

balloob · 2020-07-14T00:42:11Z

Let's not discuss this on a merged PR. Please open a new issue.

homeassistant added the cla-needed label Jul 2, 2020

probot-home-assistant bot added bugfix has-tests integration: wake_on_lan small-pr PRs with less than 30 lines. labels Jul 2, 2020

project-bot bot added this to Needs review in Dev Jul 2, 2020

homeassistant added cla-signed and removed cla-needed labels Jul 2, 2020

This was referenced Jul 8, 2020

Update wake_on_lan parameter #37406

Closed

WOL stopped working on 0.112.3 #37623

Closed

stshontikidis added 3 commits July 8, 2020 12:54

Merge branch 'dev' into wakeonlan-port-fix

9a5b58b

Update switch integration as well to pass kwargs

69c8cc3

Update test for WOL switch

6ab67d6

Dev automation moved this from Needs review to Reviewer approved Jul 8, 2020

balloob approved these changes Jul 8, 2020

View reviewed changes

balloob added this to the 0.112.4 milestone Jul 8, 2020

balloob merged commit c1de781 into home-assistant:dev Jul 8, 2020

Dev automation moved this from Reviewer approved to Done Jul 8, 2020

stshontikidis deleted the wakeonlan-port-fix branch July 8, 2020 21:41

balloob added the cherry-picked label Jul 9, 2020

balloob pushed a commit that referenced this pull request Jul 9, 2020

Add kwargs to send_magic_packet() service individually (#37387)

af87168

balloob mentioned this pull request Jul 9, 2020

0.112.4 #37668

Merged

home-assistant locked as resolved and limited conversation to collaborators Jul 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kwargs to send_magic_packet() service individually #37387

Add kwargs to send_magic_packet() service individually #37387

stshontikidis commented Jul 2, 2020 •

edited

homeassistant commented Jul 2, 2020

stshontikidis commented Jul 3, 2020 •

edited

balloob left a comment

lollo78 commented Jul 12, 2020

stshontikidis commented Jul 13, 2020

lollo78 commented Jul 13, 2020 •

edited

stshontikidis commented Jul 13, 2020

balloob commented Jul 14, 2020

Add kwargs to send_magic_packet() service individually #37387

Add kwargs to send_magic_packet() service individually #37387

Conversation

stshontikidis commented Jul 2, 2020 • edited

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

homeassistant commented Jul 2, 2020

stshontikidis commented Jul 3, 2020 • edited

balloob left a comment

Choose a reason for hiding this comment

lollo78 commented Jul 12, 2020

stshontikidis commented Jul 13, 2020

lollo78 commented Jul 13, 2020 • edited

stshontikidis commented Jul 13, 2020

balloob commented Jul 14, 2020

stshontikidis commented Jul 2, 2020 •

edited

Example entry for `configuration.yaml`:

stshontikidis commented Jul 3, 2020 •

edited

lollo78 commented Jul 13, 2020 •

edited