-1
\$\begingroup\$

I'm trying to implement some logic functions essential to control a small power system consisting of 2 Transformers labeled T1 and T2, 2 Generators (G1 and G2) and a bus tie (BT). I'm not going to overwhelm you with the unnecessary details. I need help figure out why my Arduino circuit doesn't work as expected even though the code compiles properly! I've spent too much time on it but I still can't get it. I will attach the circuit followed by the commented Arduino code.

enter image description here

The LEDS are supposed to by replaced by some relays coils (a device used to control the switching of a electrical equipment).

The following is the Arduino code:

byte lastReading;
unsigned long lastReadingTimer;
byte UpdatedPortD;
void setup() {
  lastReadingTimer = 0; //Debouncing Timer is 0 ms initially.
  // Initialize ports to I/O directions:
  DDRD = DDRD & B10000011; // Masking to perserve bits 0,1 and 7. (zeros for input)
  DDRB = DDRB | 31;     // Masking to perserve bits 6,7. PortB(8-13 pins) 0001 1111 (one=outputs).
  PORTD &= B10000011;   //Initially all inputs are LOWs.
  lastReading = PORTD & B01111100;
  UpdatedPortD = PORTD & B01111100; // Inputs to PortD, 3-7 pins.
}
//Pins from 3-7 map to switches T1,T2,G1,G2,BT respectively.
//Pins from 8-12 map to devices T1,T2,G1,G2,BT respectively.
void loop() {
  if ((PORTD & B01111100) != lastReading) {
    lastReadingTimer = millis();
  }
  if ( ((millis() - lastReadingTimer) > 50) && (UpdatedPortD != PORTD & B01111100) ) {
    UpdatedPortD = PORTD & B01111100;
  }

  // For T1= G1`.(G2`+BT`) (Simplified LOGIC EQUATION from Truth Table)
  if ( (~UpdatedPortD & (1 << 4))  && ((~UpdatedPortD & (1 << 5)) || (~UpdatedPortD & (1 << 6))) ) {
    PORTB |= 1;
  }
  else if (PORTB & 1) { // if not reset the pin.
    PORTB ^= 1;
  }
  // For T2= G2`.(G1`+BT`)
  if ( (~UpdatedPortD & (1 << 5)) && ((~UpdatedPortD & (1 << 4)) || (~UpdatedPortD & (1 << 6))) ) {
    PORTB |= 1 << 1;
  }
  else if (PORTB & 1 << 1) {
    PORTB ^= 1 << 1;
  }
  // For G1= T1`.(G2`.T2` + BT`)
  if ( (~UpdatedPortD & (1 << 2)) && ((~UpdatedPortD & (1 << 5)) && (~UpdatedPortD & (1 << 3)) || (~UpdatedPortD & (1 << 6))) ) {
    PORTB |= 1 << 2;
  }
  else if (PORTB & 1 << 2) {
    PORTB ^= 1 << 2;
  }
  // For G2= T2`.(G1`.T1` + BT`)
  if ( (~UpdatedPortD & (1 << 3)) && ((~UpdatedPortD & (1 << 4)) && (~UpdatedPortD & (1 << 2)) || (~UpdatedPortD & (1 << 6))) ) {
    PORTB |= 1 << 3;
  }
  else if (PORTB & 1 << 3) {
    PORTB ^= 1 << 3;
  }
  // For BT= G1`.G2`+ T1`.T2`.(G1`+G2`)
  if ( ((~UpdatedPortD & (1 << 4)) && (~UpdatedPortD & (1 << 5))) || (((~UpdatedPortD & (1 << 2)) && (~UpdatedPortD & (1 << 3))) && (~UpdatedPortD & (1 << 4) || ~UpdatedPortD & (1 << 5)))) {
    PORTB |= 1 << 4;
  }
  else if (PORTB & 1 << 4) {
    PORTB ^= 1 << 4;
  }
  lastReading = PORTD & B01111100;
}

Thanks very much!

\$\endgroup\$
8
  • 1
    \$\begingroup\$ The LED series resistor is plain wrong. This will limit total current to 50mA. Meaning if several LEDs are lit at once, they will be less bright. Also, most LEDs on the market are rated at 20mA so you might end up damaging them. Also I very much doubt that a single pin on Arduino can source 50mA so you'll fry the MCU as well. And you use up the battery needlessly. Typically you only need like 5mA for such LEDs too, so ~ 1k ohm per LED will likely do fine. \$\endgroup\$
    – Lundin
    Dec 13 '19 at 9:57
  • 1
    \$\begingroup\$ Just for future reference, hardware reviews of schematics can be posted at electronics.stackexchange.com. \$\endgroup\$
    – Lundin
    Dec 13 '19 at 10:00
  • \$\begingroup\$ You have a good answer now. Is the code working as expected, because if it isn't this question is off-topic for code review? \$\endgroup\$
    – pacmaninbw
    Dec 13 '19 at 13:45
  • \$\begingroup\$ Mr. @pacmaninbw, firstly, thanks for your reply. Well, it's not off topic question. I'm asking for a review of my code to figure out the problem in it. All other details and the circuit attached is not important at all and just attached for the reviewer to conceptualize the symbols used( T1 to pin 3 and so on) if this matters to them. \$\endgroup\$
    – A. Ali
    Dec 13 '19 at 22:45
  • \$\begingroup\$ Mr. @Lundin Thanks very much for your helpful reply. I'll make sure I follow your advice. \$\endgroup\$
    – A. Ali
    Dec 13 '19 at 22:52
