Skip to content
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

Wake on Lan #199

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

codetheweb
Copy link

Related to #4.

I believe the information in the above issue may be outdated, or my TV may just be different (2020 series 6). My situation:

  • There is no 'eco mode' but some called 'fast start' instead, which is effectively the same? When fast start is enabled, the TV is always accessible over the network. When disabled, the TV disconnects from the network after 15 minutes.
  • However, when in fast start mode it constantly pulls 30 W which would be nice to avoid.
  • The TV does not advertise on the /device-info endpoint that WoL is supported.
  • But empirically WoL does work when the TV is not in fast start mode.

So the TL;DR is that adding WoL support helps lower my energy bill while still allowing HomeKit to turn on my TV. 😛

It's a fairly naive implementation right now but seems to work well. One caveat is that scenes which change the input and turn on the TV at the same time don't work if the TV is offline; but that doesn't seem like a dealbreaker.

I started adding tests but then realized that many were skipped. Is that something you want me to still do?

@bschlenk
Copy link
Owner

bschlenk commented Sep 9, 2020

The skipped tests are very odd... I have a distinct memory of getting test coverage up to 100%, I must not have pushed it. If you were more than halfway done with tests, then I'd appreciate them, otherwise I can look for the branch where I was adding tests and I can write tests for this change myself.

@codetheweb
Copy link
Author

Nah, I was just getting started.
Let me know if you want any help writing them though.

src/homebridge-roku.js Outdated Show resolved Hide resolved
src/homebridge-roku.js Outdated Show resolved Hide resolved
@codetheweb
Copy link
Author

codetheweb commented Sep 9, 2020

Looking at #9, it looks like there is both an ethernetMac and a wifiMac, do you know if those are usually the same value? Since we're checking for support for wlan, which I assume is wireless lan, we should probably be using the wifiMac here. Or am I missing something?

My ethernetMac and wifiMac are different, and that's probably true for other users as well since (I'm guessing) it's different hardware for each.

WoL isn't possible over WiFi (AFAIK), probably should add a note to the README about that. Edit: not true WoL but I think Roku's implementation is to always stay connected to WiFi and then turn on the rest of the system when a WoL packet is received.

@codetheweb
Copy link
Author

Since we're checking for support for wlan, which I assume is wireless lan, we should probably be using the wifiMac here.

Oh, I see what you're saying. My TV says supportsWakeOnWlan == false; but WoL on ethernet does work. Does that mean WoL on ethernet works for all devices and wireless WoL only on some? 🤷‍♂️

Not sure how that should be codified without knowing more about WoL support.

@bschlenk
Copy link
Owner

bschlenk commented Sep 9, 2020

Maybe it would be better to fire off a wol.wake(info.ethernetMac) regardless, and then also fire off wol.wake(info.wifiMac) if info.supportsWakeOnWlan is true. I don't think there's any harm in firing off the magic packet if the device can't actually receive it. Although then you would have to run another info() check to see if the wol worked in order to give back the right status. I was hoping maybe there'd be a info.isConnectedToEthernet field, but I don't see one on mine.

@codetheweb
Copy link
Author

I think firing off packets to both addresses makes sense.

But what do you mean about having to call info() again?

@bschlenk
Copy link
Owner

Ah sorry I misremembered how things worked. I was thinking that the setters needed to return the new value but maybe not.

I'm just not sure what happens if you say power on, homebridge thinks the tv was successfully powered on, but in reality the tv never received the WOL packet.

Before this change, if it couldn't reach the tv, then the homebridge callback would be called with and error. Now it's possible to call the callback with success, when the command didn't actually work.

.catch(callback);
.on('get', async (callback) => {
try {
const info = await pTimeout(this.roku.info(), this.requestTimeout);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, will this ever work as expected? If the TV is off, this will fail immediately and never trigger the TimeoutError check below. I don't think we even need to use pTimeout at all. We can always try sending the WOL packet if this initially fails. Then you'd have to retry this.roku.info() to see if it actually turned on (the retry would need to be done after some timeout to give it time to turn on), then if it still isn't on, you call callback with the error, otherwise with null.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, will this ever work as expected?

It's been working for me so far 😛.

If the TV is off, this will fail immediately and never trigger the TimeoutError check below.

Nope, this.roku.info() doesn't resolve until around 10 seconds later if the TV is offline; node-fetch which is used by the Roku package sorta has timeout support but I don't think the Roku package implements it.

@codetheweb
Copy link
Author

I'm just not sure what happens if you say power on, homebridge thinks the tv was successfully powered on, but in reality the tv never received the WOL packet.

Good point; maybe we could add something that runs 5-10 seconds after the WoL packet is sent to see if the TV is now on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants