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 Access (woa) - 'Just in time' WOL when accessing MySQL or FileShare Server #1892

Merged
merged 1 commit into from
May 10, 2013

Conversation

t4-ravenbird
Copy link
Contributor

Forum thread : http://forum.xbmc.org/showthread.php?tid=124340

Replacement for abandoned pull request #1112

Refactored solution based on work by @pieh including simple GUI-setup as discussed in old PR.

Working for Windows (tested by @kricker) and Linux (tested by @bilbonvidia) - but needs verification for other platforms.

@Memphiz
Copy link
Member

Memphiz commented Dec 6, 2012

I'm not able to do a pull request to your repo (no clue why - but github doesn't show it as a target repo for pull requests).

For getting this to work on osx and ios add this patch (git am )

http://pastebin.com/Y4PcyF9N

Seems your repo is a bit messy in general. You shouldn't work in the master branch normally.

@t4-ravenbird
Copy link
Contributor Author

OK, I grabbed the "../network/osx/ioshacks.h" file in your patch, but this is a new and unreferenced file so how will it matter? Was your patch incomplete?

@Memphiz
Copy link
Member

Memphiz commented Dec 7, 2012

Ohh yeah - sorry. Somehow i mixed up my patches.

You need this one too of course:

Memphiz@0985c90#diff-3

@t4-ravenbird
Copy link
Contributor Author

Thanks a lot ! (Hope I got it right)
I assume you built it .. did you test out anything also?

@Memphiz
Copy link
Member

Memphiz commented Dec 7, 2012

I only tested if it recognized/discovered the macs of my sources/mysql server when i enabled the feature in power management. If i got it right this is the only thing which is platform dependend - everything else should work on all platforms right? I'm not able to test wakeup because my server is a arm-based dockstar which doesn't sleep ;)

@Memphiz
Copy link
Member

Memphiz commented Dec 7, 2012

Yep you got the commit right - though we need to redo the xcode sync when this PR is merged most likly - but thats no problem ...

@t4-ravenbird
Copy link
Contributor Author

There is also a new ping-function in the network class but I assume it must have compiled as is then .. It just does system("ping ..") and if we are lucky it will work.
The rest should be platform independent, but it does not work until it is verified;-)

@da-anda
Copy link
Member

da-anda commented Jan 17, 2013

just gave this a try. Working nice in general, but I have some issues with it:

  • I don't want my NAS to be woken unless I want to play a movies, but something seems to wake it always (maybe a script from the skin that's checking for a file)
  • I got several KaiToasts that the device is already awake - do we really need those?
  • I'm not sure if it was related to this feature enabled (will test again), but starting playback of a video with device already awake took very long
  • My NAS needs a long time to do a cold boot and the timeout the WOL action was waiting for it was to short. IIRC there is some advanced setting to change it, but IMO it should be possible to change it in GUI as WOL is really nothing "advanced" anymore these days.

