ArduinoCore-samd icon indicating copy to clipboard operation
ArduinoCore-samd copied to clipboard

SAMD outputs do not provide high drive current

Open facchinm opened this issue 8 years ago • 20 comments

Moved from https://github.com/arduino/Arduino/issues/5258 Originally reported by @MLXXXp

The documentation for the Zero and MKR1000 both state: DC Current per I/O Pin 7 mA

However, according to the ATSAMD21G18 datasheet, this current is only available if the Output Driver Strength bit (DRVSTR) for the pin has been set to 1. Otherwise, the pin can only source a maximum of 2 mA or sink 2.5 mA. I can't find anywhere in the Arduino source code that sets the DRVSTR bit for a pin. Even if it is being set somewhere that I missed, the line PORT->Group[g_APinDescription[ulPin].ulPort].PINCFG[g_APinDescription[ulPin].ulPin].reg=(uint8_t)(PORT_PINCFG_PULLEN) ; which is always executed in function digitalWrite() in file wiring_digital.c will clear DRVSTR.

Therefore, at best, the documentation is misleading.

Ways I see to address the problem:

  • Change the documentation to state that only 2 mA is available unless the user writes code to manipulate the I/O output pins directly to get 7 mA, bypassing the Arduino digital pin functions. This would probably be difficult at this point. Even SparkFun and Adafruit state the higher current for their ATSAMD21 products.
  • Change the code to always set outputs to high drive mode. Since in many cases high current isn't required, this could unnecessarily increase the chances of causing hardware damage due to shorted outputs.
  • Add a new mode to the pinMode() function, such as OUTPUT_HIGH_CURRENT, to be used instead of OUTPUT when high drive capability is desired. In addition to changing the pinMode() function, the digitalWrite() function would also have to be modified to not change DRVSTR, as was described above.

And just a side note: That same line quoted above that sets PULLEN, also clears INEN (in addition to DRVSTR), nullifying INEN having been set in pinMode(x, OUTPUT). Therefore, reading the IN register will not provide the state of the output pin after a digitalWrite().

facchinm avatar Aug 22 '16 08:08 facchinm

That same line quoted above that sets PULLEN, also clears INEN (in addition to DRVSTR), nullifying INEN having been set in pinMode(x, OUTPUT). Therefore, reading the IN register will not provide the state of the output pin after a digitalWrite().

This portion has already been addressed in #101, and will be included in the next SAMD core release.

sandeepmistry avatar Aug 22 '16 17:08 sandeepmistry

Using Nordic's NRF5X platform depends also on a high drive configuration. Nordic has more modes than SAMD platform.

GPIO_PIN_CNF_DRIVE_S0S1 Standard '0', Standard '1'. GPIO_PIN_CNF_DRIVE_H0S1 High '0', Standard '1'. GPIO_PIN_CNF_DRIVE_S0H1 Standard '0', High '1'. GPIO_PIN_CNF_DRIVE_H0H1 High '0', High '1'. GPIO_PIN_CNF_DRIVE_D0S1 Disconnected '0', Standard '1'. GPIO_PIN_CNF_DRIVE_D0H1 Disconnected '0', High '1'. GPIO_PIN_CNF_DRIVE_S0D1 Standard '0', Disconnected '1'. GPIO_PIN_CNF_DRIVE_H0D1 High '0', Disconnected '1'.

My idea is to make all modes available via OUTPUT_S0S1 .. OUTPUT_H0D1 definition. For AVR any S/H definition can mapped to OUTPUT.

For SAMD any combination including H is configured as high drive mode. Any Disconnected configuration should fail, when unsupported.

d00616 avatar Oct 18 '16 15:10 d00616

@d00616 What do you mean by "fail" a disconnected configuration when unsupported? Generate a compile error? Set low drive (or the default or whatever using just OUTPUT does)?

In any case, if a configuration can "fail", then for SAMD I think we should fail all x0x1 configurations and only accept OUTPUT_HIGH_CURRENT (plus OUTPUT of course). Maybe OUTPUT_HIGH_DRIVE is a better name, or if obscure names like OUTPUT_H0S1 are acceptable, then OUTPUT_HD or OUTPUT_H could be used. For NRF5X, if you don't want to fail for OUTPUT_H then it could mean the same as OUTPUT_H0H1.

MLXXXp avatar Oct 18 '16 18:10 MLXXXp

@MLXXXp I mean not to define the "Disconnected" modes, when it's unsupported. This helps not destroying something when it's not available.

When a MCU is not able to have a "High Drive" equivalent then OUTPUT_H???? should undefined.

I hope the naming schema supports all 8 modes available.

d00616 avatar Oct 19 '16 06:10 d00616

@d00616, you said:

For SAMD any combination including H is configured as high drive mode. Any Disconnected configuration should fail, when unsupported.

For SAMD, (other than OUTPUT, which currently sets low drive) the only mode supported is OUTPUT_H0H1. Therefore, just as you're saying "Disconnected" modes should be undefined, the only mode that should be defined for SAMD is OUTPUT_H0H1, not any combination containing an H. And since only one high drive mode is allowed for SAMD, there should be a simplified name available, such as OUTPUT_H.

However, I also note that Arduino generally tries to make things as easily readable and understandable as possible. So rather than using OUTPUT_H, spelling it out fully as OUTPUT_HIGH_DRIVE makes it clearer to someone looking at, and trying to understand, the code. Also for this reason, OUTPUT_x0x1 may be a bit cryptic for the type of people that the Arduino environment is intended for.

MLXXXp avatar Oct 19 '16 16:10 MLXXXp

I recently went through the learning process of tracking down the DRVSTR settings, so including that keyword might help the name be self documenting. How about:

OUTPUT_DRVSTR0 being the same as OUTPUT, setting low current mode and OUTPUT_DRVSTR1 being the high output mode?

Googling "samd DRVSTR" immediately leads to answers today...

In any case, it should be implemented before the next release. Sandeep's (wise I think) update from a bitwise to a register write means pinMode(pin,OUTPUT) will now force pin into low current mode. Following immediately with pinStr(pin,HIGH) (my function that does the obvious) would still result in a very brief window where pin was not set to source/sink the high current it is physically connected to. With the current release's bitwise settings I can run pinStr(pin,HIGH) first before pinMode(pin,OUTPUT) and avoid that exposure.

On the practical side, pulling 5 mA out of a pin on DRVSTR0 on a Feather M0 doesn't seem to have damaged anything. The voltage dropped to 2.5 from 3.3, but many seconds in this state didn't prevent operating OK once the fault was fixed.

sellensr avatar Feb 11 '17 14:02 sellensr

Currently using the MkrZero. Is there a work around to set the DRVSTR to allow 7 mA?

dstepit avatar Mar 23 '17 17:03 dstepit

void pinStr( uint32_t ulPin, unsigned strength) // works like pinMode(), but to set drive strength
{
  // Handle the case the pin isn't usable as PIO
  if ( g_APinDescription[ulPin].ulPinType == PIO_NOT_A_PIN )
  {
    return ;
  }
  if(strength) strength = 1;      // set drive strength to either 0 or 1 copied
  PORT->Group[g_APinDescription[ulPin].ulPort].PINCFG[g_APinDescription[ulPin].ulPin].bit.DRVSTR = strength ;
}

is what I am using

sellensq avatar Mar 23 '17 17:03 sellensq

@sandeepmistry will this be fixed in the next possible release? I think it might create unnecessary bug from user point of view. 7 mA drive is a better and safer bet that it will work with most stuff.

rocketscream avatar Sep 07 '17 14:09 rocketscream

