Skip to content

The pinMode(pin,OUTPUT) can't initialize the pin in HIGH state on a Nano Every #143

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

Open
drf5n opened this issue Mar 19, 2025 · 10 comments
Open

Comments

@drf5n
Copy link

drf5n commented Mar 19, 2025

void pinMode(uint8_t pin, PinMode mode)
{
uint8_t bit_mask = digitalPinToBitMask(pin);
if ((bit_mask == NOT_A_PIN) || (mode > INPUT_PULLUP) || isDoubleBondedActive(pin)) return;
PORT_t* port = digitalPinToPortStruct(pin);
if(port == NULL) return;
if(mode == OUTPUT){
/* Configure direction as output */
port->DIRSET = bit_mask;
} else { /* mode == INPUT or INPUT_PULLUP */

On an Uno, you can use these lines to switch directly from INPUT_PULLUP to HIGH:

digitalWrite(pin,HIGH); // enable pullup
pinMode(pin,OUTPUT); // switch to OUTPUT HIGH

On a Nano Every, those commands switch instead to an OUTPUT LOW

See the discussion at:

https://forum.arduino.cc/t/how-to-set-output-high-without-pulse/1364909

The documentation at https://docs.arduino.cc/learn/microcontrollers/digital-pins/ says it should switch directly to HIGH:

Consequently, a pin that is configured to have pullup resistors turned on when the pin is an INPUT, will have the pin configured as HIGH if the pin is then switched to an OUTPUT with pinMode().

Per this chunk of code, it looks like half of the issue was considered:

/* Input direction */
} else {
/* Old implementation has side effect when pin set as input -
pull up is enabled if this function is called.
Should we purposely implement this side effect?
*/
/* Get bit position for getting pin ctrl reg */
uint8_t bit_pos = digitalPinToBitPosition(pin);
/* Calculate where pin control register is */
volatile uint8_t* pin_ctrl_reg = getPINnCTRLregister(port, bit_pos);
/* Save system status and disable interrupts */
uint8_t status = SREG;
cli();
if(val == LOW){
/* Disable pullup */
*pin_ctrl_reg &= ~PORT_PULLUPEN_bm;
} else {
/* Enable pull-up */
*pin_ctrl_reg |= PORT_PULLUPEN_bm;
}
/* Restore system status */
SREG = status;
}
}

But the pinMode code is where the problem happens. Instead of always assuming LOW, it should use the state on the pullup to set the level before enabling the OUTPUT direction in order to match the documentation and the behavior on the '328 and '2560.

See also #86

@drf5n
Copy link
Author

drf5n commented Mar 19, 2025

And issue #132 and its PR #133

@stitech195
Copy link

stitech195 commented Mar 19, 2025

I do not agree. There is no need to match the behaviour of the 4809 to the behaviour of the 328 unless in emulation mode.
Matching denies the own identity of the 4809 and will introduce unexpected behaviour, confusing the user.

The digitalWrite function might become something like this:

  void digitalWrite(uint8_t pin, PinStatus val) {
    /* Get bit mask for pin */
    uint8_t bit_mask = digitalPinToBitMask(pin);
    if (bit_mask == NOT_A_PIN || isDoubleBondedActive(pin)) return;

    /* Turn off PWM if applicable */

    // If the pin supports PWM output, we need to turn it off
    // before doing a digital write.
    turnOffPWM(pin);

    /* Get port */
    PORT_t *port = digitalPinToPortStruct(pin);

    /* Set output to value */
    if (val == LOW) { /* If LOW */
      port->OUTCLR = bit_mask;

    } else if (val == CHANGE) { /* If TOGGLE */
      port->OUTTGL = bit_mask;
      /* If HIGH OR  > TOGGLE  */
    } else {
      port->OUTSET = bit_mask;
    }

    /* Emulation mode */
#ifdef AVR_NANO_4809_328MODE  // need to check this formulation!!
    /* Old implementation has side effect when pin set as input -
		pull up is enabled if this function is called.
		*/
    if (!(port->DIR & bit_mask)) {  // is pin input?
      /* Get bit position for getting pin ctrl reg */
      uint8_t bit_pos = digitalPinToBitPosition(pin);

      /* Calculate where pin control register is */
      volatile uint8_t *pin_ctrl_reg = getPINnCTRLregister(port, bit_pos);

      /* Save system status and disable interrupts */
      uint8_t status = SREG;
      cli();

      if (val == LOW) {
        /* Disable pullup */
        *pin_ctrl_reg &= ~PORT_PULLUPEN_bm;

      } else {
        /* Enable pull-up */
        *pin_ctrl_reg |= PORT_PULLUPEN_bm;
      }

      /* Restore system status */
      SREG = status;
    }
#endif
  }

