espressif / arduino-esp32

Arduino core for the ESP32
GNU Lesser General Public License v2.1
13.2k stars 7.34k forks source link

analogWrite() not implemented #4

Closed vshymanskyy closed 2 years ago

wouterds commented 4 years ago

@wouterds @jandolina I have created a good wrapper library to work with analogWrite https://github.com/ERROPiX/ESP32_AnalogWrite you can install it on both Arduino IDE and PlatformIO

Thanks, that does seem to work! For a bit. I'm not sure but it looks like there's a memory leak? I'm writing to 4 pins an analog value every loop and I see free memory draining rapidly until it crashes and reboots.

Edit: also can't your port it / make a PR here so it's included by default for everyone? @ERROPiX

dsyleixa commented 4 years ago

as stated, analogWrite() is crucial in the Arduino API world for cross-platform-compatibility, so please provide it for ESP32, too, ASAP. As usual, pwm frq should be somehow between 500Hz...1kHz preferably.

sdaitzman commented 4 years ago

Is there something unusually difficult/on hold about implementing this? Not being facetious, just trying to understand the issue.

skickar commented 4 years ago

thank you for this, I've been receiving emails from this thread for the last several months and this is the only one that's made me any less angry. Also, if it wasn't clear, my vote is for implementing analog write.

On Tue, Jan 14, 2020, 1:50 AM Wayne Keenan notifications@github.com wrote:

Power is red Radio is blue I can’t dim the LED Analogwrite, I miss you

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/espressif/arduino-esp32/issues/4?email_source=notifications&email_token=AJTC7HO3HD76NST6V3F2RE3Q5WDFHA5CNFSM4CSCDMFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI37GFY#issuecomment-574092055, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJTC7HOR35JBFWWIPANHBU3Q5WDFHANCNFSM4CSCDMFA .

wouterds commented 4 years ago

I briefly looked into this to see if I could copy it from @ERROPiX his repository but I have too little C/C++ knowledge (and this repository looks quite complex). But there is an open PR (https://github.com/espressif/arduino-esp32/pull/3110). Would be cool if @magicblocks & @me-no-dev could finish this up or if someone else could pick it up.

me-no-dev commented 4 years ago

it needs to work in the following way:

  1. find available PWM peripheral (LEDC, MCPWM, SigmaDelta, RMT, Soft?)
  2. Make sure that the clock requirements will not interfere with other used ports on the same peripheral (LEDC and others share some clocks)
  3. Mark the selected port as used and save it's usage type
  4. Do not forget to free the port if a call to some other pin function is made. Now this is the big issue here... means that any time you call pinMode and others, have to check if that pin is attached to port and so on.
  5. All of that functionality must be tracked by each peripheral as well (so you can use LEDC or other and still use analogWrite)

The lib above does not take any of those considerations. It's just not that simple :) But I already have some code written towards that goal.

tharangachaminda commented 4 years ago

for Arduino IDE there is a library and it worked for me perfectly for motor controller project. https://www.arduinolibraries.info/libraries/esp32-analog-write

dsyleixa commented 4 years ago

perhaps someone of the devs could test it and make a fork to integrate it to the common ESP32 standard Arduino core libs?

bperrybap commented 4 years ago

Ok, so I just ran into this issue today. Not having an analogWrite() function breaks my LCD library. (hd44780) The hd44780 library currently works on EVERY other Arduino platform/core except this one. This includes the esp8266 library. While the hd44780 library code is smart enough to use digitalPinHasPWM() to detect if PWM support exists on the pin, the code still doesn't work (won't even compile) on this platform because of a call to analogWrite() which does not exist. Users should not have to muck up their code with platform/core specific conditionals to work around a missing Arduino core library function.

Given that analogWrite() is a core API function, it should always exist in this platform. Until analogWrite() is properly implemented, which I believe is possible, my suggestion would be to add an analogWrite() function but not support PWM. The Arduino.cc analogWrite() function already handles this for pins that do not support PWM.

For the esp32 analogWrite() with PWM support disabled, it would mean treating all pins as if they do not support PWM, which technically would be correct since they don't have PWM capability using analogWrite() since the underlying code for PWM is not yet implemented. To do this:

In each variant's pins_arduino.h header file change the digitalPinHasPWM() macro to something like this:

define digitalPinHasPWM(p) (0) // analogWrite() does not yet support PWM

And for the matching analogWrite() function:

