arduino / ArduinoCore-samd

Arduino Core for SAMD21 CPU
GNU Lesser General Public License v2.1
470 stars 717 forks source link

SAMD outputs do not provide high drive current #158

Open facchinm opened 8 years ago

facchinm commented 8 years ago

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 _wiringdigital.c will clear DRVSTR.

Therefore, at best, the documentation is misleading.

Ways I see to address the problem:

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().

sandeepmistry commented 8 years ago

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.

d00616 commented 7 years ago

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.

MLXXXp commented 7 years ago

@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.

d00616 commented 7 years ago

@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.

MLXXXp commented 7 years ago

@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.

sellensr commented 7 years ago

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.

dstepit commented 7 years ago

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

sellensq commented 7 years ago
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

rocketscream commented 7 years ago

@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.

WestfW commented 6 years ago

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...

tuxedo0801 commented 6 years ago

1yr 8months without any decision? Really?

@sandeepmistry

sellensr commented 6 years ago

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.

MLXXXp commented 6 years ago

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.

mhazley commented 5 years ago
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!

DucDoug commented 4 years ago

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

fab672000 commented 4 years ago

@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;
}
dansteingart commented 2 years ago

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

fab672000 commented 2 years ago

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.

WestfW commented 2 years ago

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 commented 2 years ago

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.