digitalWrite() doesn't override analogWrite()

[JelmerT]

Describe the bug
Once PWM is started on a pin with analogWrite(), the pin can't be set with digitalWrite() anymore.

Set up
NRF52840

To Reproduce
Steps to reproduce the behavior:

analogWrite(LED_GREEN,128);
digitalWrite(LED_GREEN,LOW);

Expected behavior
PWM on LED_GREEN goes to 50%, then pin goes low.
Actual behavior
-> pin stays at 50%

[JelmerT]

seems similar to arduino/ArduinoCore-arc32#280

[pyro9]

Agreed, it looks like the same problem. The simple but low performance answer would be to remove the pin from the relevant HardwarePWM object, but the performance would be terrible.

If I had a clean slate, I would suggest adding a pin mode OUTPUT_PWM and handling that in pinMode, but that wouldn't be compatible with other platforms (of course, neither is the current situation). It would improve the performance of analogWrite.

Next possability would be a simple table of current state. It would degrade performance of digitalWrite, improve the performance of analogWrite, and cost a bit of memory.

It's kind of a pick your poison situation. Personally, I would go with putting it in pinMode to avoid breaking applications that currently work on this platform. If the call to removePin is simply made unconditional when the mode is set to output, it would also avoid breaking most current cases where analogWrite is used. I THINK

Just my 2 cents, not really my call.

[henrygab]

@pyro9 CAUTION -- I am not 100% sure, but you may end up putting the board into an unsupported state if, after you call analogWrite(), you then set the pin to a different mode.

This may need to be looked at a bit deeper; There seems to also be some other bugs related to analog write.

If it's as it looks at my first glance, one alternative design would do something like the following:

For performance, keep an array indicating if the caller used analogWrite() more recently than the caller used digitalWrite(). This would be blindly set in analogWrite(). In digitalWrite(), if the pin is indicated to be in analogWrite() mode, first remove it from the PWM peripheral, to re-enable GPIO register control.

Why I said "CAUTION"

Adafruit BSP implementation

Calling analogWrite() loops through all the hardware PWM peripherals in the system, until one of them has an empty channel slot. When found, the pin is added to that PWM peripheral (each PWM peripheral can output on 4 pins).

nRF52840 background

Once a pin is assigned to a peripheral, the nRF52840 datasheet says it should not be used for general-purpose IO, and in fact, that the peripheral's use of the pin should automatically prevent normal GPIO port registers from being able to affect what the pin is outputting.

Therefore ...

If you cause a pin to be assigned to a peripheral (e.g., PWM), and then find some way to cause the pin to un-assign from the peripheral through an unintended manner, the chip might end up in a bad state.

Importantly ...