void analogWrite(uint8_t pin, int val)
{
        if(val < 128)
                digitalWrite(pin, LOW);
        else
                digitalWrite(pin, HIGH);
}

Making these changes/updates will allow existing code that calls analogWrite() to function as it does on all the other platforms. It also allows the code to use digitalPinHasPWM() to detect if PWM is supported on the specified pin.

IMO, this is a very small/simple change that resolves the main issue of analogWrite() not existing which causes any code that attempts to use analogWrite() to fail to compile.

At some point a proper analogWrite() that supports PWM when possible, should be added to the core

infinity-computers-dot-net commented 4 years ago

Ok, so I just ran into this issue today. Not having an analogWrite() function breaks my LCD library. (hd44780)

You're wasting your breath. This core has been broken for almost four years, and the maintainers clearly have no intention of fixing it. They have ignored every attempt to address this problem, holding out for some mythical "perfect solution" that does not exist; instead of just damn well choosing one of the available options, which would be infinitely better than the absolute lack of functionality we currently have.

All I can suggest is that you fork your own, and recommend that your users switch to using that.

skickar commented 4 years ago

Pretty much the only joy that exists in this thread is irritating the cowards responsible for not implementing analogwrite and attracting thumbs downs from nerds who like excuses better than making cross-platform code.

FedericoBusero commented 4 years ago

There is a basic implementation of analogWrite available in the Arduino IDE: simply install the ESP32Servo library (the one in the Arduino IDE is the correct one, it is https://github.com/madhephaestus/ESP32Servo ), and add

#include <analogWrite.h>

It has another approach then other libraries: it has in one implementation the Servo library, analogWrite and the Tone library. This makes it possible to have a shared channel management for the 3 different things (servo, analogWrite and tone). A user doesn't need to take care of all this (as long as you don't use timers for other purposes, such as a camera).

Remarkt that you have to use values 0 .. 255 in analogWrite. In case you want to develop cross-platform with ESP8266, use PWMRANGE.

It demonstrates that it is possible to have an implementation of PWM functions compatible with other Arduino's, you just need a common channel manager, which is not yet available.

dsyleixa commented 4 years ago

thanks for your link! Looks really awesome!

AlanGreyjoy commented 3 years ago

four years later.... sponge bob

uonliaquat commented 3 years ago

@FedericoBusero you saved life

Harvie commented 3 years ago

@deadoralive023 hey! please don't use words like "finaly" in this thread unless this is merged upstream. i almost fainted of overexcitement... in vain.

uonliaquat commented 3 years ago

@deadoralive023 hey! please don't use words like "finaly" in this thread unless this is merged upstream. i almost fainted of overexcitement... in vain.

I'm new so Thanks for letting me know.

Harvie commented 3 years ago

I think after few more years in this (still unresolved) thread you will understand :-D

dsyleixa commented 3 years ago

@developers: what the heck is the problem to merge this lib https://github.com/madhephaestus/ESP32Servo into the standard esp32 core libs?

Harvie commented 3 years ago

@dsyleixa Basicaly the developers:

image

Harvie commented 3 years ago

Meanwhile the users:

image

atanisoft commented 3 years ago

what the heck is the problem to merge this lib https://github.com/madhephaestus/ESP32Servo into the standard esp32 core libs?

@dsyleixa The problem with analogWrite() is almost all usages center around using the LEDC peripheral. There is no problem if you are using just analogWrite() and none of the libraries are using the LEDC peripheral. If you allocate LEDC channel 1 for analogWrite(32, XXX) and a library also uses LEDC channel 1 for a different pin you will now have conflicts between the two usages. That is where the problem is coming from on why this has not been resolved yet.

@me-no-dev had mentioned a reservation like system for the LEDC channels at one point but that has not been exposed in the public API as of now.

dsyleixa commented 3 years ago

atanisoft: WHAT? analogWrite is an elementary common standard Arduino command and so it has to be expectedly available in the standard core libs, just like on all other Arduino boards!

atanisoft commented 3 years ago

analogWrite is an elementary common standard Arduino command

Fully agree with you there, it is part of the standard API.

The problem as I mentioned above is which peripheral should be used for generating the output signal that doesn't impact the usability of the peripherals in a negative manner. That is the problem that needs to be resolved before analogWrite() can be included in the base repository.

bperrybap commented 3 years ago

The problem as I mentioned above is which peripheral should be used for generating the output signal that doesn't impact the usability of the peripherals in a negative manner. That is the problem that needs to be resolved before analogWrite() can be included in the base repository.

@atanisoft how is that really a problem? There will often be issues when code needs to use certain resources to implement something. That isn't a valid reason to not implement something. You just pick something reasonable and move on and if necessary document the limitations and/or how it can be configured. These same types of issues exist on the AVR platform when using analogWrite() and I'm sure other platforms as well. How did they handle this on the esp8266?

As I said before, analogWrite() and digitalPinHasPWM(p) should ALWAYS exist. Worst case if the developers don't want to implement pwm (which I'm still not seeing a reason why they can't), or need more time, then they could do as I showed before. Simply have digitalPinHasPWM(p) be defined at 0 and then do what other implementations do when a pin does not support pwm. It sets the pin output to LOW when the value is below 128 and HIGH otherwise. While that implementation choice of analogWrite() would not provide pwm, it does ensure that analogWrite() exists and technically "works" as documented.

To just not implement anything, is total BS, IMO.

atanisoft commented 3 years ago

how is that really a problem?

If analogWrite() is provided by the core library then every solution proposed in this issue so far would become broken, similarly any other library that happens to implement/export a function named analogWrite() will be broken as well. Perhaps it is not as much of an issue for some developers but it will cause a number of new issues to be filed which all have the same solution: upgrade/downgrade your version. It isn't ideal but maybe it is the only solution.

Simply have digitalPinHasPWM(p) be defined at 0 and then do what other implementations do when a pin does not support pwm. It sets the pin output to LOW when the value is below 128 and HIGH otherwise.

Technically all pins can have PWM except 34-39 which are input only. So the problem will then be, which eight pins should be selected for output (assuming LEDC as the analogWrite implementation)? No matter which ones are selected it will be the wrong choice.

bperrybap commented 3 years ago

If analogWrite() is provided by the core library then every solution proposed in this issue so far would become broken, similarly any other library that happens to implement/export a function named analogWrite() will be broken as well. Perhaps it is not as much of an issue for some developers but it will cause a number of new issues to be filed which all have the same solution: upgrade/downgrade your version. It isn't ideal but maybe it is the only solution.

But that is really a backward compatibility issue, not an implementation issue. True, it can still be just as much of a problem, but when you have a platform/core that totally failed to implement API functions, there is no way to keep from having issues once the platform does finally implement those missing API functions. I see no way around having issues if analogWrite() is expected to "just exist" in the core library like it does in all the other platforms with no need for including a header file or a constructor.

It would have been nice the original pwm output API had been done with a separate library and used a separate object for each pin, so you could configure the pin including for things like using pwm or true analog output, but unfortunately it wasn't done that way.

Technically all pins can have PWM except 34-39 which are input only. So the problem will then be, which eight pins should be selected for output (assuming LEDC as the analogWrite implementation)? No matter which ones are selected it will be the wrong choice

Perhaps there can be a way to configure which pins are used through an API extension or adding a constructor. Kind of similar to what can done for the pins used for i2c. I.e. you have defaults that can be reconfigured.

dsyleixa commented 3 years ago

I agree, also on Uno or Nano or Mega not all digi Pins provide pwm - some do, some don't. IIRC, not even the Due has them all everywhere. But it eventually must be available there somewhere.

infinity-computers-dot-net commented 3 years ago

At the most basic, you could just have a #define that specifies which pins should have PWM capability. This is elementary, people!

At the absolute bare minimum, you should provide an analogWrite() that does nothing. To omit it entirely from the API is simply inexcusable, and you should be ashamed of yourselves.

skickar commented 3 years ago

You are right new guy. They should be ashamed.

I hope every other angry person on this thread is doing okay in these ghastly times, I think of you all at least once per night before I go to sleep.

Let's all hope that one win we can get out of this year is holding someone responsible for ruining our electronics project.

On Fri, Sep 25, 2020, 9:44 PM infinity-computers-dot-net < notifications@github.com> wrote:

At the most basic, you could just have a #define that specifies which pins should have PWM capability. This is elementary, people!

At the absolute bare minimum, you should provide an analogWrite() that does nothing. To omit it entirely from the API is simply inexcusable, and you should be ashamed of yourselves.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/espressif/arduino-esp32/issues/4#issuecomment-699324844, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJTC7HK6X7QC7AHQK33ZLCDSHVPSVANCNFSM4CSCDMFA .

infinity-computers-dot-net commented 3 years ago

Sarcasm is the lowest form of wit. In addition, you have just committed the fallacy of relative privation.

