From 1590563c743139b1df7deb56ec81d1f6c8ad0a07 Mon Sep 17 00:00:00 2001 From: KRKeegan Date: Thu, 26 Sep 2013 17:42:00 -0700 Subject: [PATCH 1/2] Insteon: Pass Message Clearing Decision to on_read_write_aldb on_read_write_aldb now returns a 1/0 corresponding to whether the current message should be cleared. When a bad message arrived, on_read_write_aldb attempted to requeue the message that was currently pending. However, _process_message did not clear the pending message until after this routine was run. As a result, a new message was not queued because it was duplicative, but then the current message was cleared. This resulted in stalling the message queue. Fixes bug #258 --- lib/Insteon/AllLinkDatabase.pm | 18 ++++++++++++------ lib/Insteon/BaseInsteon.pm | 21 ++++----------------- 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/lib/Insteon/AllLinkDatabase.pm b/lib/Insteon/AllLinkDatabase.pm index ec74ce24c..d76a44222 100644 --- a/lib/Insteon/AllLinkDatabase.pm +++ b/lib/Insteon/AllLinkDatabase.pm @@ -2127,7 +2127,7 @@ Called as part of any process to read or write to a device's ALDB. sub on_read_write_aldb { my ($self, %msg) = @_; - + my $clear_message = 1; #Default Action is to Clear the Current Message &::print_log("[Insteon::ALDB_i2] DEBUG3: " . $$self{device}->get_object_name . " [0x" . $$self{_mem_msb} . $$self{_mem_lsb} . "] received: " . lc $msg{extra} . " for _mem_activity=".$$self{_mem_activity} @@ -2135,6 +2135,8 @@ sub on_read_write_aldb if ($$self{_mem_action} eq 'aldb_i2read') { + #This is an ACK. Will be followed by a Link Data message, so don't clear + $clear_message = 0; #Only move to the next state if the received message is a device ack #if the ack is dropped the retransmission logic will resend the request if($msg{is_ack}) { @@ -2156,6 +2158,7 @@ sub on_read_write_aldb &::print_log("[Insteon::ALDB_i2] DEBUG3: " . $$self{device}->get_object_name . " [0x" . $$self{_mem_msb} . $$self{_mem_lsb} . "] received duplicate ack. Ignoring.") if $main::Debug{insteon} >= 3; + $clear_message = 0; } elsif(length($msg{extra})<30) { &::print_log("[Insteon::ALDB_i2] WARNING: Corrupted I2 response not processed: " @@ -2163,8 +2166,9 @@ sub on_read_write_aldb . " [0x" . $$self{_mem_msb} . $$self{_mem_lsb} . "] received: " . lc $msg{extra} . " for " . $$self{_mem_action}) if $main::Debug{insteon} >= 3; $$self{device}->corrupt_count_log(1) if $$self{device}->can('corrupt_count_log'); - #retry previous address again - $self->send_read_aldb(sprintf("%04x", hex($$self{_mem_msb} . $$self{_mem_lsb}))); + #can't clear message, if valid message doesn't arrive + #resend logic will kick in + $clear_message = 0; } elsif ($$self{_mem_msb} . $$self{_mem_lsb} ne '0000' and $$self{_mem_msb} . $$self{_mem_lsb} ne substr($msg{extra},6,4)){ ::print_log("[Insteon::ALDB_i2] WARNING: Corrupted I2 response not processed, " @@ -2172,9 +2176,10 @@ sub on_read_write_aldb . $$self{device}->get_object_name . " [0x" . $$self{_mem_msb} . $$self{_mem_lsb} . "] received: " . lc $msg{extra} . " for " . $$self{_mem_action}) if $main::Debug{insteon} >= 3; - $$self{device}->corrupt_count_log(1) if $$self{device}->can('corrupt_count_log'); - #retry previous address again - $self->send_read_aldb(sprintf("%04x", hex($$self{_mem_msb} . $$self{_mem_lsb}))); + $$self{device}->corrupt_count_log(1) if $$self{device}->can('corrupt_count_log'); + #can't clear message, if valid message doesn't arrive + #resend logic will kick in + $clear_message = 0; } elsif ($$self{_stress_test_act}){ $$self{_mem_activity} = undef; @@ -2369,6 +2374,7 @@ sub on_read_write_aldb . ": unhandled _mem_action=".$$self{_mem_action}) if $main::Debug{insteon}; } + return $clear_message; } sub _write_link diff --git a/lib/Insteon/BaseInsteon.pm b/lib/Insteon/BaseInsteon.pm index b92df149e..ff9571682 100644 --- a/lib/Insteon/BaseInsteon.pm +++ b/lib/Insteon/BaseInsteon.pm @@ -776,14 +776,8 @@ sub _process_message } elsif ($pending_cmd eq 'read_write_aldb') { if ($msg{cmd_code} eq $self->message_type_hex($pending_cmd)) { - if ($self->_aldb && $self->_aldb->{_mem_action} ne 'aldb_i2writeack'){ - #This is an ACK. Will be followed by a Link Data message - $clear_message = 0; - $self->_aldb->on_read_write_aldb(%msg) if $self->_aldb; - } else { - $self->_aldb->on_read_write_aldb(%msg) if $self->_aldb; - $self->_process_command_stack(%msg); - } + $clear_message = $self->_aldb->on_read_write_aldb(%msg) if $self->_aldb; + $self->_process_command_stack(%msg) if ($clear_message); } else { $corrupt_cmd = 1; $clear_message = 0; @@ -877,15 +871,8 @@ sub _process_message $self->request_status($self); } elsif ($msg{command} eq 'read_write_aldb') { if ($self->_aldb){ - if ($self->_aldb->{_mem_action} eq 'aldb_i2readack'){ - #If aldb_i2readack is set then this is good - $clear_message = 1; - $self->_aldb->on_read_write_aldb(%msg); - $self->_process_command_stack(%msg); - } else { - #This is an out of sequence message - $self->_aldb->on_read_write_aldb(%msg); - } + $clear_message = $self->_aldb->on_read_write_aldb(%msg) if $self->_aldb; + $self->_process_command_stack(%msg) if($clear_message); } } elsif ($msg{type} eq 'broadcast') { $self->devcat($msg{devcat}); From 211592bdb8127cdaebb84c2e3fb41967b57855e0 Mon Sep 17 00:00:00 2001 From: KRKeegan Date: Fri, 27 Sep 2013 17:34:00 -0700 Subject: [PATCH 2/2] Insteon: Don't Clear Queue on Unhandled Mem_Action Missed one instance in which the queued message should not be cleared. Should not be cleared on an unhandled mem action either. Further Fix to #258 --- lib/Insteon/AllLinkDatabase.pm | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Insteon/AllLinkDatabase.pm b/lib/Insteon/AllLinkDatabase.pm index d76a44222..d67e0944b 100644 --- a/lib/Insteon/AllLinkDatabase.pm +++ b/lib/Insteon/AllLinkDatabase.pm @@ -2373,6 +2373,7 @@ sub on_read_write_aldb main::print_log("[Insteon::ALDB_i2] " . $$self{device}->get_object_name . ": unhandled _mem_action=".$$self{_mem_action}) if $main::Debug{insteon}; + $clear_message = 0; } return $clear_message; }