@t4-ravenbird
Copy link
Contributor Author

  • The patch is supposed to do just that (wake server only when accessed). Log should show why it kicked in, but you need to enable Debug-logging. (I used LOGINFO, maybe LOGNOTICE should be used instead so it comes up without debug-logging)
  • Maybe just drop them, yes. (They only come if your 'timeout' corresponds badly with your server's actual idle-time before it enters suspend, so if you tune 'timeout' in wakeOnLan.xml it would help)
  • WakeOnLan.xml ...

@da-anda
Copy link
Member

da-anda commented Jan 18, 2013

@t4-ravenbird

  • in my case it also wakes the NAS when checking for thumbnails/fanart it seems although they already are cached (need to check log with a new build)
  • I noticed that this patch queues WOL messages even if the device is already running. Why is that needed? To prevent it from going to sleep again? Shouldn't active file access prevent this?

@t4-ravenbird
Copy link
Contributor Author

I dont understand what you mean by queuing wol messages? Only scenario where more than 1 wol gets transmitted would be if more than 1 thread accesses server at 'same time' - there is no mutex around the wakeup since worst thing that can happen due to multithreading would be just that ; more than 1 wol is transmitted in a short time-window.
After a wakeup is performed there will be no more attempts to wake until 'timeout' expires, and then only if host-up-check (ping) fails.

The patch will by design NOT prevent server from going to sleep. (If keeping server alive was acceptable behavior then this patch would not be required at all as I see it - keep-alive wols could be done by plug-ins already)

@@ -204,6 +204,7 @@ class CApplication : public CXBApplicationEx, public IPlayerCallback, public IMs

virtual void Process();
void ProcessSlow();
void ProcessSlowEnable(bool enable);

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@t4-ravenbird
Copy link
Contributor Author

update;

  • removed 'server already running' notification
  • changed log-messages from level 'info' to 'notice'
  • additional protection to avoid recursive progress-dialog (as I have experienced on my openelec build, still collecting experience on my latest build)

@da-anda
Copy link
Member

da-anda commented Feb 8, 2013

is this one ready for prime time now? If so, please rebase.
XBMC devs please check and give green light so it can go in this merge window.

@Memphiz
Copy link
Member

Memphiz commented Feb 8, 2013

-1 from me for this merge window - i'm not around until monday for this. If @davilla wants to test it for osx/ios then of course this can go in if he made it till close of the merge window. (it needs at least a resync of xcode projects)

@kricker
Copy link
Member

kricker commented Feb 8, 2013

There is still one thing that bothers me with this patch. I've discussed it in the past with t4-ravenbird and he wasn't sure how to "fix" it. For some reason it will wake server when you are accessing Add-ons. I don't think it is a big deal, but I find it odd I have to wait for me server to wake up in order to access Add-ons to watch content from the web.

@t4-ravenbird
Copy link
Contributor Author

@da-anda ready or not, I am not sure to be honest. It seems to work fine for windows-users as far as I have heard, but my own openelec-build is giving me concerns. It works as expected when booting and if xbmc as been idle for a while, but when xbmc resumes from suspend I have some observations I dont like. There appears to be various things that kicks in because of the 'OnWake' event .. and I also for some reason get premature ping-response when resuming.
In any case, I will update my repo

@kricker , that observation should be handled separately IMO. The woa feature just helps to reveal that accessing add-ons for some reason causes db-access (probably) and if that is not a justified behavior than it should be addressed.

@t4-ravenbird
Copy link
Contributor Author

Last update also adds a new 'ping'-function by trying connect on specified port
This was suggested in the thread here http://forum.xbmc.org/showthread.php?tid=124340&pid=1316368#pid1316368 and I think it is a good idea and implemented.

@Memphiz
Copy link
Member

Memphiz commented Feb 8, 2013

Is that last connect osx/ios/android save?

@t4-ravenbird
Copy link
Contributor Author

I merged your original stuff (including proj files) so they might be OK already, can you check it?

@Memphiz
Copy link
Member

Memphiz commented Feb 9, 2013

Yes they are ok - i've ment the new connect ping stuff - i will try to test next week (am out of town atm).

@t4-ravenbird
Copy link
Contributor Author

OK, yes I hope it builds on all targets (unless there is some diff in fcntl/sockopt/errno codes?)

@bilbonvidia
Copy link

When will this get merged with mainline?

@ghost
Copy link

ghost commented Feb 14, 2013

later than initially. we now have to go to yet another puppy funeral. every time one of you guys write these useless messages, one dies, and we have to attend the funeral.

@t4-ravenbird
Copy link
Contributor Author

Seems I have been chasing a ghost when I had issues with stability ; it all boiled down to a misbehaving server that occasionally responded to ping prematurely. It was an xp-machine, after I upgraded to win7 there is no issues at all.

so I combined all my changes into one commit and updated the repo ; it is also rebased

// process messages which have to be send to the gui
// (this can only be done after g_windowManager.Render())
CApplicationMessenger::Get().ProcessWindowMessages();
// only do full processing when m_slowTimer running

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@Memphiz
Copy link
Member

Memphiz commented Feb 25, 2013

Someone should implement a true ping for nix ... calling system is nothing we want to do for sure!

@t4-ravenbird
Copy link
Contributor Author

true ping would certainly be better but requires root AFAIK so that is why I made it like this.
I assume you dont like the overhead (or is there something else wrong with system()?) but code using ping() must in any case be prepared that it is blocking for a while

@kricker
Copy link
Member

kricker commented Mar 8, 2013

So we appeared to have stalled out again on the PR. What can we do to move this forward again? Has Androis and OS X been tested yet?
If we could somehow get the PVR Power saving integrated into this, I think it would be full featured and ready to merge. It seems silly to have PVR with its own wake settings.

@t4-ravenbird
Copy link
Contributor Author

I just updated to match latest changes (some settings have been shuffled around a bit that affected this PR)
There is only 1 commit, but parts of that is (still) thanks to @pieh and @Memphiz

Please have a look and let me know if something should be changed or anything else I can do to make it ready to be merged

{
while (!ShouldCancel(0,0))
{
if (m_parent->Ping())

This comment was marked as spam.

This comment was marked as spam.

@theuni
Copy link
Contributor

theuni commented Apr 5, 2013

Echoing what @Memphiz said above.

Please don't use system() for ping, but implement an actual ping. If root is required, then it will be required to spawn/use the program too, so I'm not following your logic there.

This probably won't work on android (if it does, it will be hit or miss), and ping is not guaranteed to exist on embedded systems. Please track down a simple implementation (c/p from busybox should work i'd think) and use it instead.

@t4-ravenbird
Copy link
Contributor Author

@theuni OK, will investigate busybox. Regarding spawning it works since the 'ping' command has some special permissions if i recall correctly

@t4-ravenbird
Copy link
Contributor Author

Some links on ping ;
http://stackoverflow.com/questions/1189389/python-non-privileged-icmp
https://developer.apple.com/library/mac/#documentation/Darwin/Reference/ManPages/man4/icmp.4.html
https://groups.google.com/forum/?fromgroups=#!topic/android-ndk/Xnc8c3A3OLw

As I understand it ;

  • ping requires opening a socket in raw mode ; socket (AF_INET, SOCK_RAW, IPPROTO_ICMP)
  • opening socket in raw mode requires root
  • ping utility (and busybox) is installed with permissions to "auto-switch to root" if started by non-root user and are therefore able to get the job done.
  • for DARWIN it is explicitly permitted to do ; socket (AF_INET, SOCK_DGRAM, IPPROTO_ICMP) to get around the problem and be able to do a non-root icmp ping
  • it is unclear if Linux supports the same and for which versions/distributions ; Android fails according to discussion on google above.

I checked my old Android-phone (v 2.3.7) and it has 'ping' utility on it (but phone is rooted and running MIUI so I dont know what its worth)

I think using "system("ping")" is the approach with the best chance of getting it to work on most targets ; and if it does not work this patch also adds a "ping by tcp-connect" feature that can be used, check https://github.com/xbmc/xbmc/pull/1892/files#L13R358

@t4-ravenbird
Copy link
Contributor Author

Updated PR and

My private machines (openelec) are running this and behaving well so far

@t4-ravenbird
Copy link
Contributor Author

The pointer dereference issue made me check again and I realized that after merging with changed handling of settings-files (and inheriting ISettingsHandler) protection is needed for the list of wake-up-servers, so latest commit adds that

@t4-ravenbird
Copy link
Contributor Author

@jmarshallnz Thanks for your input on this PR. After I changed the protection against reentry as discussed above it has run without faults so is looking OK. If you have the time please have a new look and let me know if there is anything more I should look at or if it can be queued for merge?

@@ -4534,7 +4534,51 @@ msgctxt "#13025"
msgid "Joystick unplugged"
msgstr ""

#empty strings from id 13026 to 13049
msgctxt "#13026"

This comment was marked as spam.

@t4-ravenbird
Copy link
Contributor Author

@MartijnKaijser Done. @garbear I checked other recently added files and they are stamped only 2013 so I stick to that too

@t4-ravenbird
Copy link
Contributor Author

Updated for refactored settings system.

@da-anda
Copy link
Member

da-anda commented May 4, 2013

I noticed that if my NAS didn't wake up in time (and not even in "extra time") and WOL is aborted, I'm not able to play the movie or anything else from that share even after my NAS is finally up. I have to exit and reenter the current view/folder in order to finally play it. Is it related to code of this PR or some temporary flag XBMC core is setting for a file/share? I'm on windows.

@kricker
Copy link
Member

kricker commented May 4, 2013

I'm pretty sure that is core XBMC code. Since your server did not wake in time XBMC thinks the item is missing and will not play it.

@t4-ravenbird
Copy link
Contributor Author

I think it is windows networking that flags a share as unavailable for a while after a failed connection attempt. I think you could make a similar observation connecting a share using windows explorer.

@t4-ravenbird
Copy link
Contributor Author

Any chance this can be merged in this window? What is missing or wrong, as far as I know I have resolved all findings that has been pointed out and I am still ready to amend whatever is preventing it from going in

@da-anda
Copy link
Member

da-anda commented May 9, 2013

could you please rebase again @t4-ravenbird ? I try to convice devs to hit the green button :)

@t4-ravenbird
Copy link
Contributor Author

@da-anda rebased .. and thanks for your involvement! and thanks to @Memphiz for darwin port, I hope I managed to merge that work correctly (there has been many iterations but hopefully it did not break on the way)

@da-anda
Copy link
Member

da-anda commented May 10, 2013

Thanks @t4-ravenbird. @Memphiz or any other mac user - could you please check if it still compiles and is working?

@t4-ravenbird
Copy link
Contributor Author

@da-anda had to go in order to get response from system("ping")
result from the system() call would fail always when SA_NOCLDWAIT was installed (might differ between distributions)

edit : i think this is same issue ; http://stackoverflow.com/questions/11084959/system-always-returns-non-zero-when-called-as-cgi
and this http://compgroups.net/comp.unix.programmer/system-fails-with-errno-echild/536132

@da-anda
Copy link
Member

da-anda commented May 10, 2013

@t4-ravenbird I unfortunately can't find a OSX tester. I did a test-compile via Jenkins (link in according form topic), so compile seems fine, but function test is missing. We still have a couple of hours, but chances are that it (again) might not make it into this merge window - sorry.

@regnets
Copy link

regnets commented May 10, 2013

I'm not a developer, but i got a working macbook and willing to test the pr. Where can i find the compiled binary?

@t4-ravenbird
Copy link
Contributor Author

@da-anda well you have in any case done a great job, maybe now if it misses this window it could be 'staged' to go into next at least?

@regnets http://forum.xbmc.org/showthread.php?tid=124340&pid=1417537#pid1417537

da-anda pushed a commit that referenced this pull request May 10, 2013
Wake On Access (woa) - 'Just in time' WOL when accessing MySQL or FileShare Server
@da-anda da-anda merged commit 2e96671 into xbmc:master May 10, 2013
@@ -216,7 +216,12 @@ int CPowerManager::BatteryLevel()
}
void CPowerManager::ProcessEvents()
{
m_instance->PumpPowerEvents(this);
static int nesting = 0;

This comment was marked as spam.

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature non-breaking change which adds functionality Type: Improvement non-breaking change which improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet