in reply to Re^2: Parsing Motorola S-Rec file
in thread Parsing Motorola S-Rec file

Looks pretty good to me already! That said, here's a few suggestion for what I'd do differently, personally:

So that's how I'd do it. Just for the sake of convenience, here's my version of this subroutine, in full:

use List::Util; # ... # Parse a single line of S-record, validate the line, and extract the +address # and data fields. Return a list of three scalars: # 1. TRUE if extraction was successful, FALSE otherwise (i.e. not a v +alid data # carrying S-record, bad s-record length, bad s-record checksum). # 2. The address of the line's payload, as a string. # 3. The data of the line's payload, as a string. sub ProcessSrecLine { # Get rid of all line endings, regardless of which flavor they are (my $line = shift) =~ s/[\r\n]//g; # does this look like a S1, S2, or S3 record? These are the only o +nes that # carry an actual data payload. Other record types do not, so just + ignore # them. return unless $line =~ m/^s([1-3])(\p{IsXDigit}{2})(\p{IsXDigit}{4 +,6,8})(\p{IsXDigit}+)(\p{IsXDigit}{2})$/; my ($type, $count, $address, $data, $cs) = ($1, $2, $3, $4, $5); # does the address length match the record type? return unless length $address == 2 * ($type + 1); # Make sure the COUNT field makes sense. It should represent the s +um of # address, data, and checksum fields, in bytes return unless hex($count) == (length($address) + length($data) + l +ength($cs)) / 2; # Make sure the checksum makes sense. It is the two's complement o +f # the sum of count, address, and data fields my $compCs = reduce { $a + $b } map { hex $_ } ($count, $address =~ m/../g, $data =~ m/../g); $compCs %= 256; $compCs = 255 - $compCs; # if the checksum actually matches, then call it a good S-record my $status = $compCs == hex($cs); return ($status, $address, $data); }

All of this is just my personal preferences and style, of course. As always, TIMTOWTDI (there is more than one way to do it), and I'm hardly an experienced Perl programmer anyway, so any advice I give may well fly in the face of established best practices. Caveat lector. :)

Another note - all this is completely untested, too.

EDIT: of course, there's also no need for a $status variable if you only assign to it once and then return, too:

# ... # if the checksum actually matches, then call it a good S-record return ($compCs == hex($cs), $address, $data);

Replies are listed 'Best First'.
Re^4: Parsing Motorola S-Rec file
by Tux (Canon) on Sep 03, 2014 at 13:46 UTC

    Why not take the last step too?

    $ cat test.pl use 5.16.2; use warnings; use Benchmark qw( cmpthese ); use List::Util qw( reduce sum ); my $count = 12; my $address = "A42187B56F"; my $data = "39AD0D96D51CD3"; my $r = sub { reduce { $a + $b } map { hex $_ } ($count, $address =~ m/../g, $data =~ m/../g); }; my $s = sub { sum map { hex } $count, (unpack "(A2)*", $address), (unpack "(A2)*", $data); }; say $r->(); say $s->(); cmpthese (-2, { LUreduce => $r, LUsum => $s }); $ perl test.pl 1487 1487 Rate LUreduce LUsum LUreduce 216392/s -- -40% LUsum 359162/s 66% --

    Enjoy, Have FUN! H.Merijn
      Ah, good point! I'm so used to using reduce for everything that sum didn't even occur to me. (And I've never been as comfortable with (un)pack as with regular expressions; time to remedy that, perhaps.)
      I'm sorry, but my Perl understanding is not quite up to par to understand what's going on here, or why this is better. Can you please clarify?

        Using sum plus unpack "(A2)*" is 66% faster than using reduce plus m/../g.


        Enjoy, Have FUN! H.Merijn