I'm not sure whether changing the pin's IO state is a documented (in which case it wouldn't mess with chip state), undocumented (in which case Nordic would have to weigh in ), or "undefined behavior" result.

But, I think I have a low-perf impact way to resolve it, so let me give that a go first....

[pyro9]

I was thinking that switching a pin back to digital I/O would involve calling removePin on the correct HardwarePWM object (in practice on each object until one of them returns true), wherever that operation takes place. (see cores/nRF5/HardwarePWM.cpp) That will need testing to make sure the nRF52840 responds appropriately.

I see that on other devices, the mode switch is done unconditionally in digitalWrite, but that's because it's a much quicker operation on that hardware.

[henrygab]

Update:
I can confirm that, when a pin is connected to a peripheral (such as PWM), that normal GPIO control is disconnected. Therefore, given the design used in this BSP, it will be necessary to track whether the pin is being used for PWM via analogWrite(), and then to check in digitalWrite() if it's first needed to remove the pin from the PWM peripheral.

@JelmerT -- Is this blocking a project for you? Do you have a deadline to meet?

If so, I will prioritize just this one aspect, to unblock you. If not, I would prefer to fix a few issues related to PWM usage at once.

[JelmerT]

@henrygab I'm using a workaround right now, which works, just isn't pretty.
I'm all for fixing more issues at once, as long as it gets addressed. Thanks for checking!

[henrygab]

I was thinking that switching a pin back to digital I/O would involve calling removePin on the correct HardwarePWM object (in practice on each object until one of them returns true), wherever that operation takes place. (see cores/nRF5/HardwarePWM.cpp) That will need testing to make sure the nRF52840 responds appropriately.

I see that on other devices, the mode switch is done unconditionally in digitalWrite, but that's because it's a much quicker operation on that hardware.

Yes, you are correct. The more interesting part is reducing the overhead. After all, it would be unfortunate if every call to digitalWrite() added an additional 300 clock cycles.

The solution I'm looking at generally follows this pattern:

• uint16_t array[] -- stores, on a per-nRF pin basis, the PWM channel that the pin is assigned to.
  • A value of 0xFFFF indicates the nRF pin was not assigned to a PWM channel by analogWrite().
  • A channel indicates both the HwPWM instance and the channel of that HwPWM instance that is used for the nRF pin.
  • Tracking is by nRF pin because multiple Arduino pin numbers map to a single PORT.PIN (nRF pin)
• analogWrite()
  • if nRF pin is NOT yet assigned to PWM channel, then attempt to do so and update the assigned channel in the array
  • normal operation
• digitalWrite()
  • if nRF pin IS assigned to a PWM channel, then first detach that nRF pin and update array
  • normal operation
• pinMode()
  • TBD -- need to examine for compatibility, but generally will be similar to above

Using this concept, the overhead should be fairly minimal in most cases:

• load from array a value
• test the value
• branch to normal operation

Of course, in the other branch path, there will be overhead as the pin is attached or detached from a PWM instance. But, this is necessary and not as performance-critical.

I welcome any additional thoughts on this... I've in the midst of putting a prototype for the above together now.

[hathach]

Thanks everyone for helpful discussion, since the scenario switching between gpio <-> pwm is not used that much. I like to keep the solution as simple as possible, suggest to call pinMode() again to unbind the pin from HWM module e.g

analogWrite(LED_GREEN,128);
pinMode(LED_GREEN, OUTPUT); // call hwm removePin() under the hook if needed.
digitalWrite(LED_GREEN,LOW);

since pinMode() is not called that often, this won't affect other usage. This is somewhat needed when user want to switch from PWM to other mode such as Input.

[henrygab]

@hathach -- I respectfully disagree.

Given the following general goals for a BSP:

1. Arduino API documentation defines "correct" behavior
2. Arduino boards show examples of "correct" behavior
3. Sketches written to the Arduino API should be compatible across BSPs, with only changes to the pin numbers being necessary

This issue clearly documents a place where the AdaFruit nRF52 BSP deviates from these goals. For example, please consider the following sample sketch.

---

Click to expand the sample sketch

#define ANALOG_WRITE_PIN_UNDER_TEST 5
void setup() {
  // initialize digital pin LED_BUILTIN as an output.
  pinMode(LED_BUILTIN, OUTPUT);
  pinMode(ANALOG_WRITE_PIN_UNDER_TEST, OUTPUT);
}
// the loop function runs over and over again forever
void loop() {
  digitalWrite(LED_BUILTIN, HIGH);   // turn the LED on (HIGH is the voltage level)
  analogWrite(ANALOG_WRITE_PIN_UNDER_TEST, 128);  
  delay(20);                       // wait for a second
  digitalWrite(LED_BUILTIN, LOW);    // turn the LED off by making the voltage LOW
  digitalWrite(ANALOG_WRITE_PIN_UNDER_TEST, 0);  
  delay(20);                       // wait for a second
}

---

This sample sketch works fine on, for example, the Arduino Nano, but silently breaks when moved to an nRF52 board using the Adafruit BSP.

---

Click to expand behavioral differences

This sketch uses the Arduino APIs exclusively, and thus should work the same on all boards with the necessary hardware (PWM output via analogWrite() on a pin).

For example, on an Arduino Nano, this sketch does exactly what the author intended:

• when the LED pin is HIGH, the PWM pin outputs a 50% duty cycle PWM pulse
• when the LED pin is LOW, the PWM pin is also LOW

When this same sketch is used on the nRF52 series, the compilation succeeds without warnings. Running the sketch (even if compiled at high debug level) outputs no warnings. Unfortunately, the behavior is markedly different:

• The PWM pin outputs the 50% duty cycle PWM always ... it never turns off

---

Therefore, because existing Arduino boards allow the mixing of calls to analogWrite() and digitalWrite(), the AdaFruit nRF52 BSP should also support this Arduino-API-level behavior.

Every BSP-specific workaround cost that is externalized to users makes the Arduino platform less user-friendly. Moreover, because this bug is SILENT (neither compiler nor runtime warnings), not fixing this in the BSP seems like it will cause random confusion for users ... with ZERO indication of why this is failing only on the nRF52 boards.

@hathach -- Are you sure the right decision here is to force changes to existing sketches?

Also, I already have a solution for this in mind and implementation started, with very low runtime overhead. I also was going to fix the undocumented failures that can occur from sketches using more than one PWM-related API (e.g., Tone, Servo, analogWrite, ...). If you don't want these issues fixed, please let me know that also.

[hathach]

@henrygab thank you very much for your willing and time spent for thought and work on this issue. I am more than happy if you or anyone would like to spent your precious time on any issues. Though I don't want people spending too much time implement feature that 90% people won't use. If you already started the work, please continue as you please, just to make sure to keep it simple, even we have to break the API convention like my pinMode() suggestion, people will be able to google it to this issue.

For the PWM usage conflict, we could discuss more on #459, I have also have a simple solution that is not 100% guarantee to work as well :D.