-4

complete beginner here, I was wondering if it's possible to simplify this code, to thin it down or shorten it up.

As you can easily tell, this thing makes one led turn on when the other is off and viceversa, in a loop.

void setup()
{
  pinMode(3, OUTPUT);
  pinMode(13, OUTPUT);
}

void loop()
{

  digitalWrite(13, HIGH);

  if (digitalRead(13) == HIGH)
    digitalWrite(3, LOW);

  delay(1000); // Wait for 1000 millisecond(s)

  digitalWrite(13, LOW);

  if (digitalRead(13) == LOW)
    digitalWrite(3, HIGH);

  delay(1000); // Wait for 1000 millisecond(s)

}
Gabriel Staples
  • 36,492
  • 15
  • 194
  • 265
Davide Santonocito
  • 3
  • 4
  • 1
    It needs to be formatted. That's about it. It's blocking, it could be made non-blocking by using the "Blink Without Delay" example as a guide. You really don't need the if statements because you know what you just wrote to the pins. But they're not causing any issue, the optimizer probably takes them out anyway. – Delta_G May 14 '20 at 22:26
  • thanks a lot for your comment. Unfortunately my limited knowledge made it hard to understand your comment – Davide Santonocito May 14 '20 at 23:06
  • 1
    Yes, but there sure are a lot of great words to google and learn about. Give it a try. – Delta_G May 15 '20 at 00:22
  • Or you could ask a specific question of some sort. Without knowing what you think is broken or what exactly you don't understand I don't know what to explain. I can't give you a full programming course in the comments section of a web forum. It's just not possible. – Delta_G May 15 '20 at 00:23
  • 1
    Code review is off-topic here. Maybe try [Code Review SE](https://codereview.stackexchange.com/). – gre_gor May 15 '20 at 02:31
  • @DavideSantonocito, if you move (repost) this question over at the [Code Review Stack Exchange](https://codereview.stackexchange.com/), post a link to it here and I'll just move my answer over there too. This is a useful question to help beginner Arduino users, as well as beginner C or C++ developers. – Gabriel Staples May 15 '20 at 08:00

2 Answers2

1

Here's what I see:

  1. Use constant expressions (constexpr) to set the pin numbers for 3 and 13 to constant 8-bit variables, rather than writing 3 and 13 everywhere, as that's hard to maintain and hard to know what those numbers mean.
  2. Since the LED states are opposites, carry their state in a single variable, led_state, which you toggle. No need to write the state, then read the state. In this case, that doesn't make any sense.
  3. Remove the duplication of having two delay calls. You can simplify this to one.
  4. Lastly, don't use delay() to handle this, it is a wasteful call that uses literally 100% of the CPU just to delay. But, let's save that change for another day, as this is enough to absorb for now. To see how to do this without the blocking/wasteful delay(), study this code:
    1. (beginner-friendly): https://www.arduino.cc/en/tutorial/BlinkWithoutDelay
    2. and then this code (much more advanced--timestamp-based cooperative multi-tasking): Best way to read from a sensors that doesn't have interrupt pin and require some time before the measure is ready

Here's the code with my changes (although still using delay()):

constexpr uint8_t PIN_LED1 = 3;
constexpr uint8_t PIN_LED2 = 13;

void setup()
{
  pinMode(PIN_LED1, OUTPUT);
  pinMode(PIN_LED2, OUTPUT);
}

void loop()
{
  static bool led_state = LOW;

  digitalWrite(PIN_LED1, led_state);
  digitalWrite(PIN_LED2, !led_state);

  led_state = !led_state; // toggle the state
  delay(1000);
}

Additional references:

  1. constexpr: https://en.cppreference.com/w/cpp/language/constexpr
  2. uint8_t means "unsigned 8-bit type". Unsigned means "only positive values", so this can store numbers 0 to 255 in it. See here: http://www.cplusplus.com/reference/cstdint/. It comes from the stdint.h header file. byte in Arduino speak is an alias to this. https://www.arduino.cc/reference/en/language/variables/data-types/byte/
  3. static is a keyword which has many meanings, but here I'm using it in the simple C sense which means it gets initialized with what I set it to ONCE--only the first time, and then keeps its last-known value each new loop cycle, instead of getting reset. Read about it here: What does "static" mean in C?
Gabriel Staples
  • 36,492
  • 15
  • 194
  • 265
0
digitalWrite(13, HIGH);

  if (digitalRead(13) == HIGH)
    digitalWrite(3, LOW);

After setting pin 13 high, there is no need to read its value.

digitalWrite(13, HIGH);
digitalWrite(3, LOW);

is sufficient.

Other than that you don't need to write beautyful code as a beginner. Focus on understanding what is going on and how to achieve what. Beautiful things are usually created by masters, not apprentices.

Piglet
  • 27,501
  • 3
  • 20
  • 43