PWM random inversion

[stephenf7072]

Subject of the issue

Analog output pin PWM waveform sometimes comes out as inverted.

A hack fix is to call am_hal_ctimer_clear(timer,segment) before/inside analogWrite, works for a single pin, I have not tested on multiple pins. I looked at this in some detail and it's a little beyond me for now, but I gather it's got to do with the Ambiq SDK changing from 2.2 to 2.3+, and they have made some kind of fix for it.

As an addition, the PWM frequency (about 180Hz) is low enough to cause buzzing on my peripherals, an option to change it would be really awesome.

Some related stuff here:
https://forum.sparkfun.com/viewtopic.php?f=170&t=51506&p=212699&hilit=analogwrite#p212699

Your workbench

Arduino IDE 1.8.12 on Windows 10. Core 1.0.30 (latest as at 31/3/2020)

Steps to reproduce

set up PWM pin to drive an LED
set PWM in a while loop - no issue, apparently
set PWM once each loop - random inversion

Expected behaviour

Write 25% (ie. 64 of 255), LED is dim

Actual behaviour

LED is bright (ie. 75%)

[oclyke]

@stephenf7072 I tried to reproduce your results, but I could use a little more info. I was unable to replicate the inversion under typical circumstances but I was able to cause undesirable behavior when pushing the CTimer peripherals hard.

Here's my test code:

#define PWM_PIN 5 // built-in led on RBA ATP
#define DELAY 2 // time between writing new PWM values in loop (ms)

#define DELAY_IN_LOOP 1 // 1 to add delays in loop (works) 0 to have no delays (no worky)

void setup() {
  analogWrite(PWM_PIN, 64); // setting the duty cycle once allows the hardware to handle PWM generation w/o interruption
  delay(1000);

  // set PWM value in a while loop for 5 sec
  // using delays here allows the hardware to generate the signal without being reset - which happens when the desired duty cycle is changed
  uint32_t now = millis();
  while(millis() < 5000 + now){ 
    for(uint16_t indi = 0; indi < 255; indi++){
      analogWrite(PWM_PIN, indi);
      delay(DELAY);
    }
    for(uint16_t indi = 255; indi > 0; indi--){
      analogWrite(PWM_PIN, indi);
      delay(DELAY);
    }
  }
}

// set PWM value once per loop w/o any delays
// without any delays this code does not produce the desired results - because the duty cycle is updated before any pulse frames can be ouput
uint16_t value = 0;
int dir = 1;
void loop() {
  analogWrite(PWM_PIN, value);
  value += dir;
  if(value == 0){
    dir = 1;
  }else if(value == 255){
    dir = -1;
  }
#if DELAY_IN_LOOP
  delay(DELAY);
#endif
}

Could you share a reproducible example? (I am using the RedBoard Artemis ATP with the LED on pin 5 [CT8])

Clearing the timer in calls to analogWrite() causes problems with the first method of generating a pulsating LED in the test sketch.

P.s. regarding the frequency of the PWM in analogWrite(). This is an artifact of my desire to provide the utmost possible resolution in calls to analogWrite(). The fastest clock that can be selected on the Apollo3 for the CTimer modules (as supported in HAL 2.2.0) is 12 MHz. The PWM generation requires specification of the 'frame width' and the high time (both in number of pulses at the clock speed). analogWrite() uses a frame width of 0xFFFF which is the maximum value that can be used. This simplifies things for when the user sets the resolution with analogWriteResolution().

That is all that way to be compliant with the Arduino API. However I built in a back-door for use on the Artemis / Apollo3.

Try using ap3_pwm_output(uint8_t pin, uint32_t th, uint32_t fw, uint32_t clk) to create a pwm signal with a period of fw and a time high of th in terms of clock cycles of the clock source clk on pin)

clk should be a HAL enumerated / defined value. So for example:

ap3_pwm_output(5, 3000, 6000, AM_HAL_CTIMER_HFRC_12MHZ); would create a 50% duty cycle PWM signal with a frequency of 2kHz

[stephenf7072]

Thanks for having a look - you're right, I was remiss in not providing example code to replicate the error. The inversion happens when I'm running all sorts of other things in the loop, so it could be a bit before I have a chance to dig into it more.

