hollie / misterhouse

Perl open source home automation program. It's fun, it's free, and it's entirely geeky.
242 stars 130 forks source link

Network item - using WOL blocks status #773

Open keynet opened 5 years ago

keynet commented 5 years ago

I updated Network_Item from to the latest GIT version so as to include the Wake-On-LAN function, previously in a separate library which I've used extensively.

However, the use of the new 'start' state (for WOL function) seems to break the ability to subsequently report network status - after 'start' is used to wake a device (which works, shows state 'start'), the states 'up' and 'down' no longer get reported - at least with my version of Perl (5.18) - or maybe other factors? This is repeatable. The ping process for the relevant item no longer runs (results file not updated).

keynet commented 5 years ago

Answering myself...

The issue seems to be that using a 'set' command nearly always kills the ping timer associated with that network item, so killing the monitor for good, as it is only started once in 'new'. The timer is stored in the Network Item object as 'timer' here: $self->{timer} = new Timer; But Network Item inherits 'Generic_item' functions, so appears to conflict with an existing variable by the same name.

Working (proposed solution: rename the variable)

$self->{nw_ping_timer} = new Timer;
$self->{nw_ping_timer}->set( $interval, sub { Network_Item::ping_check($self); }, -1 );

While I'm here, the 'default_setstate' function isn't used, and this code in 'setstate_start' doesn't do anything useful, and if it did what was intended, I think not desirable as it assumes actual state which is not necessarily true (may be already 'up' when a start is actioned).

if ( $self->state eq "start" ) {
     $state eq "down";
     $self->set($state);
 }
hplato commented 5 years ago

So a few things;

  1. Thanks for doing this! I’ve always thought that WOL and network_item should be combined together.
  2. Interesting about set — i’ve seen this behaviour before in some of my code, since items like razberry, opensprinkler, venstar, nanoleaf all rely on polling which was based off network_item. I’ll look through some of my stuff to see if I might have had a workaround.
  3. I’m wondering if maybe the states should be changed to on and off to make it more consistent with all the other states in MH. It might break some usercode, but would be a better long term change. I’d be interested in other thoughts around this.

On Dec 30, 2018, at 6:35 AM, keynet notifications@github.com wrote:

Answering myself...

The issue seems to be that using a 'set' command nearly always kills the ping timer associated with that network item, so killing the monitor for good, as it is only started once in 'new'. The timer is stored in the Network Item object as 'timer' here: $self->{timer} = new Timer; But Network Item inherits 'Generic_item' functions, so appears to conflict with an existing variable by the same name.

Working (proposed solution: rename the variable)

$self->{nw_ping_timer} = new Timer; $self->{nw_ping_timer}->set( $interval, sub { Network_Item::ping_check($self); }, -1 ); While I'm here, the 'default_setstate' function isn't used, and this code in 'setstate_start' doesn't do anything useful, and if it did what was intended, I think not desirable as it assumes actual state which is not necessarily true (may be already 'up' when a start is actioned).

if ( $self->state eq "start" ) { $state eq "down"; $self->set($state); } — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/hollie/misterhouse/issues/773#issuecomment-450561202, or mute the thread https://github.com/notifications/unsubscribe-auth/AExbSMNHs5o-uO4XNDdSqV_PUd66mN_Uks5u-MEKgaJpZM4ZkATS.

keynet commented 5 years ago

I think once-upon-a-time it might have made sense to change the network states to ON and OFF for uniformity, but probably not now as a) there's plenty of code out there that would need to be changed, b) ON and OFF may possibly have been used as well as 'up' and 'down' (by others, considering generic item inheritance), and c) there are many items that use special values in the state variable to indicate various conditions - it's not so unusual.

keynet commented 4 years ago

And one more for this item... At least in Linux systems - this tiny change stops all the "Name or service not known" logspam on the terminal when network items are offline: $self->{process} = new Process_Item( $ping_test_cmd . $address . " 2>&1 | grep ttl" );