From ecee24f6afe276397812a2e33fa5fea1e180dc0f Mon Sep 17 00:00:00 2001 From: Michael Stovenour Date: Fri, 6 Dec 2013 08:36:06 -0600 Subject: [PATCH 1/5] Fix data3 for responder records to PLM scene Fix Issue #327. This fix also applies when a responder is I2CS and the controller is I1. Added logging for the data3 value. Aligned the logging levels between sync_links() and update_link(). --- lib/Insteon/AllLinkDatabase.pm | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/Insteon/AllLinkDatabase.pm b/lib/Insteon/AllLinkDatabase.pm index 3a4a3a5f9..a2190c7e6 100644 --- a/lib/Insteon/AllLinkDatabase.pm +++ b/lib/Insteon/AllLinkDatabase.pm @@ -853,7 +853,7 @@ sub add_link # For I2CS devices the default data3 for links is 01 # For all other devices the default data3 for links is 00 my $data3_default = '00'; - if ($insteon_object->can('engine_version') && $insteon_object->engine_version eq 'I2CS') { + if ($$self{device}->can('engine_version') && $$self{device}->engine_version eq 'I2CS') { $data3_default = '01'; } my $data3 = ($link_parms{data3}) ? $link_parms{data3} : '00'; @@ -912,7 +912,7 @@ sub add_link { &::print_log("[Insteon::AllLinkDatabase] DEBUG2: adding link record to " . $$self{device}->get_object_name . " light level controlled by " . $insteon_object->get_object_name - . " and group: $group with on level: $on_level and ramp rate: $ramp_rate") + . " and group: $group with on level: $on_level, ramp rate: $ramp_rate, local load(data3): $data3") if $self->{device}->debuglevel(2, 'insteon'); my ($data1, $data2); if($link_parms{is_controller}) { @@ -971,20 +971,23 @@ sub update_link # strip optional s (seconds) to append ramp_rate my $ramp_rate = $link_parms{ramp_rate}; $ramp_rate =~ s/(\d)s?/$1/; - &::print_log("[Insteon::AllLinkDatabase] updating " . $$self{device}->get_object_name . " light level controlled by " . $insteon_object->get_object_name - . " and group: $group with on level: $on_level and ramp rate: $ramp_rate") if $self->{device}->debuglevel(1, 'insteon'); my $data1 = &Insteon::DimmableLight::convert_level($on_level); my $data2 = ($$self{device}->isa('Insteon::DimmableLight')) ? &Insteon::DimmableLight::convert_ramp($ramp_rate) : '00'; # For I2CS devices the default data3 for links is 01 # For all other devices the default data3 for links is 00 my $data3_default = '00'; - if ($insteon_object->can('engine_version') && $insteon_object->engine_version eq 'I2CS') { + if ($$self{device}->can('engine_version') && $$self{device}->engine_version eq 'I2CS') { $data3_default = '01'; } my $data3 = ($link_parms{data3}) ? $link_parms{data3} : '00'; $data3 = $data3_default if ($data3 eq '00' || $data3 eq '01'); + &::print_log("[Insteon::AllLinkDatabase] DEBUG2: updating " . $$self{device}->get_object_name + . " light level controlled by " . $insteon_object->get_object_name + . " and group: $group with on level: $on_level, ramp rate: $ramp_rate, local load(data3): $data3") + if $self->{device}->debuglevel(2, 'insteon'); + $$self{_success_callback} = ($link_parms{callback}) ? $link_parms{callback} : undef; $$self{_failure_callback} = ($link_parms{failure_callback}) ? $link_parms{failure_callback} : undef; From 6efe1b7ead2752e074f5a496704c543ea721244e Mon Sep 17 00:00:00 2001 From: Michael Stovenour Date: Fri, 13 Dec 2013 08:24:31 -0600 Subject: [PATCH 2/5] Move data3 logic into the Insteon device objects Add link_data3() function at the device level Modify aldb functions that set or check data3 to use the new function Add grotesque, level 5 logging that should be removed later --- lib/Insteon/AllLinkDatabase.pm | 51 ++++++++++++++++------------------ lib/Insteon/BaseInsteon.pm | 37 ++++++++++++++++++++++-- lib/Insteon/Lighting.pm | 30 ++++++++++++++++++++ 3 files changed, 89 insertions(+), 29 deletions(-) diff --git a/lib/Insteon/AllLinkDatabase.pm b/lib/Insteon/AllLinkDatabase.pm index a2190c7e6..202e0a7e6 100644 --- a/lib/Insteon/AllLinkDatabase.pm +++ b/lib/Insteon/AllLinkDatabase.pm @@ -488,7 +488,7 @@ sub delete_orphan_links # Initialize Variables my ($linked_device, $plm_scene, $controller_object, $link_defined, - $controller_id, $responder_id, $link_data3, $recip_data3, + $controller_id, $responder_id, $recip_data3, $group_object, $data3_object); my $group = lc $$self{aldb}{$linkkey}{group}; my $is_controller = $$self{aldb}{$linkkey}{is_controller}; @@ -504,6 +504,7 @@ sub delete_orphan_links %delete_req = (%delete_req, deviceid => $deviceid, group => $group, is_controller => $is_controller, data3 => $data3); +::print_log("[Insteon::AllLinkDatabase] DEBUG-omg: Evaluating $selfname($self_id) link: is_controller=$is_controller deviceid=$deviceid group=$group data3=$data3 interface_id=$interface_id") if $main::Debug{insteon} >= 5; # Is the linked device defined in MH? if (! ref $linked_device) { $delete_req{cause} = "no device with deviceid: $deviceid could be found"; @@ -536,21 +537,21 @@ sub delete_orphan_links my @lights = $member->find_members('Insteon::BaseLight'); $member = $lights[0] if (@lights); # pick the first } - # For resp, D3 = resp group; For cont, D3 = cont group - $link_data3 = ($is_controller) ? $group : $member->group; +#TODO - In the ALDB, the primary key is the combination of device ID "and" group +#It is possible to link two buttons on a keypad link to the same controller +#e.g. one button turns on and the other turns off when the controller activates +#So then we should be checking the group here too. Otherwise when removing links +#this could inadvertently leave extra links in the ALDB. if (lc($member->device_id) eq $responder_id) { + #Ask self what we should have in data3 + #For rspndr, D3 = rspndr group; For ctrlr, D3 = ctrlr group + my $link_data3 = $$self{device}->link_data3(($is_controller ? $group : $member->group), $is_controller); +::print_log("[Insteon::AllLinkDatabase] DEBUG-omg: member: ".$member->get_object_name."(".$member->device_id.") data3: $data3; link data3: $link_data3") if $main::Debug{insteon} >= 5; if ($data3 eq $link_data3){ $link_defined = 1; $recip_data3 = ($is_controller) ? $member->group : $group; last MEMBERS; } - elsif (($link_data3 eq '00' || $link_data3 eq '01') && - ($data3 eq '00' || $data3 eq '01' )){ - # Allow for 00 or 01 interchangability - $link_defined = 1; - $recip_data3 = ($is_controller) ? $member->group : $group; - last MEMBERS; - } } } @@ -850,14 +851,7 @@ sub add_link } my $is_controller = ($link_parms{is_controller}) ? 1 : 0; - # For I2CS devices the default data3 for links is 01 - # For all other devices the default data3 for links is 00 - my $data3_default = '00'; - if ($$self{device}->can('engine_version') && $$self{device}->engine_version eq 'I2CS') { - $data3_default = '01'; - } - my $data3 = ($link_parms{data3}) ? $link_parms{data3} : '00'; - $data3 = $data3_default if ($data3 eq '00' || $data3 eq '01'); + my $data3 = $$self{device}->link_data3($link_parms{data3},$is_controller); # check whether the link already exists my $key = $self->get_linkkey($device_id, $group, $is_controller, $data3); @@ -974,14 +968,7 @@ sub update_link my $data1 = &Insteon::DimmableLight::convert_level($on_level); my $data2 = ($$self{device}->isa('Insteon::DimmableLight')) ? &Insteon::DimmableLight::convert_ramp($ramp_rate) : '00'; - # For I2CS devices the default data3 for links is 01 - # For all other devices the default data3 for links is 00 - my $data3_default = '00'; - if ($$self{device}->can('engine_version') && $$self{device}->engine_version eq 'I2CS') { - $data3_default = '01'; - } - my $data3 = ($link_parms{data3}) ? $link_parms{data3} : '00'; - $data3 = $data3_default if ($data3 eq '00' || $data3 eq '01'); + my $data3 = $$self{device}->link_data3($link_parms{data3},$is_controller); &::print_log("[Insteon::AllLinkDatabase] DEBUG2: updating " . $$self{device}->get_object_name . " light level controlled by " . $insteon_object->get_object_name @@ -1184,7 +1171,17 @@ sub has_link $deviceid = lc $insteon_object->device_id; } my $key = $self->get_linkkey($deviceid, $group, $is_controller, $data3); - return (defined $$self{aldb}{$key}); + + #Now that we have a linkkey, compare the linkkey's data3 value. + my $found = 0; + if (defined $$self{aldb}{$key}) { + #Ask self what we should have in data3 + $data3 = $$self{device}->link_data3($data3,$is_controller); + + $found++ if( $$self{aldb}{$key}{data3} == $data3); + } + + return ($found); } =back diff --git a/lib/Insteon/BaseInsteon.pm b/lib/Insteon/BaseInsteon.pm index 00b797014..577d4e40e 100644 --- a/lib/Insteon/BaseInsteon.pm +++ b/lib/Insteon/BaseInsteon.pm @@ -2704,6 +2704,40 @@ sub get_voice_cmds return \%voice_cmds; } +=item C + +Returns the data3 value that should be used when creating a link for this device. +This sub provides the default for all Insteon devices. Individual device +definitions can override this sub to return something unique. This sub was +modivated by the need to return unique values for data3 on responder links for +group 01. Most Insteon devices will learn 00 for the data3 responder link value, +however the In-LineLinc Relay learns 01 when manually linking and requires that +value when creating the links programmatically. + +=cut + +sub link_data3 +{ + my ($self, $group, $is_controller) = @_; + + my $link_data3; + + if( $is_controller) { + #Default to 01 if no group was supplied + #Otherwise just return the group + $link_data3 = ($group) ? $group : '01'; + } else { #is_responder + #Default to 00 if no group was supplied + $link_data3 = ($group) ? $group : '00'; + + #Default for group 01 is data3 == 00 + #Override this in inerited classes if needed + $link_data3 = '00' if( $group eq '01'); + } + + return $link_data3; +} + =back =head2 INI PARAMETERS @@ -2999,10 +3033,9 @@ sub sync_links $$self{sync_queue_failure_callback} = ($failure_callback) ? $failure_callback : undef; $$self{sync_queue_failure} = undef; my $self_link_name = $self->get_object_name; - my $insteon_object = $self->interface; my $interface_object = Insteon::active_interface(); my $interface_name = $interface_object->get_object_name; - $insteon_object = $self->get_root; + my $insteon_object = $self->get_root; ::print_log("[Insteon::BaseController] WARN!! A device w/ insteon address: " . $self->device_id . ":01 could not be found. " . "Please double check your items.mht file.") if (!(defined($insteon_object))); diff --git a/lib/Insteon/Lighting.pm b/lib/Insteon/Lighting.pm index 72452196b..e554897c3 100644 --- a/lib/Insteon/Lighting.pm +++ b/lib/Insteon/Lighting.pm @@ -626,6 +626,36 @@ sub new return $self; } + +=item C + +Returns the data3 value that should be used when creating a link for this device. +This sub overides the parent class to map group 01 to data3 01. This is required +by the I2CS In-LineLinc Relay. + +=cut + +sub link_data3 +{ + my ($self, $group, $is_controller) = @_; + + my $link_data3 = SUPER::link_data3($group, $is_controller); + + if( !$is_controller) { #is_responder + #For I2CS devices the default data3 for responder links is 01. + #This is to support the I2CS In-LineLinc Relay. There may be more + #permutations of the 00 vs. 01 problem and I1 devices may have + #the same requirement. This code is a work in progress as more + #information is gathered about Relay type devices. + if ($$self{device}->can('engine_version') && $$self{device}->engine_version eq 'I2CS') { + $link_data3 = '01'; +::print_log("[Insteon::SwitchLincRelay] Overriding link_data3 to 01 for group 01"); + } + } + + return $link_data3; +} + =back =head2 AUTHOR From 8f43939f30d7e39e9da40b65f6acde81868fb487 Mon Sep 17 00:00:00 2001 From: Michael Stovenour Date: Mon, 16 Dec 2013 22:27:30 -0600 Subject: [PATCH 3/5] Added missing link_data3() for Insteon_PLM --- lib/Insteon_PLM.pm | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/lib/Insteon_PLM.pm b/lib/Insteon_PLM.pm index ab2b7bc1c..f14d5effa 100644 --- a/lib/Insteon_PLM.pm +++ b/lib/Insteon_PLM.pm @@ -927,6 +927,33 @@ sub firmware { return $$self{firmware}; } +=item C + +Returns the data3 value that should be used when creating a link for this device. +This sub was modivated by the need to return unique values for data3 on responder +links for group 01. The PLM will store the responder's devcat data for controller +entries. That's fundamentally hard so just do the same as for other devices for +now. Can make this smarter in the future if needed. + +=cut + +sub link_data3 +{ + my ($self, $group, $is_controller) = @_; + + my $link_data3; + + if( $is_controller) { + #Default to 01 if no group was supplied + #Otherwise just return the group + $link_data3 = ($group) ? $group : '01'; + } else { #is_responder + #Default to 01 if no group was supplied + $link_data3 = ($group) ? $group : '01'; + } + + return $link_data3; +} =back =head2 INI PARAMETERS From c271c0a26b5381705ff79dc7dbc82b1b4928f7e6 Mon Sep 17 00:00:00 2001 From: Michael Stovenour Date: Mon, 16 Dec 2013 23:19:48 -0600 Subject: [PATCH 4/5] Add call to link_data3 in Insteon_PLM::has_link --- lib/Insteon/AllLinkDatabase.pm | 15 +++++++++++++-- lib/Insteon/Lighting.pm | 4 ++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/Insteon/AllLinkDatabase.pm b/lib/Insteon/AllLinkDatabase.pm index 202e0a7e6..93491a22b 100644 --- a/lib/Insteon/AllLinkDatabase.pm +++ b/lib/Insteon/AllLinkDatabase.pm @@ -546,7 +546,7 @@ sub delete_orphan_links #Ask self what we should have in data3 #For rspndr, D3 = rspndr group; For ctrlr, D3 = ctrlr group my $link_data3 = $$self{device}->link_data3(($is_controller ? $group : $member->group), $is_controller); -::print_log("[Insteon::AllLinkDatabase] DEBUG-omg: member: ".$member->get_object_name."(".$member->device_id.") data3: $data3; link data3: $link_data3") if $main::Debug{insteon} >= 5; +::print_log("[Insteon::AllLinkDatabase] DEBUG-omg: member: ".$member->get_object_name."(".$member->device_id.") data3: $data3; link data3: $link_data3") if $main::Debug{insteon} >= 5; if ($data3 eq $link_data3){ $link_defined = 1; $recip_data3 = ($is_controller) ? $member->group : $group; @@ -612,6 +612,7 @@ sub delete_orphan_links # Does a reciprocal link exist? if (! $linked_device->has_link($$self{device},$group,($is_controller) ? 0 : 1, lc $recip_data3)) { +::print_log("[Insteon::AllLinkDatabase] DEBUG-omg: No reciprocal link: \$recip_data3 = $recip_data3") if $main::Debug{insteon} >= 5; $delete_req{cause} = "no reciprocal link was found on " . $linked_device->get_object_name; push @{$$self{delete_queue}}, \%delete_req; } @@ -2778,7 +2779,17 @@ sub has_link my ($self, $insteon_object, $group, $is_controller, $data3) = @_; my $key = $self->get_linkkey($insteon_object->device_id, $group, $is_controller, $data3); - return (defined $$self{aldb}{$key}); + + #Now that we have a linkkey, compare the linkkey's data3 value. + my $found = 0; + if (defined $$self{aldb}{$key}) { + #Ask self what we should have in data3 + $data3 = $$self{device}->link_data3($data3,$is_controller); + + $found++ if( $$self{aldb}{$key}{data3} == $data3); + } + + return ($found); } =back diff --git a/lib/Insteon/Lighting.pm b/lib/Insteon/Lighting.pm index e554897c3..7f8c0c205 100644 --- a/lib/Insteon/Lighting.pm +++ b/lib/Insteon/Lighting.pm @@ -639,7 +639,7 @@ sub link_data3 { my ($self, $group, $is_controller) = @_; - my $link_data3 = SUPER::link_data3($group, $is_controller); + my $link_data3 = $self->SUPER::link_data3($group, $is_controller); if( !$is_controller) { #is_responder #For I2CS devices the default data3 for responder links is 01. @@ -647,7 +647,7 @@ sub link_data3 #permutations of the 00 vs. 01 problem and I1 devices may have #the same requirement. This code is a work in progress as more #information is gathered about Relay type devices. - if ($$self{device}->can('engine_version') && $$self{device}->engine_version eq 'I2CS') { + if ($self->can('engine_version') && $self->engine_version eq 'I2CS') { $link_data3 = '01'; ::print_log("[Insteon::SwitchLincRelay] Overriding link_data3 to 01 for group 01"); } From 8ff4022c0a5c14eb835ded410d7a6481b47b53a1 Mon Sep 17 00:00:00 2001 From: Michael Stovenour Date: Wed, 18 Dec 2013 11:44:19 -0600 Subject: [PATCH 5/5] Remove grotesque debug logging --- lib/Generic_Item.pm | 1 - lib/Insteon/AllLinkDatabase.pm | 9 ++++----- lib/Insteon/Lighting.pm | 1 - 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/Generic_Item.pm b/lib/Generic_Item.pm index 0399bf523..ef1888000 100644 --- a/lib/Generic_Item.pm +++ b/lib/Generic_Item.pm @@ -1352,7 +1352,6 @@ sub debuglevel $debug_level = 1 unless $debug_level; my $objname; $objname = lc $object->get_object_name if defined $object; - ::print_log("[Generic_Item] debuglevel: Processing debug for object $objname ... " . $main::Debug{$objname}) if $main::Debug{$debug_group} >= 5; return 1 if $main::Debug{$debug_group} >= $debug_level; return 1 if defined $objname && $main::Debug{$objname} >= $debug_level; return 0; diff --git a/lib/Insteon/AllLinkDatabase.pm b/lib/Insteon/AllLinkDatabase.pm index 93491a22b..c55bdda03 100644 --- a/lib/Insteon/AllLinkDatabase.pm +++ b/lib/Insteon/AllLinkDatabase.pm @@ -504,7 +504,6 @@ sub delete_orphan_links %delete_req = (%delete_req, deviceid => $deviceid, group => $group, is_controller => $is_controller, data3 => $data3); -::print_log("[Insteon::AllLinkDatabase] DEBUG-omg: Evaluating $selfname($self_id) link: is_controller=$is_controller deviceid=$deviceid group=$group data3=$data3 interface_id=$interface_id") if $main::Debug{insteon} >= 5; # Is the linked device defined in MH? if (! ref $linked_device) { $delete_req{cause} = "no device with deviceid: $deviceid could be found"; @@ -546,7 +545,6 @@ sub delete_orphan_links #Ask self what we should have in data3 #For rspndr, D3 = rspndr group; For ctrlr, D3 = ctrlr group my $link_data3 = $$self{device}->link_data3(($is_controller ? $group : $member->group), $is_controller); -::print_log("[Insteon::AllLinkDatabase] DEBUG-omg: member: ".$member->get_object_name."(".$member->device_id.") data3: $data3; link data3: $link_data3") if $main::Debug{insteon} >= 5; if ($data3 eq $link_data3){ $link_defined = 1; $recip_data3 = ($is_controller) ? $member->group : $group; @@ -612,7 +610,6 @@ sub delete_orphan_links # Does a reciprocal link exist? if (! $linked_device->has_link($$self{device},$group,($is_controller) ? 0 : 1, lc $recip_data3)) { -::print_log("[Insteon::AllLinkDatabase] DEBUG-omg: No reciprocal link: \$recip_data3 = $recip_data3") if $main::Debug{insteon} >= 5; $delete_req{cause} = "no reciprocal link was found on " . $linked_device->get_object_name; push @{$$self{delete_queue}}, \%delete_req; } @@ -907,7 +904,8 @@ sub add_link { &::print_log("[Insteon::AllLinkDatabase] DEBUG2: adding link record to " . $$self{device}->get_object_name . " light level controlled by " . $insteon_object->get_object_name - . " and group: $group with on level: $on_level, ramp rate: $ramp_rate, local load(data3): $data3") + . " and group: $group with on level: $on_level," + . " ramp rate: $ramp_rate, local load(data3): $data3") if $self->{device}->debuglevel(2, 'insteon'); my ($data1, $data2); if($link_parms{is_controller}) { @@ -973,7 +971,8 @@ sub update_link &::print_log("[Insteon::AllLinkDatabase] DEBUG2: updating " . $$self{device}->get_object_name . " light level controlled by " . $insteon_object->get_object_name - . " and group: $group with on level: $on_level, ramp rate: $ramp_rate, local load(data3): $data3") + . " and group: $group with on level: $on_level," + . " ramp rate: $ramp_rate, local load(data3): $data3") if $self->{device}->debuglevel(2, 'insteon'); $$self{_success_callback} = ($link_parms{callback}) ? $link_parms{callback} : undef; diff --git a/lib/Insteon/Lighting.pm b/lib/Insteon/Lighting.pm index 7f8c0c205..b25f972bc 100644 --- a/lib/Insteon/Lighting.pm +++ b/lib/Insteon/Lighting.pm @@ -649,7 +649,6 @@ sub link_data3 #information is gathered about Relay type devices. if ($self->can('engine_version') && $self->engine_version eq 'I2CS') { $link_data3 = '01'; -::print_log("[Insteon::SwitchLincRelay] Overriding link_data3 to 01 for group 01"); } }