Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding digitalToggle to core? #130

Open
RobTillaart opened this issue Dec 5, 2020 · 6 comments
Open

Adding digitalToggle to core? #130

RobTillaart opened this issue Dec 5, 2020 · 6 comments

Comments

@RobTillaart
Copy link

RobTillaart commented Dec 5, 2020

Description

The Arduino core has a digitalWrite() and a digitalRead() function.
In many sketches there is a need to toggle a pin, either for a blinking LED or a clock pin for swSPI etc.

There are two typical ways to invert a pin - see code below

  // using a state holding the value of the pin
  digitalWrite(pin, state);
  state = 1 - state;

  // read pin, invert and write back
  digitalWrite(pin, !digitalRead(pin));

The latter one is slower as it redo a lot of "pin math", so I implemented a version of digitalToggle() for AVR.
Attached test sketch shows the gain compared to the two methods.

Results test on UNO

Time us		1000 calls
Reference:	7392	// read invert write
      Var:	4784	// use state var
   Toggle:	3964	// use digitalToggle returning state
   Toggle:	3520	// use digitalToggle returning NO state

The gain of toggle returning state is 46% resp 16%
The gain of toggle returning NO state is 52% resp 26%

Imho these gains are interesting, esp for clocking data

Implementation for AVR

Arduino.h

uint8_t digitalToggle(uint8_t pin);     // returns the new state of the pin

wiring_digital.c

uint8_t digitalToggle(uint8_t pin)
{
  uint8_t port = digitalPinToPort(pin);
  volatile uint8_t *out;

  if (port == NOT_A_PIN) return 0;
  uint8_t timer = digitalPinToTimer(pin);
  if (timer != NOT_ON_TIMER) turnOffPWM(timer);

  uint8_t bit = digitalPinToBitMask(pin);

  out = portOutputRegister(port);
  uint8_t oldSREG = SREG;
  cli();
  *out ^= bit;      // invert bit
  SREG = oldSREG;
  return ((*out & bit) != 0);
}

Note: a no state returning version is straightforward given the above code.

Test sketch

digitalToggle.zip

uint32_t start, Tref, Tref2, Tnew;
const uint8_t pin = 13;

uint8_t state = LOW;

void setup()
{
  Serial.begin(115200);
  Serial.println();
  Serial.println(__FILE__);

  pinMode(pin, OUTPUT);
  digitalWrite(pin, LOW);

  start = micros();
  for (int i = 0; i < 1000; i++) digitalWrite(pin, !digitalRead(pin));
  Tref = micros() - start;

  start = micros();
  for (int i = 0; i < 1000; i++)
  {
    digitalWrite(pin, state);
    state = 1 - state;
  }
  Tref2 = micros() - start;

  start = micros();
  for (int i = 0; i < 1000; i++) digitalToggle(pin);
  Tnew = micros() - start;

  Serial.print("Reference:\t");
  Serial.println(Tref);
  Serial.print("      Var:\t");
  Serial.println(Tref2);
  Serial.print("   Toggle:\t");
  Serial.println(Tnew);
  Serial.print("     Gain:\t");
  Serial.println(Tref - Tnew);
  Serial.print("     Perc:\t");
  Serial.println(100.0 - (100.0 * Tnew) / Tref, 1);

  pinMode(13, OUTPUT);
}

void loop()
{
  static int cnt = 0;
  if (cnt == 60)
  {
    cnt = 0;
    Serial.println();
  }
  cnt++;
  // digitalToggle(pin);
  int x = digitalToggle(pin);
  Serial.print(x);

  delay(1000);
}

Additional context

Additional requests

@matthijskooijman
Copy link
Collaborator

Sounds like a good additoin. I think it belongs to the ArduinoCore-API repo, though, so I'm moving it there.

As for AVR, IIRC you can also toggle a pin by writing a 1 to the PINx register, which would be even (slightly) faster.

@matthijskooijman matthijskooijman transferred this issue from arduino/Arduino Dec 5, 2020
@RobTillaart
Copy link
Author

RobTillaart commented Dec 5, 2020

Implementation for ESP32

For ESP32 platform I have not yet tested code.
No gain expected here, although there is one (1 << pin) and one if () less on average

esp32-hal-gpio.h

int digitalToggle(uint8_t pin);

esp32-hal-gpio.c

extern int IRAM_ATTR __digitalToggle(uint8_t pin)
{
  uint32_t val = 0;
  uint32_t mask = 0x01;
  
  if (pin < 32) 
  {
    mask = (0x01 << pin);
    val =  (GPIO.in & mask) != 0;
    if (val) GPIO.out_w1tc = ((uint32_t) mask);
    else     GPIO.out_w1ts = ((uint32_t) mask);
    return (val == 0);
  } 
  if( pin < 34)
  {
    mask = (0x01 << (pin - 32));
    val = (GPIO.in1.val & mask) != 0;
    if (val) GPIO.out1_w1tc.val = ((uint32_t) mask);
    else     GPIO.out1_w1ts.val = ((uint32_t) mask);
    return (val == 0);
  }
  return 0;
}

extern int digitalToggle(uint8_t pin) __attribute__ ((weak, alias("__digitalToggle")));

@edgar-bonet
Copy link
Contributor

Nice idea. Here is a small optimization opportunity:

  return ((*out & bit) != 0);

The compiler should issue a “load” instruction for *out, in order to read the port register, although its contents is known, as it has just been written. You can save this memory access by explicitly caching that contents:

  uint8_t state = *out;
  state ^= bit;      // invert bit
  *out = state;
  SREG = oldSREG;
  return ((state & bit) != 0);

I would expect this to save two CPU cycles, although I did not test. The compiler cannot perform this optimization itself because the volatile keyword explicitly forbids it.

@Perehama
Copy link

That was my first contribution here on github, as I just had the same idea others have had. I came here from the Arduino forums. My only contribution is to make it a void return so it operates otherwise like digitalWrite(). There is an AVR specific version that would write a 1 to the portInputRegister(port) as per Atmel, writing a 1 to PINx will toggle that pin, but the XOR operation is portable to other microcontrollers.

void digitalToggle(uint8_t pin) {
  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 (timer != NOT_ON_TIMER) turnOffPWM(timer);
  out = portOutputRegister(port);
  uint8_t oldSREG = SREG;
  cli();
  *out ^= bit;
  SREG = oldSREG;
}

@jacoblgit
Copy link

Implementation for SAMD

SAMD has a PORT register OUTTGL for single instruction toggling of an I/O pin, enabling significant performance gain over the read/modify/write approach.

Common.h

void digitalToggle(pin_size_t pinNumber);

wiring_digital.c

void digitalToggle(pin_size_t ulPin)
{
  // Handle the case the pin isn't usable as PIO
  if (g_APinDescription[ulPin].ulPinType == PIO_NOT_A_PIN)
  {
    return;
  }

  EPortType port = g_APinDescription[ulPin].ulPort;
  uint32_t pin = g_APinDescription[ulPin].ulPin;
  uint32_t pinMask = (1ul << pin);

  PORT->Group[port].OUTTGL.reg = pinMask;
  return;
}

@RobTillaart
Copy link
Author

@jacoblgit

Recently put some "missing" functions into a OUTPIN class, only the AVR optimized so far.
Besides the digitalToggle() it supports pulseHigh(), pulseLow() for short pulses and pulseOut() for longer pulses.

PulseOut was implemented to complement the existing PulseIn().
From there it was a small step to the dedicated pulseHigh() and pulseLow() finctions.

It might be an idea to extend that class with SAMD optimized versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants