Go Down

Topic: Code in wiring_digital.c (Read 1 time) previous topic - next topic

kewakl

Two functions in wiring_digital.c seem to NOT follow best practices procedures for coding.

Quote
Line 41:
   if (mode == INPUT) {
      uint8_t oldSREG = SREG;
                cli();
      *reg &= ~bit;
      *out &= ~bit;
      SREG = oldSREG;
   } else if (mode == INPUT_PULLUP) {
      uint8_t oldSREG = SREG;
                cli();
      *reg &= ~bit;
      *out |= bit;
      SREG = oldSREG;
   } else {                                          <----- WHERE IS else if (mode == OUTPUT)
      uint8_t oldSREG = SREG;
                cli();
      *reg |= bit;
      SREG = oldSREG;
   }
and

Quote
Line 138:
void digitalWrite(uint8_t pin, uint8_t val)
{
   uint8_t timer = digitalPinToTimer(pin);
   uint8_t bit = digitalPinToBitMask(pin);
   uint8_t port = digitalPinToPort(pin);
   volatile uint8_t *out;

   if (port == NOT_A_PIN) return;

   // If the pin that support PWM output, we need to turn it off
   // before doing a digital write.
   if (timer != NOT_ON_TIMER) turnOffPWM(timer);

   out = portOutputRegister(port);

   uint8_t oldSREG = SREG;
   cli();

   if (val == LOW) {
      *out &= ~bit;
   } else {                                          <----- WHERE IS else if (val == HIGH)
      *out |= bit;
   }

   SREG = oldSREG;
}
The header file Arduino.h defines all the params, why have they not been used?
I have checked  arduino 0022, 1.0.5r2, 1.6.12 and 1.8.1 sources. All have this deficiency.
Quote
Line 40:
#define HIGH 0x1
#define LOW  0x0

#define INPUT 0x0
#define OUTPUT 0x1
#define INPUT_PULLUP 0x2

westfw

"Robustness principle."

The LOW/HIGH values are essentially booleans, 0 means LOW, anything else behaves as HIGH.  Yeah, you could add a cast and an extra operation, but it's not really needed.

OUTPUT/INPUT is harder to justify, but follows the same logic.

What would you have it do with "undefined" values?  It's not like there is any way to signal an error...

kewakl

#2
Jan 27, 2017, 02:23 pm Last Edit: Jan 27, 2017, 04:12 pm by kewakl
Quote
What would you have it do with "undefined" values?  It's not like there is any way to signal an error...
But, Hmmm someone bothered to define them.....
Ignore the function call maybe.

C\C++ is so demanding in some respects, and this library just leaves pinMode OUTPUT and digitalWrite HIGH flapping in the breeze.

pert

Blink with the stock wiring_digital.c compiled for Uno with Arduino AVR Boards 1.6.17 is 928 bytes.
Blink with your suggested changes to wiring_digital.c compiled for Uno with Arduino AVR Boards 1.6.17 is 934 bytes.

This is not only a matter of saving memory, those extra bytes represent extra clock cycles. The slowness of digitalWrite() is already a very common complaint, now you want to make it even slower for what benefit?

You need to understand the limitations of the microcontrollers. This is not a computer running at gigahertz with gigabytes of RAM. Optimization in this case is "best practices procedures".

But, Hmmm someone bothered to define them.....
Of course they defined them those definitions are used in practically every sketch.

westfw

Quote
someone bothered to define them
They're readability aids for the target audience.

kewakl

#5
Jan 28, 2017, 04:46 pm Last Edit: Jan 28, 2017, 05:33 pm by kewakl
Hey, guys, thanks for the food for MY thoughts.
I appreciate your input!

My background is more PLC/Ladder than C/C++, so the unaccounted-for OUTPUT and HIGH seemed a glaring omission. Thanks again.

bperrybap

The LOW/HIGH values are essentially booleans, 0 means LOW, anything else behaves as HIGH.  Yeah, you could add a cast and an extra operation, but it's not really needed.
Worse, if you check for HIGH, there is TONs of code out there that would break if digitalWrite() does not make the output high for any value other than LOW.

There are many many sketches that cheat and abuse the API by making the assumption that any value not LOW is presumed to mean HIGH.

There are also lots of sketches out there that assume LOW is 0 and HIGH is 1.
That is also an abuse of the API as that is simply not a given based on the API definition/documenation.

Here is an example of abusing the API:
Code: [Select]
digitalWrite(pin, (datavalue & mask));

to be safe the code should have used something like:
Code: [Select]
digitalWrite(pin, (datavalue & mask) ? HIGH : LOW);

I've pointed this out in many threads over the years.

In fact, in some of those, I also point out that had people properly used HIGH and LOW.
digitaWrite() could have supported pwm as well as stable signals.
i.e it could have defined LOW as 0 and HIGH as 255 to mean stable low and high signals.
Then any value in between LOW and HIGH could be a pwm output.
This would not violate the existing digitalWrite() API but would break lots of existing code that has abused the API by making certain implementation assumptions.

--- bill

Go Up