And the pinMode function:

void pinMode(uint8_t pin, PinMode mode) {
  uint8_t bit_mask = digitalPinToBitMask(pin);

  if ((bit_mask == NOT_A_PIN) || (mode > INPUT_PULLUP) || isDoubleBondedActive(pin)) return;

  PORT_t *port = digitalPinToPortStruct(pin);
  if (port == NULL) return;

  if (mode == OUTPUT) {

    /* Configure direction as output */
    port->DIRSET = bit_mask;

  } else {                    /* mode == INPUT or INPUT_PULLUP */
#ifdef AVR_NANO_4809_328MODE  // need to check this formulation!!
    if (mode == INPUT_PULLUP) digitalWrite(pin, HIGH);
    else digitalWrite(pin, LOW);  //
#else
    uint8_t bit_pos = digitalPinToBitPosition(pin);
    /* Calculate where pin control register is */
    volatile uint8_t *pin_ctrl_reg = getPINnCTRLregister(port, bit_pos);

    /* Save state */
    uint8_t status = SREG;
    cli();

    /* Configure direction as input */
    port->DIRCLR = bit_mask;

    /* Configure pull-up resistor */
    if (mode == INPUT_PULLUP) {

      /* Enable pull-up */
      *pin_ctrl_reg |= PORT_PULLUPEN_bm;

    } else { /* mode == INPUT (no pullup) */

      /* Disable pull-up */
      *pin_ctrl_reg &= ~(PORT_PULLUPEN_bm);
    }

    /* Restore state */
    SREG = status;
  }
#endif
  }

@drf5n
Copy link
Author

drf5n commented Mar 19, 2025

I mean that the behavior should match the API as documented at

https://docs.arduino.cc/learn/microcontrollers/digital-pins/

It does make mention of the processor differences, but only as differences in pull-up resistor values.

This code shouldn't result in opposite outcomes on different Arduinos:

// See https://docs.arduino.cc/learn/microcontrollers/digital-pins/
void setup() {
  const int pin = 2;
  digitalWrite(pin, HIGH); // enable pullup
  pinMode(pin, OUTPUT); // switch to OUTPUT
  Serial.begin(115200);
  Serial.println(digitalRead(pin) == HIGH ? "HIGH" : "LOW" );
  digitalWrite(pin, HIGH); 
  Serial.println(digitalRead(pin) == HIGH ? "HIGH" : "LOW" );
}
void loop() {}

@cattledogGH
Copy link

There are now 9 official Arduino boards in the Nano Family. Should they all need to emulate the pinMode behaviour of the original Nano?

I agree with @stitech195 that only in the 328 emulation mode should the Nano Every have that behaviour.

@stitech195
Copy link

Another complication: when in emulation mode, and the sketch contains direct memory access to the ports, the names of the ports as known in the 328 are translated to the names in the 4809. Are these side effects also mimiced? Should they?

@drf5n
Copy link
Author

drf5n commented Mar 19, 2025

There are now 9 official Arduino boards in the Nano Family. Should they all need to emulate the pinMode behaviour of the original Nano?

