Random code flyby comment in util.c on pipelining logical evaluators

[robertlipe]

I was just reading through the code as the project caught my interest. I landed in https://github.com/GlasgowEmbedded/glasgow/blob/main/firmware/util.c and this caught my eye. I don't have the ability to build/test it, and I won't hard-sell the style (upon which you may have already taken a stance), but just as a tip, logical operators of the same type are guaranteed to be evaluated left to right and with short-circuit, so it won't call a write if the start fails and so on.

Therefore, I think that i2c_reg8_read might be more readable as

if(!i2c_start(addr<<1))
   && (!i2c_write(&reg, 1))
   && (!i2c_start((addr<<1)|1))
   && (!i2c_read(value, length))
  return true;
  
i2c_stop():
return false;

It's possible the ! should be dropped in there or the whole expression could be inverted instead of inverting every case. I can't find a definition of them to be sure of the polarity of the return value.

The point is that they'll be evaluated left to right, and evaluation will be stopped upon the first failure. At that point, stop is called and false is returned.

The same basic technique of pipelining can be used in reg8_write() and many other places in the code. Variations of whitespace may or may not be more to your taste:

if (i2c_start() && i2c_write() && i2c_start() && i2c_read())
return true;
i2c_stop()
return false;

You are, of course, free to ignore or even dislike the suggestion. It just catches my eye as an easier style to follow in my experience writing driver code like this.

As a negative to this style, it is harder to set breakpoints on individual failures or to build up meaningful profiling data, such as learning that 1.7% of the i2c_reads are never called because the second i2c_start is failing—or whatever. Rarely has this mattered to me in professional life, but they are cases I've encountered, so I'm disclosing them. It's also possible you just don't like this, and that's OK. :-)

I feel no particular need to argue about it (tabs! spaces! 2 character indents!) but I'll answer any questions I can.

Thank you for building a cool-looking project, and good luck!

[whitequark]

I am aware of the basic evaluation rules of the C language.

[robertlipe]

Cool enough. My work here is done. Happy hacking!