adafruit / ArduinoCore-samd

115 stars 119 forks source link

analogWriteResolution() issues #256

Open mzero opened 4 years ago

mzero commented 4 years ago

The implementation in wiring_analog.c is inconsistent w.r.t. the write resolution value (_writeResolution):

  1. When writing to a DAC on a non-SAMD51 platform, the resolution is ignored, and the value assumed to be10 bits, and simply truncated (& 0x3ff, see line 475)
  2. When writing to PWM on a SAMD51 platform, the resolution is ignored, and the value assumed to be 8 bits.
  3. The initial value of the write resolution is 12 bits on SAMD51 platforms, but the default for Wiring is 8.

The first two are simply missing calls to mapResolution at about lines 475 and 487. The third can be fixed by removing the declaration of _writeResolution out from the conditionals (lines 30 to 36), and using 8 as the initializer for all platforms.

See: https://github.com/adafruit/ArduinoCore-samd/blob/master/cores/arduino/wiring_analog.c

It is important that the resolution be respected so that sketches work the same on different platforms (to within the limits of HW, of course). It is important that the resolution be initialized the same, as few sample sketches call analogWriteResolution and so the range of 8 bits is generally just assumed.

hathach commented 4 years ago

I did quick check, it is indeed the case. M0 should has an map resolution like other, would you mind submitting an PR for this. If you are busy, I could do this later on.

mzero commented 4 years ago

I can. I'll try to write a suitable test sketch as well. I these boards that I can test it on: Feather M0 Express, Feather M4 Express, and Circuit Playground Express. Hopefully, others can test this on other boards.