If you implement an API, you are obliged to implement the whole API, or else you break compatibility with everything.

This issue has remained unresolved for four years - most of which time was covid-free - when at the very least, they could have bare-minimum "solved" it with a dummy function. And if the number of posts in this thread are any indication, this omission affects a large number of users. So yes, they should be ashamed.

skickar commented 3 years ago

I was agreeing with you nerd, we're all a little tense on this thread.

On Sat, Sep 26, 2020, 8:46 AM infinity-computers-dot-net < notifications@github.com> wrote:

Sarcasm is the lowest form of wit. In addition, you have just committed the fallacy of relative privation.

If you implement an API, you are obliged to implement the whole API, or else you break compatibility with everything.

This issue has remained unresolved for four years - most of which time was covid-free - when at the very least, they could have bare-minimum "solved" it with a dummy function. So yes, they should be ashamed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/espressif/arduino-esp32/issues/4#issuecomment-699505051, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJTC7HJV6Q4R5EGUMYLKTXTSHX5F3ANCNFSM4CSCDMFA .

alexceltare2 commented 3 years ago

Can we all calm TF down? @me-no-dev already said he's working on a proper implementation and, as he pointed out, is not as simple as creating a wrapper around Arduino functions. @ERROPiX already made something like that for y'all impatient pricks.

atanisoft commented 3 years ago

Yes, this is far from a simple solution as thought and there are a number of factors that play into the implementation that must be considered.

infinity-computers-dot-net commented 3 years ago

impatient pricks

This offensive remark contravenes the code of conduct.

atanisoft commented 3 years ago

impatient pricks

This offensive remark contravenes the code of conduct.

Agreed, this issue is rife with comments that would potentially fall afoul of the code of conduct.

Can we go back to on-topic civilized discussion of the issue(s) at hand?

dpharris commented 3 years ago

Mike Can you please explain what issues are complicating the solution? Is it management of allocation of resources, eg pins and channels etc? David

On Mon., Sep. 28, 2020, 04:35 Mike Dunston, notifications@github.com wrote:

Yes, this is far from a simple solution as thought and there are a number of factors that play into the implementation that must be considered.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/espressif/arduino-esp32/issues/4#issuecomment-699951814, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEDQSRWP4BUDF36SVHHDVDSIBYIVANCNFSM4CSCDMFA .

atanisoft commented 3 years ago

Is it management of allocation of resources, eg pins and channels etc?

That would be one of the two big issues:

  1. There are a total of 16 possible analog signal sources (LEDC has eight, Sigma-delta has another eight).
  2. These two peripherals can be controlled directly via the ESP-IDF API which will bypass the Arduino HAL APIs.

Managing the first one as part of calling pinMode(..) or analogWrite(...) is doable but the second one will invariably result in a lot of frustration and issues filed for the output on an analog pin being "wrong" due to a third-party library using the peripheral in a different manner.

There are also likely other issues which have resulted in this issue unfortunately sitting for so long.

me-no-dev commented 3 years ago

Every time I think about it, I find new issues that can arise. There is no global solution for this. Locally in sketches it's rather simple to do. You can ifdef ESP32 and define a macro. Then you need to pay attention at what libraries in your project are using (if they need one of the peripherals that can do PWM) and make sure you do not overlap.

PWM can be provided by LEDC (HS and LS), MCPWM, RMT and SigmaDelta. DAC is also analogWrite (maybe even more than PWM).

Each of those peripherals can provide other functionality as well.

me-no-dev commented 3 years ago

How did they handle this on the esp8266?

ESP8266 does not have PWM capable peripherals. I wrote a software PWM lib for it.

infinity-computers-dot-net commented 3 years ago

I'm not sure what you mean by "PWM capable peripherals" here. The ESP8266 has hardware PWM.

atanisoft commented 3 years ago

ESP8266 does not have PWM capable peripherals.

It seems to have the Sigma-delta peripheral but it is not being used for analogWrite.

The ESP8266 has hardware PWM.

It's not being used for PWM though, https://github.com/esp8266/Arduino/blob/5b767a37eb983a475fe29a8be6b489d62e833251/doc/reference.rst#analog-output

me-no-dev commented 3 years ago

SigmaDelta is not really PWM @atanisoft :) but can be used to drive motors and such. It was not documented and had few channels (not that I remember much).

@infinity-computers-dot-net it does not. PWM is done in software, using one of the hardware timers.

infinity-computers-dot-net commented 3 years ago

Oh, it's done using an interrupt!?