3
\$\begingroup\$
  • For embedded systems, don't use the native integer types of C, nor weird custom types like byte. Always use the types from stdint.h.
  • Avoid declaring variables at file scope. If you must, because they are shared between several functions in the same file, then declare them as static to narrow down the scope.
  • Never use binary constants 0b because they are non-standard and non-portable. In addition, they are hard to read. Programmers are assumed to be able to read hex.
  • Similarly, don't mask with decimal numbers like 31 either, that's even more confusing. 0x1F is much clearer.
  • Don't use "magic numbers" at all. Instead of code like DDRD = DDRD & B10000011; you should have something like DDRD = DDRD0 | DDRD1 | DDRD7. This is now self-documenting code, so the comment "Masking to perserve bits 0,1 and 7" is no longer necessary.
  • Hardware peripheral registers are not to be regarded as normal variables! When accessing hardware registers, avoid accessing them several times needlessly, since that can cause unexpected order of evaluation bugs or extra needless read accesses. Or in worse case, spurious writes that toggle the port very quickly before writing the final value - such code creates EMI and hardware glitches.

    The void loop() should be rewritten as:

    uint8_t portb = PORTB;
    uint8_t portd = PORTD;
    
    /* all if statements and arithmetic uses the above 2 RAM variables */
    
    PORTB = portb; // write to the port one single time, when done
    
  • There's no award for most operators on a single line, quite the contrary. Expressions like if ( ((~UpdatedPortD & (1 << 4)) && (~UpdatedPortD & (1 << 5))) || (((~UpdatedPortD & (1 << 2)) && (~UpdatedPortD & (1 << 3))) && (~UpdatedPortD & (1 << 4) || ~UpdatedPortD & (1 << 5)))) need to be split in several expressions on several lines. It is important to realize that doing so does not lead to slower code.

    Also, instead of having your program work on "magic port numbers", you can use meaningful variable names. Just an example with random names, since I don't know what your actual buttons are doing:

    bool button_up    = ~UpdatedPortD & (1u<<2);
    bool button_down  = ~UpdatedPortD & (1u<<3);
    bool button_left  = ~UpdatedPortD & (1u<<4);
    bool button_right = ~UpdatedPortD & (1u<<5);
    
    if(button_up && button_left)
    { 
      /* do stuff */
    }
    
  • Your code has the usual embedded systems problems with implicit integer type promotion, which is always a ticking bomb waiting to explode. For example ~UpdatedPortD gives you a 16 bit signed int with value 0xFF** (negative value), which is not what you want, ever. 1 << 4; perform bit shifts on a 16 bit signed int. And so on. Never use bitwise operations on signed types!

    To solve this, always cast back promoted expressions to the intended type and always u suffix integer constants where it matters. 1 << 4 should be 1u << 4.

    Overall, legacy 8 bit MCUs are very hard to program and implicit promotions is one reason. I strongly recommend beginners to use 32 bit ARMs instead, since they are much more straight-forward.

  • 50ms is a very long de-bounce time, to the point where humans might start to notice the lag (we might start to notice latency from somewhere around 100ms and beyond). Most switches don't need nearly that long, 10ms is sufficient for most buttons like tactile switches etc. If in doubt, hook up the button to an oscilloscope, feed it 5V and watch the bounce.
\$\endgroup\$
2
  • \$\begingroup\$ Mr. @Lundin I really can't thank you enough for such a detailed and immensely helpful contribution. I did all what you recommended. Unfortunately, The code still doesn't simulate (but it compiles without errors or warnings). I'll try another simulaution software with another board model. As a final resort, I'll use the high level standard functions (pinMode(),...etc) along with other high level oop traditions, which I think would solve the problem. Thanks again! \$\endgroup\$
    – A. Ali
    Dec 13 '19 at 22:40
  • \$\begingroup\$ While I agree hex should be preferred over binary in most cases, binary still has its place. Just not here. Very good points raised overall. \$\endgroup\$
    – Mast
    Dec 16 '19 at 5:29

Not the answer you're looking for? Browse other questions tagged or ask your own question.