I did notice and try out the ap3_pwm_output() function, and it didn't work for me (at higher frequencies, the PWM duty cycle stayed very low). Again something I can't replicate in a simple sketch, I'll look into more when I get the chance.

[Wenn0101]

I have taken a quick look at this as well.

I made this sketch to try to observe the problem (The sketch will pulse the led by repeatedly calling analogWrite). I believe I can see the problem in this sketch and I am using it to try to characterize when/how the inversion happens. We can use this sketch for testing.

const int ledPin = A0;
const int FPS = 10; //(frames/sec)
const int rateOfChange = 100; //(counts/second)
const int maxVal = 240;
const int minVal = 20;

//some values im going to compute in setup and use in loop
int countsPerFrame;
int delayTime;

//initial conditions
int val = 20;
int dir = 1;

void setup() {
  pinMode(ledPin, OUTPUT);
  analogWrite(ledPin, val);
  delayTime = 1000/FPS; //(msec/frame)
  countsPerFrame = rateOfChange/FPS; //(counts/frame)
  
 Serial.begin(9600);
 delay(50);
 Serial.println( "FPS: " + String(FPS) + ", DelayTime : " + delayTime + ", CountsPerFrame : " + countsPerFrame);
}

void loop() {
  val += dir*countsPerFrame;
  if(val > maxVal)
  {
    val = maxVal;
    dir= -1;
  }
  else if (val < minVal)
  {
    val = minVal;
    dir = 1;
  }

  analogWrite(ledPin, val);
  //Serial.println(val);
  delay(delayTime);
}

[walshbp]

I did notice and try out the ap3_pwm_output() function, and it didn't work for me (at higher frequencies, the PWM duty cycle stayed very low).

Playing around with ap3_pwm_output on my nano, I noticed differences dependent on which pin I used. Varying inputs on ap3_pwm_output while using pin A14 (Pad 35), always yielded a constant 180Hz signal. Pin 13 (Pad 6) worked as expected.

[oclyke]

Following some leads from this SparkFun forum post suggested that Ambiq SDK versions 2.3.0 and higher would include a fix for this. I created a branch upgrade-sdk-2.4.2 that uses the latest SDK. Using that branch and the test sketch from @Wenn0101 I was unable to see any inversion (both visually and with the aid of a digital logic analyzer)

To confirm that this was a solution I switched back to the master branch and re-tested the same code. I observed no visible differences so I am not sure that there was any significant change. @Wenn0101 does the test code produce the error by default or do you need to change some parameters such as delayTime? Or perhaps - as some have suggested - the inversion is pad-dependent? I switched from A0 to Apollo3 pad 5 to use the built-in LED on my RBA ATP.

[oclyke]

@stephenf7072 I just verified ap3_pwm_output() for several test cases such as:

ap3_pwm_output(ledPin, 100, 100, AM_HAL_CTIMER_HFRC_12MHZ);
ap3_pwm_output(ledPin, 98, 100, AM_HAL_CTIMER_HFRC_12MHZ);
ap3_pwm_output(ledPin, 50, 100, AM_HAL_CTIMER_HFRC_12MHZ);
ap3_pwm_output(ledPin, 2, 100, AM_HAL_CTIMER_HFRC_12MHZ);
ap3_pwm_output(ledPin, 1, 100, AM_HAL_CTIMER_HFRC_12MHZ);
ap3_pwm_output(ledPin, 0, 100, AM_HAL_CTIMER_HFRC_12MHZ);

This produced duty cycles as expected with a base frequency of 120 kHz.

it didn't work for me at higher frequencies

How great frequencies are we talking about? Have you seen the note in the source code:

//**********************************************
// ap3_pwm_output
// - This function allows you to specify an arbitrary pwm output signal with a given frame width (fw) and time high (th).
// - Due to contraints of the hardware th must be lesser than fw by at least 2.
// - Furthermore fw must be at least 3 to see any high pulses
//
// This causes the most significant deviations for small values of fw. For example:
//
// th = 0, fw = 2 -->   0% duty cycle as expected
// th = 1, fw = 2 --> 100% duty cycle --- expected 50%, so 50% error ---
// th = 2, fw = 2 --> 100% duty cycle as expected
//
// th = 0, fw = 3 -->   0% duty cycle as expected
// th = 1, fw = 3 -->  33% duty cycle as expected
// th = 2, fw = 3 --> 100% duty cycle --- expected 66%, so 33% error ---
// th = 3, fw = 3 --> 100% duty cycle as expected
//
// th = 0, fw = 4 -->   0% duty cycle as expected
// th = 1, fw = 4 -->  25% duty cycle as expected
// th = 2, fw = 4 -->  50% duty cycle as expected
// th = 3, fw = 4 --> 100% duty cycle --- expected 75%, so 25% error ---
// th = 4, fw = 4 --> 100% duty cycle as expected
//
// ...
//
// Then we conclude that for the case th == (fw - 1) the duty cycle will be 100% and
// the percent error from the expected duty cycle will be 100/fw
//**********************************************

[oclyke]

@Wenn0101 and I were able to fix the random inversion and add an additional way to control the frequency of analog output.

Check out the no-inversion branch to give it a spin.

Be aware: not all PWM capable pins on the Apollo3 are independent of one another. There are 'primary' and 'secondary' pins. The primary pins determine the frequency of the signal for that pair (of which there are 16... Timers 0-7 segements A-B). In order to change the frequency of a secondary output the frequency of the corresponding primary output must be changed as well. This is accounted for as well as we can in the core, but it may still cause some unforeseen behaviours.