Has there been some sort of decision or non-decision on this? I was fiddling with max toggle speeds, and the 12MHz output waveform with "normal" drive strength is really ugly (doesn't quite get to GND) A patch to always use full drive strength in pinMode() is pretty trivial.

Is there actually a reason to use anything other than "high" drive strength? I'm not particularly in favor of adding a new API to choose...

WestfW avatar Apr 26 '18 06:04 WestfW

1yr 8months without any decision? Really?

@sandeepmistry

tuxedo0801 avatar Apr 26 '18 16:04 tuxedo0801

Note that there are "GPIO Clusters" with limited total sink/source capacity, so you may not want to switch on high strength without a reason. (Details vary with the specific chip -- see the datasheet.) My very limited experience suggests low strength is somewhat self limiting. Also note that the datasheet gives typical GPIO output voltages as <10% VDD and >90% VDD, not all the way to ground and supply levels.

sellensr avatar Apr 26 '18 19:04 sellensr

Another argument for not having only high drive mode is that it can cause more signal noise due to faster slew rates into the same load.

MLXXXp avatar Apr 26 '18 19:04 MLXXXp

void pinStr( uint32_t ulPin, unsigned strength) // works like pinMode(), but to set drive strength
{
  // Handle the case the pin isn't usable as PIO
  if ( g_APinDescription[ulPin].ulPinType == PIO_NOT_A_PIN )
  {
    return ;
  }
  if(strength) strength = 1;      // set drive strength to either 0 or 1 copied
  PORT->Group[g_APinDescription[ulPin].ulPort].PINCFG[g_APinDescription[ulPin].ulPin].bit.DRVSTR = strength ;
}

is what I am using

Thanks for this @sellensq - this worked for me!

mhazley avatar Oct 19 '18 15:10 mhazley

Do you know if analogwrite function can set the drive strength? such as for pin11 ATSAMD21G18A? and how?. The value in analogwrite is already set to 255 (pin11, 255) but I need some more power to the pin. tx

DucDoug avatar Feb 03 '20 20:02 DucDoug

@facchinm You're right that the DRVSTR flag is unfortunately still reset ATM by the pinMode(pin, OUTPUT) code, I wrote my own pinModeOutput variant that fixes that problem below and tested the flag stays on after that: (note the use of the PORT_PINCFG_DRVSTR flag in the new mask)

// like pinMode(pin, OUTPUT), but to set drive strength (max is 2mA if false, 8mA if true)
bool setHighStrenghOutputPinMode( uint32_t ulPin)
{
  // Handle the case the pin isn't usable as PIO
  if ( g_APinDescription[ulPin].ulPinType == PIO_NOT_A_PIN ) return false;

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

  // enable input, to support reading back values, with pullups disabled and DRVSTR set
  PORT->Group[port].PINCFG[pin].reg=(uint8_t)(PORT_PINCFG_INEN | PORT_PINCFG_DRVSTR) ;
  // Set pin to output mode
  PORT->Group[port].DIRSET.reg = pinMask;
  return true;
}
 

fab672000 avatar Feb 10 '20 17:02 fab672000

Following up, @fab672000 does your excellent modification also apply to the DACs?

dansteingart avatar Oct 06 '21 14:10 dansteingart

Following up, @fab672000 does your excellent modification also apply to the DACs?

As my code indicates, it should be applicable to any pin type different from 'PIO_NOT_A_PIN'. So if your pin is not in that category, it should be able to set the DRVSTR flag for that pin, but that's all I can say as for particular cases or restrictions you may still need to look at the SAMD datasheet. Also note that you can know if the flag was set if the function returns true, when this bit can't be set, it should return false.

fab672000 avatar Oct 06 '21 17:10 fab672000

I don't think I've seen anyone present a strong argument (or ANY argument, really) against changing this, and yet it's been five years plus without any fix. What's the problem?

WestfW avatar Dec 13 '21 07:12 WestfW

PS: I don't believe that you can change the output power of DACs. That circuitry is separate, and there's a section in the datasheet that says (I think) you're supposed to have at least a 5kohm load.

WestfW avatar Dec 13 '21 07:12 WestfW