digitalWrite() doesn't override analogWrite()

JelmerT commented 4 years ago

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 commented 4 years ago

pyro9 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@hathach -- I respectfully disagree.

Given the following general goals for a BSP:

Arduino API documentation defines "correct" behavior
Arduino boards show examples of "correct" behavior
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 commented 4 years ago

@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.

henrygab commented 4 years ago

@pyro9 , @JelmerT --- Just an FYI that I've not forgotten. Rather, I was requested to keep this separate from other PRs, which is still pending. To reduce merge issue, I am waiting for those outstanding PRs to finalize.

Click to expand a subtle issue

In case you decide to code a fix yourself, ensure that you do **_NOT_** rely on the Arduino pin number as a unique identifier. The same underlying chipset pin is referred to by more than one Arduino PIN # in variant.cpp. The simplest examples are the pin numbers for analog pin definitions ... they are not #define'd to the same Arduino pin number as the digital pin, but rather have their own distinct pin number in the variant.cpp files. I don't understand why this was designed this way, so won't say it should be changed, but it definitely is unexpected. Thus, Arduino can use two apparently-distinct pin numbers, both attempting to use the same underlying pin on the chip. Obviously, this can be very confusing for at least novice Arduino users. It also could cause issues if you thought you could track pin usage without converting through the g_ADigitalPinMap array.

shtarbanov commented 4 years ago

Is there any progress on this? I have tried the suggestion with adding the pinMode() line but it didn't work. I tried removePin() but that isn't found - I am on nrf52832. I just need a way to turn off the PWM and go to digital output, even if it wastes a lot of clock cycles. It would be great if you can offer an example with code.

I really like the idea of having PWM as a pinMode parameter. This would make my life so much easier.

henrygab commented 4 years ago

Blocked waiting for @hathach to approve/merge #496

hathach commented 4 years ago

@henrygab if you plan to work on this issue, please put a short summary here first, I don't want you to spend too much of your time to fix an edge case that 90% of user won't use. What i suggested here is sufficient and easy enough to implement https://github.com/adafruit/Adafruit_nRF52_Arduino/issues/447#issuecomment-601598316

I really like the idea of having PWM as a pinMode parameter. This would make my life so much easier.

It won't be implemented since it is not an Arduino standard to use pinMode.

henrygab commented 4 years ago

@henrygab if you plan to work on this issue, please put a short summary here first

Problem Summary: Arduino API allows arbitrary ordering of calls to analogWrite() and digitalWrite(). nrf52 BSP, because it offloads analogWrite() to PWM peripheral, must track and detach from PWM when caller then uses digitalWrite(), else the digital write will be hidden until the pin is detached from the PWM peripheral.

Solution summary (very similar to prior answer):

[ ] Add a uint8_t array[NUMBER_OF_PINS]
- Stores, on a per-nRF pin basis, an indication that the pin was last used by HwPWM
- Define in hardwarePWM.h, instantiate in digitalWrite()
[ ] HwPWM addPin() and removePin() will unconditionally write the values 1 and 0, respectively
[ ] digitalWrite()
- Will read the value from the array for the current nrfPin
- Will avoid branch when value is 0, using attribute __builtin_expect(..., 0)
- else, will jump to code that removes the pin from all HwPWM instances, prior to continuing normal path

Benefits of this solution:

Performance of digitalWrite() will be minimally impacted (read, optimized branch for normal case)
Callers can then use digitalWrite() and analogWrite() in any sequence

Explicitly out-of-scope:

General code that prevents use of a pin by more than one peripheral at a time.
In other words, if sketch calls tone() after a call to analogWrite() for the same nrfPin, they get the same (likely bad) result as would occur today.

henrygab commented 4 years ago

@shtarbanov -- A quick (100% untested) prototype is available for code review.

No testing has been done, this is strictly a prototype. However, if you do run it, and it works, I'd love to know... Especially if you compile with debug level 2 and see output prefixed with "DWRI" on the com port.

hathach commented 4 years ago

sigh, I really prefer a short summary, I am too lazy on both reading & writing English. I guess we could discuss more on your PR.

shtarbanov commented 4 years ago

Thanks for the update. I ended up just using analogWrite() with 0 and 255 as a replacement for digitalWrite() with LOW and HIGH, respectively. But I am curious whether it would be more efficient in terms of speed and power (this is our top priority) to use the digitalWrite() instead in those two cases?

henrygab commented 4 years ago

sigh, I really prefer a short summary, I am too lazy on both reading & writing English. I guess we could discuss more on your PR.

I thought I gave a short summary?

hathach commented 4 years ago

sigh, I really prefer a short summary, I am too lazy on both reading & writing English. I guess we could discuss more on your PR.

I thought I gave a short summary?

yeah, you did, it is long summary, By my std, anything longer than 5 lines is rather long. But don't worry about it, please ignore my previous comment, I am probably too lazy

henrygab commented 4 years ago

@JelmerT -- I closed my PR #561 (which I think had a working solution) as hathach did not like my solution.

If you still want this fixed. please ping hathach.

hathach commented 3 years ago

If you really want this and couldn't wait for other to implement, try to fix it yourself and submit PR to help others.

Montvydas commented 1 day ago

Thanks for the update. I ended up just using analogWrite() with 0 and 255 as a replacement for digitalWrite() with LOW and HIGH, respectively. But I am curious whether it would be more efficient in terms of speed and power (this is our top priority) to use the digitalWrite() instead in those two cases?

@shtarbanov I measured the PWM running the background uses 670 uA, even if you do analogWrite(LED, 0)! So if energy consumption is of interest, this won't do. I found a quick workaround for this however, but it might not work for everyone. All you need to do is disabling the PWM timer, after that any digitalWrite() function will start working :) Give this a try:

void loop() {
  analogWrite(LED, 127); // Hello from PWM World!
  delay(1000);
  NRF_PWM0->ENABLE = 0; // Turns off the PWM on ALL pins... LED is now LOW.

  digitalWrite(LED, HIGH); // Back in ON/OFF World!
  delay(1000);
  digitalWrite(LED, LOW);
  delay(1000);
}

Since I only needed to control either all LEDs together or all LEDs are off, this did the job.

adafruit / Adafruit_nRF52_Arduino

digitalWrite() doesn't override analogWrite() #447