macro digitalPinToTimer(P) breaks on DUE

[bperrybap]

The macro digitalPinToTimer(P) is supposed to report whether the pin
supports PWM.

This allows doing something like this:

  // Check if the pin is associated to a timer, i.e. PWM
  // ---------------------------------------------------
  if(digitalPinToTimer(_backlightPin) != NOT_ON_TIMER)

The macro in DUE not only doesn't work but is improperly defined:

# define digitalPinToTimer(P)       (  )

This creates a compile error on DUE code.
This needs to be updated and corrected to work correctly.

[bperrybap]

Guys, it's been 6 months, several releases and no fix yet.

The code I've seen using this is trying to detect if the pin supports pwm or not.
The reason it is being used that the current behavior of analogWrite() on non pwm pins is actually not very desireable and the code is trying to determine if pwm is not supported in order to change the pin dimming behavior on non pwm pins.

The current behavior for analogWrite() is that on non PWM pins < 128 is LOW and > 127 is HIGH.
In many cases, what is more desirable is that 0 is LOW and any other non zero value is HIGH. For things like a backlight, this makes more sense as it means that when dimming is attempted on a non pwm/dimming pin,
you get full on.
Off still works, but dimming doesn't.
This is not the case with the analogWrite() current behavior.
If you attempt to dim on a non PWM pin, the backlight will be off for half the values.

Anyway, for this fix, there are multiple choices, it could return NOT_ON_TIMER which would indicate that pwm is never supported, or it could return something else to indicate that pwm is supported, perhaps even call due_get_channel_status() to detect if pwm is supported.

I think the most important thing is to make sure that any code that uses this macro doesn't error off with a compile error.

[matthijskooijman]

Hey Bill,

it seems that digitialPinToTimer is the wrong way to detect if PWM is
supported - it's really an AVR implementation detail that a timer is
needed for PWM. I suspect that on the Due, stuff is organized in a
significantly different manner.

Having said that, in (at least) 1.5.x there is a digitalPinHasPWM()
macro, which seems to do exactly what you need and is implemented on the
due as well:

https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/sam/variants/arduino_due_x/variant.h#L69

Isn't this sufficient for what you need?

Gr.

Matthijs

[bperrybap]

That is a good alternative for this application. However, I still think that the the variant header file needs to be fixed since the way it is now it is defining a broken macro.
I'd pick between 1 of 2 ways to fix this:
Either completely remove the macro, so that at more obvious compile error would occur
or simply define it as NOT_ON_TIMER so that that any code that attempts to use it would continue to compile and "work". Any code attempting to use it would however,
think that PWM is not supported even if the pin supported it - which is probably an ok compromise given that the functionality doesn't really exist.

[matthijskooijman]

That is a good alternative for this application. However, I still
think that the the variant header file needs to be fixed since the way
it is now it is defining a broken macro.
Ah, I now see what you mean:

#define digitalPinToTimer(P) ( )

at https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/sam/variants/arduino_due_x/variant.h#L64

Seems I missed that this was a github issue, I thought I replied to a
mailing list post ;-p

I'd pick between 1 of 2 ways to fix this:
Either completely remove the macro, so that at more obvious compile error would occur
or simply define it as NOT_ON_TIMER so that that any code that
attempts to use it would continue to compile and "work". Any code
attempting to use it would however, think that PWM is not supported
even if the pin supported it - which is probably an ok compromise
given that the functionality doesn't really exist.
Your suggestions seem like the sane options here, but I'm not sure which
is best.

Leaving it out allows code to do #if defined(digitalPinToTimer) and know
hat the platform doesn't support pin/timer mappins, but defining it as
NOT_ON_TIMER allows existing code to compile directly (though probably
not run as expected, so I'm inclined to leaving it out entirely).

@cmaglie, any thoughts?

[bperrybap]

I went and dug a little deeper. The digitalPinToTimer() macro is in Arduino.h for the AVR in both 1.x and 1.5x which is down in the core area. On the 1.5x SAM it used to be in Arduino.h but has been moved to the variant file starting in 1.5.6-r2

Given the functionality relaly doesn't exist for the SAM core,
I think perhaps the best alternative might be to simply remove it all together to let any code using it hard fail with a clean error message vs the odd message you get now.

I have corrected my code to check for the existence of digitalPinHasPWM() to use it; otherwise fall back to using digitalPinToTimer() and then if that a doesn't exist, assume no PWM.

[bperrybap]

Just as a final thought, maybe when the digitalPinToTimer() macro is removed, a comment could be added in its place
to help out users looking for the macro. Maybe something like this macro doesn't exist in the SAM core and that for detecting PWM support, digitalPinHasPWM() can be used instead.

Although, it might be usueful to put this comment back up in Arduino.h rather than in the variant file so that it isn't missed should there ever be other SAM variants and also to make its location consistent with the AVR definition which is in Arduino.h rather than the variant files.

[cmaglie]

fixed with #2319.

Do you have some suggestions on how to handle portModeRegister too (apart completely disabling it)?

[JulyJim]

So the macro is gone and LiquidCrystal::setBacklight won't compile with this error:

C:\Documents and Settings\Vaclav\My Documents\Arduino\libraries\LiquidCrystal\LiquidCrystal.cpp: In member function 'virtual void LiquidCrystal::setBacklight(uint8_t)':
C:\Documents and Settings\Vaclav\My Documents\Arduino\libraries\LiquidCrystal\LiquidCrystal.cpp:173:41: error: 'digitalPinToTimer' was not declared in this scope
       if(digitalPinToTimer(_backlightPin) != NOT_ON_TIMER)
                                         ^
Error compiling.

From your discussion I am assuming that is the way it should work / report error.
So I can safely comment out the unusable code as long as I keep track of the change for future reference.

[bperrybap]

LiquidCrystal::setBacklight won't compile with this error.

I'm assuming you are working with fm's LiquidCrystal library.
You could take the "lazy" way out and comment out the now non-compiling code,
but that would remove/break the functionality.
The better choice is to update the code to use a more portable way of detection
and then progressively fall back to non pwm when necessary.

Here is the updated code I that I use:

  // Check if the pin is associated to a timer, i.e. PWM
  // Newer 1.5x Arduino has a macro to check for PWM capability on a pin
  // ---------------------------------------------------

#if digitalPinHasPWM
      if(digitalPinHasPWM(_backlightPin))
#elif digitalPinToTimer
      // On 1.x Arduino have to check differently
      if(digitalPinToTimer(_backlightPin) != NOT_ON_TIMER)
#else
      if(0) // no way to tell so assume no PWM
#endif

--- bill