analogWrite() and Servo::attach() conflict

[henrygab]

Describe the bug

analogWriteResolution(uint8_t res) will cycle through the three (or four) PWM peripherals, and set each PWM peripheral's maximum value by calling PWM::setMaxValue() to reflect this maximum resolution.

analogWrite(uint32_t pin, uint32_t value) adds the pin to the first unused PWM channel. It will cycle through the three (or four) PWM peripherals, each of which has four channels, until it finds a free channel.

Servo::attach(int pin, int min, int max) also adds the pin to the first unused PWM channel, and also cycles through the three (or four) PWM peripherals, each of which has four channels, until it finds a free channel. It then unconditionally calls both PWM::setClockDiv(7) and PWM::setMaxValue(2500) (for 20ms / 50Hz freq) for the PWM peripheral that had the free channel.

Set up (please complete the following information)
Discovered via code review while investigating #447.

Based on code review, it appears that a call to analogWrite() on a first pin, followed by a call to Servo::attach() on a second pin, will result in both pins being controlled by a single PWM peripheral. In addition, the call to Servo::attach() will disrupt the PWM output from the first pin, because the maximum value and clock divisor will be changed.

Expected behavior
analogWrite() and Servo::attach() do not interact with each other.

[henrygab]

Goals

• Prevent analogWrite() and Servo::attach() from interfering with each other.

• Prevent Servo::attach() from using a PWM peripheral, unless that PWM peripheral either (a) was already being used by a Servo, or (b) is not currently in use.

• PreventanalogWrite() using a PWM peripheral, unless that PWM peripheral either (a) was already being used for analog writes, or (b) is not currently in use.

• Prevent analogWriteResolution() from modifying a PWM peripheral, unless that PWM peripheral either (a) was already being used for analog writes, or (b) is not currently in use. The value should be cached. In addition, when starting a new PWM peripheral for analog writes, apply the cached resolution.

---

Thoughts on how to fix...

• Each object (analog, servo) tracks if it's currently using one of the hardware PWM objects using state that is private to that class of object.
• When looking for a new PWM channel, it will first look at those PWM objects that the class of object is already using, and preferentially use another channel from that peripheral.
• If there are no free channels in already-in-use PWM peripherals, then it will check other PWM peripherals, but only if they are not enabled.
• When adding a new PWM peripheral (and thus 4 new channels), ensure the peripheral is enabled to prevent other objects from trying to simultaneously use them.

---

[henrygab]

• tone()/noTone() APIs also seem to blindly use peripherals. It even has the following comment:

// Use fixed PWM2, TODO could conflict with other usage

[henrygab]

list of potentially impacted functions

From the Arduino API references, here's a list of functions that might be impacted by fixing this. file paths are all under cores\nRF5\.... Please comment if you know of other functions that would/should also be checked....

Function	Header	Implementation
digitalRead()	wiring_digital.c	wiring_digital.h
digitalWrite()	wiring_digital.c	wiring_digital.h
pinMode()	wiring_digital.c	wiring_digital.h
analogRead()	wiring_analog_nRF52.c	wiring_analog_nRF52.h
analogReadResolution()	wiring_analog_nRF52.c	wiring_analog_nRF52.h
analogReference()	wiring_analog_nRF52.c	wiring_analog_nRF52.h
analogWrite()	wiring_analog.cpp	wiring_analog.h
analogWriteResolution()	wiring_analog.cpp	wiring_analog.h
tone()	Tone.cpp	Tone.h
noTone()	Tone.cpp	Tone.h
pulseIn()	no change	no change
pulseInLong()	not implemented	not implemented
shiftIn()	no change	no change
shiftOut()	no change	no change

[henrygab]

@hathach -- Before I start finalizing and starting any testing of fixes in this, I would like your review of the intended higher-level (architectural) design. In particular, I would appreciate your review of the proposed method to detect whether the PWM peripheral is already in use. A later review will be on the wrapper API that makes this simple, and will focus on ensuring a method to atomically acquire a peripheral (only if not already in use) is exposed.

To summarize the problem, various abstraction layers in the BSP have code that use peripherals, even if those peripherals are already in use (e.g., by other abstraction layers, libraries, etc). For PWM, the abstraction layers can include Tone, analogWrite, etc.

At a very high level, the way to fix this...

• At each abstraction layer, track which peripherals the layer has already acquired and is using
• At each abstraction layer, prefer to re-use / add to existing use of those peripherals already acquired
  • e.g., for analogWrite, if there's a free channel in a PWM peripheral that analogWrite is already using, then add the pin to that channel ... rather than attempting to acquire another PWM peripheral
• When needing to acquire another peripheral, do not use a peripheral if it appears to already be in use

For PWM, the proposed definition of "in use" for a PWM peripheral:

1. If the device is enabled, it's in use
2. If the device has any pins connected to any of the channels, it's in use
3. If the interrupt is connected, it's in use
4. Else, it can be claimed

The above concept can detect any use of the peripheral, whether by the BSP or a 3rd party library. It allows for smooth transition ... as more code checks before starting to use a peripheral, the system gets more robust ... while not requiring any 3rd party code do these checks.

Thanks!

[hathach]

Thank you very much Henry, unfortunately I am currently porting esp32s2 to my TinyUSB stack. Making progress but it stills takes time. Since esp32s2 port blocks others from Adafruit team to work on circuit python port, it takes the priority. Please be patient meanwhile.

[henrygab]

FYI, I've started a branch for this in my fork, to give you a sense of what the design I've mentioned would look like.

• See PWM_Conflicts

The design fixes many bugs, which although not likely seen in short "example" sketches, can bite in attempts to use the board in more complex designs.

The first few commits show how analogWrite() gets updated, so that it avoids overwriting of a PWM peripheral that is in use by something else.

[hathach]

Sorry for the delayed response, I really like your idea and code on your branch. yeah I am fully aware of the potential conflict of PWM peripherals. Neopixel also use PWM as well (But we can leave that off for now). But I am too lazy to fix it unless it becomes a real problems. Not many sketches mix uses them to the point of conflict so far.

For this PR, we don't have to resolve everything. We can just ignore digitalRead()/write() them user should not use the pin that already used by other purpose. analogWrite() & Servo() and Tone()` should be improved to

1. Look for an suitable peripheral PWM which is defined as:
   • unused (ENABLE and 4 Pins are not connected) like how neopixel lib does , don't worry about the IRQHandler, the PIN is pretty much the indicator.

• occupied by the same group of function e.g analogWrite()/Servo() and still has a free channel

2. Claim the peripheral (if not already), HardwarePWM class should have an owner byte to indicate whether the current owner is analog(), Servo, Tone() (Neopixel but leave this one for later, since it is on different lib).
3. So far the same group of function should be able to share the same peripheral with different channel.
4. analogWriteResolution() should only set a global variable, which only apply to module occupied by the analogWrite() function. same applies to Servo().

If you don't mind, you could create an (draft) PR for this. It would be much easier to review code changes. I will also try to submit pr to your pr as well if needed.

[henrygab]

We can just ignore digitalRead()/write() them user should not use the pin that already used by other purpose.

We may have to disagree on whether ignoring such calls is correct... I do not think that it is good, you think it's OK.

analogWrite() & Servo() and Tone()` should be improved to

1. Look for an suitable peripheral PWM which is defined as:

   • unused (ENABLE and 4 Pins are not connected) like how neopixel lib does , don't worry about the IRQHandler, the PIN is pretty much the indicator.

• occupied by the same group of function e.g analogWrite()/Servo() and still has a free channel

Yes, if by "occupied by the same group of function", you mean analogWrite() is separate from Servo(), and both are separate from Tone(), all of which are separate from other libraries.

2. Claim the peripheral (if not already), HardwarePWM class should have an owner byte to indicate whether the current owner is analog(), Servo, Tone() (Neopixel but leave this one for later, since it is on different lib).

I suppose HardwarePWM could keep an opaque void * that corresponds to an "Owner Context". I had planned on each abstraction keep track of its "in use" ones itself, but using a void * may be more useful for debugging. I like it (but it's not part of the PR yet).

3. So far the same group of function should be able to share the same peripheral with different channel.

Yes!

4. analogWriteResolution() should only set a global variable, which only apply to module occupied by the analogWrite() function. same applies to Servo().

Yes!

If you don't mind, you could create an (draft) PR for this. It would be much easier to review code changes. I will also try to submit pr to your pr as well if needed.

Yes!

[hathach]

We may have to disagree on whether ignoring such calls is correct... I do not think that it is good, you think it's OK.

If you are willing to work on this, that would be great. However, please make it as a separate PR. Since this is really optional to me. There are many things that will go wrong with many of Arduino libraries if user don't call API in a certain way. We won't able to fix everything. I see no problems if there are no complains 😄

Yes, if by "occupied by the same group of function", you mean analogWrite() is separate from Servo(), and both are separate from Tone(), all of which are separate from other libraries.

Yes, it is what I mean. I am not native English speaker, I have trouble expressing my thought all the time :D .

[dhalbert]

There is code in CircuitPython to do cross-peripheral timer management: https://github.com/adafruit/circuitpython/blob/master/ports/nrf/peripherals/nrf/timers.c
and see, for instance, https://github.com/adafruit/circuitpython/blob/master/ports/nrf/common-hal/pulseio/PWMOut.c#L142