dpharris commented 3 years ago

We need a generalized method of indicating reservation of resources. The same problems arise with timers, many libs use them, and one has to try to not have collisions.

Could we not have an initializer that indicates the resource, and return a success/failure flag?

Eg: if( err = analogInit(pin, LEDC) ) { Serial.print("\nPin assignment failed:"); Serial.print(err); where err might indicate 'no peripheral is available' etc

Another problem is that the peripherals share the same clock base, so interactions between the peripherals.

Still, we should be able to come up with some common solution.

If you build it, they will come. Ie, future code will reap the benefit.

David

On Mon, Sep 28, 2020 at 9:05 AM Me No Dev notifications@github.com wrote:

Every time I think about it, I find new issues that can arise. There is no global solution for this. Locally in sketches it's rather simple to do. You can ifdef ESP32 and define a macro. Then you need to pay attention at what libraries in your project are using (if they need one of the peripherals that can do PWM) and make sure you do not overlap.

PWM can be provided by LEDC (HS and LS), MCPWM, RMT and SigmaDelta. DAC is also analogWrite (maybe even more than PWM).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/espressif/arduino-esp32/issues/4#issuecomment-700129932, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEDQSWHRRJN5QBPTDTBQV3SICX3PANCNFSM4CSCDMFA .

devyte commented 3 years ago

The ESP8266 has hardware PWM

No it doesn't, at least not for analogWrite().

At the ESP8266 Arduino repo we implemented a sw waveform generator as a centralized engine of pulse output channels of (mostly) arbitrary shape, and then reimplemented sw pwm, i. e. analogWrite(), on top of that.

The reason we did that is because with the prior code you couldn't mix certain libs, e. g. Servo and pwm wouldn't play nice together, or pwm would fail if you output a Tone. With the waveform generator you can mix.

There are now new versions of the waveform generator proposed, each with pros and cons. They have both undergone rather tough testing. Both PRs are viable, we'll likely keep both.

@me-no-dev I don't know if our waveform generator implementation applies to a FreeRTOS-based platform like the esp32, but if you would like to discuss, you know where to find me :)

jandolina commented 3 years ago

This thread has been amazing. Thank you all for keeping the ESP community alive. As a starter chip the ESP family is probably not the right choice. Once you understand things on the Arduino hardware and in the IDE, then you can branch out to unlimited controllers out there. My current favorite combo is VSCode + TTGO Lora 32 v2 with OLED.

Back to the topic. Do you think it would make sense to take a LED PWM library and just call it PWM so there is no confusion with the LED names? Not out of the box, but still a low bar to entry.

Harvie commented 3 years ago

The resource allocation argument is IMO invalid. This is just something that happens with ANY arduino library. If you've ever coded for Arduino UNO (atmega), you know that in such cases you have to manage resources yourself. at least to some extent, but definitely when using 3rd party libraries or directly accessing the peripherals without using the arduino wrappers.

If 3rd party code manages timers directly on atmega, it will screw arduino PWM. It's not a big concern for me. As a developer you should choose if

1.) You are beginner and want to use arduino wrappers (and therefore have to use them almost exclusively). 2.) You are advanced and want to unleash full performance of hardware by using ESP-IDF directly, but then you have to understand the consequences and possibly might loose the ability to use beginner friendly arduino layer.

No need for some magic "solve it all" layer... You will never be able to fix all edge cases anyway...

Harvie commented 3 years ago

Just choose some sensible default peripheral to provide 2 to 8 pins with PWM and add option to manualy select different peripheral if needed. Similary to the way we have multiple UARTs or SPIs and stuff like that piggybacked to the original single UART API that we had in early days of arduino. This is not perfect solution, but good enough for anyone who is willing to use Arduino API, which is not perfect anyways... If you need more than few PWM outputs, just use esp-idf OR wait 10 years before someone comes with perfect solution.

Arduino API is mostly usefull for learning, prototyping, etc... Anyone serious will use esp-idf anyways... We just need to have enough API to make all analogwrite examples shipped with arduino IDE to compile and run.

dsyleixa commented 3 years ago

People who use the Arduino IDE for either boards are 99% beginners and hobby users and mostly do not even understand neither real C++ coding nor RTOS coding, but they have the benefit that Arduino API code on a Uno also runs on a Due, Zero, SAMD51, or (hopefully) ESPs. Finally that's also basically the Arduino/Wiring philosophy. So a Arduino-API-compatibility also for the ESPs are crucial.