I agree with @stitech195 that only in the 328 emulation mode should the Nano Every have that behaviour.

In that case, the fix is to change the erroneous API documentation to document it:

https://docs.arduino.cc/learn/microcontrollers/digital-pins/

@drf5n
Copy link
Author

drf5n commented Mar 19, 2025

Wait -- the API behavior should have absolutely nothing to do with the "registers emulation" mode:

Image

The custom coding for a processor with different registers should make the hardware conform to the API.

Are you really saying that if you tweak the "Registers emulation" setting it should swap the behavior of this code?

// See https://docs.arduino.cc/learn/microcontrollers/digital-pins/
void setup() {
  const int pin = 2;
  digitalWrite(pin, HIGH);
  pinMode(pin, OUTPUT);  Serial.begin(115200);
  Serial.println(digitalRead(pin) == HIGH ? "HIGH" : "LOW" );
  digitalWrite(pin, HIGH); 
  Serial.println(digitalRead(pin) == HIGH ? "HIGH" : "LOW" );
}
void loop() {}

@stitech195
Copy link

stitech195 commented Mar 19, 2025

The emulation mode is introduced with the UNO WiFi Rev2, containing the ATmega4809. The objective was that existing sketches, written for the normal UNO could run without any changes. This logically means that all manners that the new processor is better (more RAM, more timers, more USARTs etc) will not be used. And even then, the emulation can't be complete (UNO Wifi Rev2 and Nano Every provide PWM at only 5 pins, not 6, etc).
I never used this feature. It only caused me problems.
In your code snippet the pin got the pull up resistor switched on. After switching to Output the pin was low. This happened independent of the emulation mode as it is now.

@drf5n
Copy link
Author

drf5n commented Mar 20, 2025

The objective was that existing sketches, written for the normal UNO could run without any changes. This logically means that all manners that the new processor is better (more RAM, more timers, more USARTs etc) will not be used. And even then, the emulation can't be complete (UNO Wifi Rev2 and Nano Every provide PWM at only 5 pins, not 6, etc).
I never used this feature. It only caused me problems.
In your code snippet the pin got the pull up resistor switched on. After switching to Output the pin was low. This happened independent of the emulation mode as it is now.

I think it means that code written for the normal Uno that explicitly references the 328 registers would work unmodified, but it does not mean that it limits the normal API code (non direct register manipulation code).

For instance the digitalWrite() statement to be affected by changes in the register emulation mode, it might need #ifndef(PORTB) or #ifndef(AVR_NANO_4809_328MODE) ... conditional macros and separate code paths to actually reference the registers. As is, the digitalWrite and rest of the API-implementing code completely ignores the extra macros and code defined in https://github.com/arduino/ArduinoCore-megaavr/blob/5e639ee40afa693354d3d056ba7fb795a8948c11/cores/arduino/NANO_compat.h and https://github.com/arduino/ArduinoCore-megaavr/blob/5e639ee40afa693354d3d056ba7fb795a8948c11/cores/arduino/NANO_compat.cpp and still provides the full featurea of more RAM, timers, USARTS, etc... On the Nano Every, digitalWrite() in "registers Emulation" and non-"registers Emulation" mode runs the exact same code and does not access the '328 port manipulation macros.

@WestfW
Copy link
Contributor

WestfW commented Mar 20, 2025

Should they all need to emulate the pinMode behaviour of the original Nano?

Not necessarily, but there does need to be a way to initialize the pin at HIGH level.
(OTOH, several cores DO implement this sort of backward compatibility, sometimes at great cost.)

Currently on 4809, the digitalWrite(P, HIGH) turns on the input pullup if the pin is in input mode, but does NOT set the port output register. So when you change the output mode with pinMode(), the output register bit is still 0, and the pin will be set to zero as well.
While having the digitalWrite() switch to input pullup mode is questionable (and "only for backward compatibility), I can't think of any arguments for it NOT setting the output port register.

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

No branches or pull requests

4 participants