PWM signal from analogWrite does not stop when pin is set to different mode

[carlosperate]

Looks the analogWrite() "pwm" signal on the nRF51 (now PWM hardware, so uses a timer instead) does not stop operation even after pinMode() and digitalWrite() has been applied to the same pin.

Simple sketch for testing:

const int analogOutPin = 9;

void setup() {
  // put your setup code here, to run once:
  
}

void loop() {
  // Increase PWM until maximun value, takes around a couple of seconds
  for (byte i = 0; i<255; i++) {
    analogWrite(analogOutPin, i);
    delay(10);
  }

  // Toggle the same pin high and low a couple of times
  delay(1000);
  pinMode(analogOutPin, OUTPUT);
  digitalWrite(analogOutPin, HIGH);
  delay(1000);
  digitalWrite(analogOutPin, LOW);
  delay(1000);
  digitalWrite(analogOutPin, HIGH);
  delay(1000);
  digitalWrite(analogOutPin, LOW);
  delay(1000);
}

Expected behaviour:

• PWM signal increasing from minimum to maximum
• Same pin signal toggled high and low a couple of times
• loop iterates

Recorded behaviour:

• PWM signal increasing from minimum to maximum
• PWM signal stays at maximum value
• loop iterates

[sandeepmistry]

@carlosperate thanks for reporting! Any suggestions on fixing this?

[micooke]

The arduino core checks for this in digitalWrite.
This adds a small overhead, but its probably best to be consistent.

Looking at the core it would be easy to fix (maybe as little as 3 lines), i can have a look into it if you wish.

[sandeepmistry]

@micooke that would be great!

[micooke]

Hmm, its turning out to be a bit trickier than i expected. I am working on it though. I need to move a few static globals around so that they are accessible by wiring_digital.h. Ive implemented what i think should work, but it just doesnt at the moment. Ill keep you posted

[micooke]

I think i fixed it locally, i will need someone to check on a nrf52832 though.
Linkage is a little 'special', so if you digitalWrite on all allocated pwm pins timer1 will stay on and interrupt but won't toggle any pins (not a biggie). Fixed (only needed for NRF51)

I also added a fallback i found in the avr core that will use digitalWrite if all pwm channels are exhausted.
@sandeepmistry - it looks like pwm channel 0 is unavailable. Do you know if this is used by another peripheral?

Another note, there are only 3 PWM channels.
For the NRF51 : TIMER1 is used, TIMER2 could also be used to expand the number of PWM channels
For the NRF52 : I havent looked, but its a totally different structure

[micooke]

@sandeepmistry - ill wait on #186 and rebase my repo before i send a PR for this.

[micooke]

To summarise, analogWrite allocates a pwm channel and digitialWrite should deallocate a pwm channel if a write is registered to a pwm channel allocated to a (now) digital pin.

The static pwm pin allocation / timer active references were visible to wiring_digital.c but their state is invalid so i needed to move the struct defines to a global include (wiring_private.h) that is visible to both wiring_digital.c and wiring_analog_.c, then instantiate a struct in wiring_analog_.c which is referenced via extern in wiring_digital.c.

While i was modifying these 4 files i semi-standardised wiring_analog_*.c to use the same structs, and (as mentioned earlier) added a fallback i found in the avr core that will use digitalWrite if all pwm channels are exhausted.

The last thing to explain, the NRF51 could use timer2 to allocate more pwm pins (Timer1 is used by the softcore). I didnt explore this but it is the reason that the irqNumber variable was added to the PWMStatus struct (allocation for future expansion).

struct PWMStatus {
   int8_t numActive;
   int8_t irqNumber;
};

[micooke]

Needs testing against the nRF52 - Im sorry but i dont have one of these to test against.

[floe]

Since I need a very specific and stable PWM frequency without interrupt jitter, I implemented an alternative version which works entirely in hardware, based on the PPI and GPIOTE peripherals. I think that would also solve the issue with the PWM channels on the side. If there's interest, I'll see if I can prepare a PR.

[floe]

@micooke didn't see you already have a fix ready. Will this be merged at some point?

[micooke]

@floe - yeah sorry, i should have made reference to PR#195 instead of the last commit.

If you have any comments on my implementation please comment on the PR as the merge seems to have stalled.

And just to confirm here, the PR works for both the NRF51 and NRF52 😊