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

PERL WARNING - sub ConvLaCrosse robust #1038

Merged
merged 9 commits into from
Nov 27, 2021
Merged

Conversation

HomeAutoUser
Copy link
Contributor

@HomeAutoUser HomeAutoUser commented Nov 24, 2021

  • Please check if the PR fulfills these requirements
  • Tests for the changes have been added / modified (needed for for bug fixes / features)
  • commandref has been added / updated (needed for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  • Bugfix (please link issue)
  • Feature enhancement
  • Documentation update
  • Unittest enhancement
  • other
  • What is the current behavior?
    (You can also link to an open issue here, if this describes the current behavior)

    • sub ConvLaCrosse creates PERL WARNING because it is not robust
      2021-11-23T22:45:19.0077148Z ##[error]Illegal hexadecimal digit 'P' ignored at FHEM/lib/SD_Protocols.pm line 1973.
      --> cause, test submit not valid hex value
      subtest 'test ConvLaCrosse, not hexadezimal' => sub {
      plan(2);
      my $hexMsg='010503B7PA1041AAAAAAAAPF';
      my @ret=$Protocols->ConvLaCrosse($hexMsg) ;
      is($#ret,1, "ConvLaCrosse reported some error");
      like($ret[1],qr/!= checksum/,'check error message');
      };

      --> sub in SD_Protocols.pm does not check for HEX validity
      sub ConvLaCrosse {
      my $self = shift // carp 'Not called within an object';
      my $hexData = shift // croak 'Error: called without $hexdata as input';
      return ( 1,'ConvLaCrosse, Usage: Input #1, $hexData needs to be at least 8 chars long' )
      if ( length($hexData) < 8 ) ; # check number of length for this sub to not throw an error
      my $ctx = Digest::CRC->new( width => 8, poly => 0x31 );
      my $calcCrc = $ctx->add( pack 'H*', substr( $hexData, 0, 8 ) )->digest;
      my $checksum = sprintf( "%d", hex( substr( $hexData, 8, 2 ) ) ); # Todo: Needs some check to be hexadzimal conform
      return ( 1, qq[ConvLaCrosse, checksumCalc:$calcCrc != checksum:$checksum] )
  • What is the new behavior (if this is a feature change)?

    • sub ConvLaCrosse is robust
    • this PERL WARNING no longer exists
  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • Other information:

@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #1038 (e3a8cca) into master (6959426) will increase coverage by 0.10%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1038      +/-   ##
==========================================
+ Coverage   60.68%   60.78%   +0.10%     
==========================================
  Files         117      117              
  Lines        9004     8984      -20     
  Branches     1411     1409       -2     
==========================================
- Hits         5464     5461       -3     
+ Misses       2533     2518      -15     
+ Partials     1007     1005       -2     
Flag Coverage Δ
fhem 51.54% <50.00%> (+0.10%) ⬆️
modules 60.78% <80.00%> (+0.10%) ⬆️
perl 91.61% <80.00%> (-0.05%) ⬇️
unittests 60.78% <80.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
FHEM/lib/SD_Protocols.pm 79.17% <50.00%> (-0.10%) ⬇️
t/SD_Protocols/02_ConvLaCrosse.t 100.00% <100.00%> (ø)
t/FHEM/14_SD_BELL/09_parseDatat.t
t/FHEM/14_SD_AS/00_load.t 100.00% <0.00%> (ø)
FHEM/00_SIGNALduino.pm 63.81% <0.00%> (+0.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6959426...e3a8cca. Read the comment docs.

@elektron-bbs
Copy link
Contributor

Wollen wir denn jetzt wirklich alles doppelt und dreifach prüfen?
Diese Prüfung ist doch schon in der 00_SIGNALduino.pm sub SIGNALduino_Parse_MN enthalten:

  if ($rmsg !~ /^MN;D=[0-9A-F]+;(?:R=[0-9]+;)?$/){
    $hash->{logMethod}->($hash->{NAME}, 3, qq[$hash->{NAME}: Parse_MN, faulty msg: $rmsg]);
    return ; # Abort here if not successfull
  }

@HomeAutoUser
Copy link
Contributor Author

Es geht mir in diesem PR nicht ums doppelt prüfen, es geht mir um die Robustheit der Sub als einzelnes.
Der Gedanke war mal, das man Subs, einzeln nutzen kann um ggf Code zu sparen. Ja es kommt sehr selten vor. (fast unwarscheinlich in Devicespezifischen)

Nur wenn man jede Sub separat absichert, so kann man diverse PERL WARNINGs vermeiden. Es macht in meinen Augen keinen Sinn, Subs aufzurufen und zu testen indem man künstlich Warnings generiert. Entweder richtig oder die Liste und Logs werden länger und länger mit WARNINGs.

@@ -1965,12 +1965,15 @@ sub ConvLaCrosse {
my $self = shift // carp 'Not called within an object';
my $hexData = shift // croak 'Error: called without $hexdata as input';

return ( 1,"ConvLaCrosse, Usage: Input #1, $hexData is not valid HEX" )
Copy link
Contributor

Choose a reason for hiding this comment

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

Rein wegen der Optik, wollte ich erst vorschlagen, hier nur auf qq zu ändern, aber die Meldung ist nicht für einen Anwender sondern eher für einen Entwickler der diese Funktion aufruft. Daher würde ich analog zu den anderen Validierungen auf croak wechseln um den Aufrufer als Fehlerquelle ausgeben:

Suggested change
return ( 1,"ConvLaCrosse, Usage: Input #1, $hexData is not valid HEX" )
croak qq[ConvLaCrosse, Usage: Input #1, $hexData is not valid HEX] ;
``

@HomeAutoUser
Copy link
Contributor Author

@sidey79 bittefein :-)

@HomeAutoUser HomeAutoUser merged commit c252041 into master Nov 27, 2021
@HomeAutoUser HomeAutoUser deleted the master_PW_ConvLaCrosse branch November 29, 2021 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants