Go Down

Topic: Whack-a-bug (Read 950 times) previous topic - next topic

Coding Badly


I just spent about 30 minutes tracking down a very annoying bug.  The code below is the distillation.  Can you spot the problem?  (Code should run on any Arduino compatible.)

Code: [Select]
#define USISIF  7
#define USIOIF  6
#define USIPF   5
#define USIDC   4
#define USICNT3 3
#define USICNT2 2
#define USICNT1 1
#define USICNT0 0

void setup( void )
{
  Serial.begin( 115200 );
}

void loop( void )
{
  Serial.print(
      (1 << USIOIF)
      |  // USIOIF: Counter Overflow Interrupt Flag; cleared because it is used in the loop below.
//      (1 << USIPF) | // USIPF: Stop Condition Flag; cleared for no particular reason.
//      (1 << USIDC) | // USIDC: Data Output Collision; cleared for no particular reason.
      (0 << USICNT3) | (0 << USICNT2) || (0 << USICNT1) | (0 << USICNT0)  // USICNT3:0: Counter Value; cleared because it is used in the loop below.
      , HEX );

  Serial.print( F(" =? " ) );

  Serial.print(
      (1 << USIOIF)
//      |  // USIOIF: Counter Overflow Interrupt Flag; cleared because it is used in the loop below.
//      (1 << USIPF) | // USIPF: Stop Condition Flag; cleared for no particular reason.
//      (1 << USIDC) | // USIDC: Data Output Collision; cleared for no particular reason.
//      (0 << USICNT3) | (0 << USICNT2) || (0 << USICNT1) | (0 << USICNT0)  // USICNT3:0: Counter Value; cleared because it is used in the loop below.
      , HEX );

  Serial.println();

  delay( 1000 );
}

MichaelMeissner

#1
Apr 03, 2013, 02:54 pm Last Edit: Apr 03, 2013, 02:58 pm by MichaelMeissner Reason: 1
You used '||' in one place instead of '|'
Code: [Select]

(0 << USICNT2) || (0 << USICNT1)


Also, you really don't need all of the 0 << n, since that is always 0, unless you want it for documentation.

It might be simpler to have a bunch of const int variables and let the compiler optimize these into a single mask:

Code: [Select]

const unsigned overflow           = 1U << USIOIF;
const unsigned stop_condition = 1U << USIPF;
const unsigned data_collision   = 1U << USIDC;

Serial.print (overflow | stop_condition | data_collison, HEX);

Jack Christensen

Quote
//      (0 << USICNT3) | (0 << USICNT2) || (0 << USICNT1) | (0 << USICNT0)  // USICNT3:0: Counter Value; cleared because it is used in the loop below.


That one belongs to a category of mistakes that I never seem to be able to completely stop making!
MCP79411/12 RTC ... "One Million Ohms" ATtiny kit ... available at http://www.tindie.com/stores/JChristensen/

Coding Badly

You used '||' in one place instead of '|'


Exactly!

Quote
Also, you really don't need all of the 0 << n, since that is always 0, unless you want it for documentation.


Ditto!  (Can copy-paste-modify without spending so much time with the datasheet.)

Quote
It might be simpler to have a bunch of const int variables and let the compiler optimize these into a single mask:


Thanks!

Coding Badly

That one belongs to a category of mistakes that I never seem to be able to completely stop making!


My bane are inverted logical operations (e.g. using less-than instead of greater-than).  That one was just a stuttering finger.

dc42


You used '||' in one place instead of '|'


Exactly!


I believe gcc is capable of warning about this if you add -Wlogical-op to the command line. Unfortunately, it doesn't appear that the Arduino IDE provides any easy way of adding extra options to the gcc command line.
Formal verification of safety-critical software, software development, and electronic design and prototyping. See http://www.eschertech.com. Please do not ask for unpaid help via PM, use the forum.

Coding Badly


I think version 1.5 does through one of the new configuration files.

Thanks for the tip!

Go Up