Additionally... inversion prevention has been changed from one strategy to another. Previously the analogWrite() function would attempt to block until at least one pulse had been sent -- so that the compare values can change when the output is low (because the output just inverts on compares - you can't force the phase). That allowed the user to change the desired value as quickly as possible while maintaining the integrity of the duty cycle.

Now the way it works is that analogWrite() does not block - it simply resets the timer. This has a consequence -- changing the value before a pulse has completed can truncate it. If you do this repeatedly you will not observe the expected average duty cycle.

It is possible that we could combine the frequency control with the old method of inversion protection - though the old method felt too prone to cause race conditions. Perhaps if it could be interrupt-driven...

TLDR:
Two new functions:

ap3_err_t analogWriteFrameWidth(uint32_t fw); // set the frame width in clock cycles (12 MHz clock is used by default for analogWrite)
ap3_err_t analogWriteFrequency(float freq); // allow the core to try to get close to your desired analogWrite frequency in Hz

[stephenf7072]

Thanks for your efforts guys - especially on setting the frequency, that will be fantastic.

I tried the new branch and... it's not working too well for me. I can compile/load/run some basic examples (blink, GPIO_sleep_with_wake), but anything with I2C or SPI doesn't even compile.
I2C example: /Example1_BasicReadings
SPI example: /CardInfo

I'm running Arduino IDE 1.8.12, and I replaced the contents of C:\Users\XXXX\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\1.0.30 with the trial branch, then restarted Arduino.

Error messages reference Arduino_Apollo3/libraries/SPI/src/SPI.cpp which (among others?) perhaps wasn't updated with the SDK but should have been? The undeclared AM_HAL_IOM_FULLDUPLEX in the error messages was declared in the previous SDK version, but apparently only as a "To do" item. I tried commenting out the lines which use it, which let it compile, but the application sketch on my custom board wouldn't even spit out a hello world, so I can't test the PWM aspect. I suspect full duplex operation is fairly important though.

The complete error messages (for both SPI and interestingly, I2C):
**I tried a million times to use code tags, and delete the excess apostrophes, but still can't get it to display properly, sorry.

`C:\Users\steph\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\1.0.30\libraries\SPI\src\SPI.cpp: In member function void SPIClass::_transfer(void*, void*, size_t):

C:\Users\steph\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\1.0.30\libraries\SPI\src\SPI.cpp:297:30: error: AM_HAL_IOM_FULLDUPLEX was not declared in this scope

 iomTransfer.eDirection = AM_HAL_IOM_FULLDUPLEX;

C:\Users\steph\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\1.0.30\libraries\SPI\src\SPI.cpp:297:30: note: suggested alternative: AM_HAL_IOM_NUM_MODES

 iomTransfer.eDirection = AM_HAL_IOM_FULLDUPLEX;

                          AM_HAL_IOM_NUM_MODES

C:\Users\steph\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\1.0.30\libraries\SPI\src\SPI.cpp:309:33: error: AM_HAL_IOM_FULLDUPLEX was not declared in this scope

if (iomTransfer.eDirection == AM_HAL_IOM_FULLDUPLEX)

C:\Users\steph\AppData\Local\Arduino15\packages\SparkFun\hardware\apollo3\1.0.30\libraries\SPI\src\SPI.cpp:309:33: note: suggested alternative: AM_HAL_IOM_NUM_MODES

if (iomTransfer.eDirection == AM_HAL_IOM_FULLDUPLEX)

                             AM_HAL_IOM_NUM_MODES

exit status 1
Error compiling for board SparkFun RedBoard Artemis ATP.

`

[oclyke]

@stephenf7072 I apologize - this is an artifact of upgrading to the 2.4.2 release of the AmbiqSuite SDK. I had only tested on analogWrite examples so far. I will resolve these issues on that branch and then rebase.

[oclyke]

@stephenf7072 the no-inversion branch is now fixed. However AmbiqMicro removed the enumerated value for full duplex transfers in the IOMaster peripherals. For the time being full duplex transfers in SPI may not work in these two branches.

I will follow up with someone from Ambiq to see how we should implement full duplex SPI transfers.

[stephenf7072]

Thanks Owen for your efforts, I've yet to play with this one further because I've been distracted by the interrupt/sleeping stuff in the issue I just created.

[oclyke]

Everyone, I just made a final addition to that branch and made PR #153
I think we will all be happy that analogWrite should no longer invert and also has no restrictions on how frequently it is called. This is made possible by making ap3_pwm_output block until at least one pulse has been emitted.

[stephenf7072]

Looks like I'm a bit late to the party here, but a couple of comments now I've had a chance to spend some time on this issue:

1. I've replaced ap3_analog.cpp and ap3_analog.h in Core 1.0.30 and I haven't seen any PWM inversion. Great!
2. The changes to make compliant with SDK2.4 are a great thing, but it was unclear whether they will work with I2C or SD, so merging them into the master branch seems perhaps a bit hasty...
3. Especially given "float tempFloat = 1; Serial.println(tempFloat);" under the current master branch will cause a crash. Same for printing floats with Serial.printf().
4. Neither analogWriteFrameWidth() or analogWriteFrequency() seem to work correctly under the current master branch. Any setting gives incorrect PWM duty in my test code on a Redboard ATP (using an AIN with RC circuit to properly verify the duty output)

Test code:
(any hints on using code tags better?)

#define LED 5
#define outPin 29
#define inPin 16
int PWMLevel = 0;
int inputLevel = 0;
int scaledInputLevel = 0;
#define inputScale 100000/330196 //For 2x 10k resistors (Gnd->A16, A16->A29), 2.2uF capacitor (Gnd->A16)

void setup() 
{
  Serial.begin(115200);
  pinMode(LED, OUTPUT);  
  pinMode(inPin, INPUT);
  pinMode(outPin, OUTPUT);

//  analogWriteFrequency(180);
//  analogWriteFrameWidth(30000); // set the frame width in clock cycles (12 MHz clock is used by default for analogWrite)
}

void loop() 
{
  char inString[50] = "";    // string to hold input 
  int inChar = 0;
  char tempInChar[2] = "_";
  tempInChar[0] = (char)inChar;
    
  while (Serial.available() > 0) 
  {
    inChar = Serial.read();
    tempInChar[0] = (char)inChar;
    if (inChar != '\n')  // As long as the incoming byte is not a newline, convert the incoming byte to a char and add it to the string
      strcat(inString, tempInChar);
    else  // if you get a newline, process the string
    { 
      if(strlen(inString) > 1)  
      {
        PWMLevel = atoi(inString);
        Serial.println(PWMLevel);
        analogWrite(outPin,PWMLevel);
      }
    }
  }

  inputLevel = analogRead(inPin);
  scaledInputLevel = inputLevel*inputScale;
  Serial.printf("PWMLevel:%d,\tAin:%d,\tScaled Ain:%d\n",PWMLevel,inputLevel,scaledInputLevel);

  delay(500);
}

[oclyke]

Hints on code tags:

• single grave marks `code` are a shorthand that can be used in inline code snippets
• when going for multi-line code blocks you must use the full mark - that is three graves on either side like so: ```code``` (in this case the distinction between the two helped make it possible to format that example... see raw text)
• in multi-line code blocks you may specify the language for syntax coloring after a space on the first line

``` c
code in c
```

• I just learned that to enclose lots of backticks you may also 'encapsulate' them with a greater number of them, thanks to this stack exchange question

Coming back after a meeting to test / address what else you mentioned :D

[oclyke]

1. Good!
2. Good point. I will be speaking with someone from Ambiq Micro soon and will make it a point to ask about this.
3. I do not believe that this is related to the SDK increment. Nate and I observed this issue several months ago when we were still on SDK 2.0.0 or 2.2.0 - I am addressing that issue separately.
4. Just performed testing on the master branch and could not observe any issues. Tested using both pins 5 and 29 - which represent both outputs of the inter-dependent timer 2 segment A. Simplified test code is below. I used a DLA to verify output

const int ledPin = 5;
const int maxVal = 255;
const int minVal = 0;

//initial conditions
int val = 64*3;
int dir = 1;

void setup() {
//  // optional: test the extended API to adjust frequency or frame width
//  analogWriteFrequency(600.0); // sets analogWrite frequency to 600.0 Hz - valid range is ~200Hz to ~3MHz (very high frequencies will suffer resolution degradation)
//  analogWriteFrameWidth(400); // sets analogWrite period to 400 ticks of default 12 MHz clock - 12000000/400 = ~30 kHz
  
  analogWrite(ledPin, val);
  delay(500);                // test holding a signal
}

void loop() {
  val += dir;
  if(val > maxVal)
  {
    val = maxVal;
    dir= -1;
  }
  else if (val < minVal)
  {
    val = minVal;
    dir = 1;
  }

  analogWrite(ledPin, val); // test maximum re-writing speed w/o inversion
}

[oclyke]

I have patched support for FULLDUPLEX SPI transfers in #154

[stephenf7072]

I just tried again with my exact same setup and test code, only thing different being the master branch from , and... success, mostly. Maybe a change in the last few days, maybe something I did.

I could break analogWriteFrequency() with an input of 183Hz or lower, giving bogus output, usually about 50% duty cycle for any analogWrite() value other than 0 or 255. With an input of 184 Hz or more it was fine, so good enough for my purposes, might be worth a note somewhere though.

I couldn't make analogWriteFrameWidth() break, even with input value of 1200000000 (100Hz at 12MHz clock). Good times.

PS: thanks and noted re. the code tags. A grave subject indeed.
PPS: I tried my big project code with the new core - it compiles with apparently no errors or new warnings, but won't run - won't even turn on an LED or establish a serial connection. And yet, a simple serial sketch runs fine. Maybe something to do with #includes or something, I'll try some more things and try to pin it down. These are what I'm using:

#include "Arduino.h"
//#include "RunningMedian.h"
#include <SparkFunBME280.h>
#include <PushButtonClicks.h>
#include <wire.h>
#include <SPI.h>
#include <SD.h>
#include "AS726X.h"
#include "RTC.h"
#include <Time.h>
#include <TimeLib.h>
#include <hsc_ssc_i2c_SF.h>
#include <EEPROM.h>
#include "PCF8574.h"

[oclyke]

Hmmmm.... my first thought is to ask about what compiler you are using. I have not fixed the conflict between:

• BFD assertion errors when compiling with gcc-2018-q4 or later and
• Floating point super-crashes when compiling with gcc-2017-q4

So if you are using any floating point operations and compiling with the 2017 compiler that could be your issue (Blink runs fine cause it never touches floats...)

[stephenf7072]

Compiler? Dunno, whatever Arduino IDE 1.8.12 uses, and/or whatever is in the Artemis core.

Yeah, it's this line involving floats in the declaration of the global variable:
uint32_t sysTicksToSleep = (uint32_t)((double)msToSleep/1000 * TIMER_FREQ);

So I guess it all comes back to the floats issue.