Hello builat

A couple of things have been mentioned already. Apart from that there are 2 things that I find pretty bad about your code:

a) _device_read() is misnamed

sub _device_read{
   my $d = shift;  # reduced line to relevant parts

   ...

   # parse device config
   $d->{regs}=from_json( $d->{regs} );
[download]

You are modifying the provided hash reference but the function name implies no such thing. A more suitable description would be something like '_convert_registers_and_device_read' if this is intentional or assign the result of from_json($d->{regs}) to a temporary variable if it is not. As a third option you could extract the JSON-conversion into a different function.

b) error handling

sub _device_read{
  ...
  $r->{$reg} = _prittyfy_byte(...);
  ...
};

sub _prettify_byte{
    return (...) if( $_[0] );
    return( $c{_err_} );
};
[download]

If prettify fails, then it returns a hash key from a global variable? Why even bother to return something if it's a static string? Why not simply return undef? (If it's an error message, then you can access $c{_err_} later on anyway?)

additional comment 1: Why do you convert a byte into a float in the first place?

additional comment 2: How does this work? ('_prettify_byte([1, 2])' results in 0.00)

General comments

Technically there's nothing wrong with optimizing your code to use as few variables as possible. From a maintenance / debug POV I think that is bad. If I can split a computation into separate steps then usually I put the value into a suitably named variable. My loop could end up like this:

my %registers = %{$d->{regs}->{r}};
foreach my $reg (keys %registers){
  my $reg_id  = hex $reg;
  my $reg_num = $registers{$reg};

  # read_input_registers returns an array reference
  my $tmp = $mbc->read_input_registers($reg_id, $reg_num);

  $r->{$reg} = _prettify_byte($tmp);
};
[download]

or maybe like this:

my %registers = %{$d->{regs}->{r}};
foreach my $reg (keys %registers){
  # read_input_registers returns an array reference
  my $tmp = $mbc->read_input_registers(hex $reg, $registers{$reg});

  $r->{$reg} = _prettify_byte($tmp);
};
[download]

...but not as a single statement that spans multiple lines and calls 2 different functions.

Why? I'm optimizing for debugging / maintenance and not for performance. If performance turns out to be an issue, then I can still optimize for it later on. But first I need to get the code correct. Having variables I can inspect and validate is a nice thing. Being able to tell at a glance if a statement looks reasonable is another.

Apart from that: I dislike your coding convention regarding $d and $r. I think it's totally fine to use a single letter variable inside a small loop (let's say 4 lines of code). For everything longer than that I'd like to have a descriptive name. Even if it's just 'input' and 'result'. If it works for you, then fine. I'm just saying I don't like it.

---

---

Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!

Titles consisting of a single word are discouraged, and in most cases are disallowed outright.

Read Where should I post X? if you're not absolutely sure you're posting in the right place.

Please read these before you post! —

• How do I compose an effective node title?
• How do I post a question effectively?
• Markup in the Monastery

Posts may use any of the Perl Monks Approved HTML tags:

a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr

You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)

	For:		Use:
	&		&
	<		&lt;
	>		&gt;
	[		&#91;
	]		&#93;

Link using PerlMonks shortcuts! What shortcuts can I use for linking?

See Writeup Formatting Tips and other pages linked from there for more info.