Skip to content

Full implementation of AnalogWrite #6544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
VojtechBartoska opened this issue Apr 6, 2022 · 19 comments
Closed

Full implementation of AnalogWrite #6544

VojtechBartoska opened this issue Apr 6, 2022 · 19 comments
Labels
Type: Feature request Feature request for Arduino ESP32

Comments

@VojtechBartoska
Copy link
Contributor

VojtechBartoska commented Apr 6, 2022

Current implementation of AnalogWrite is based on LedC Peripheral which has some limitations.

This issue tracks possible full implementation of AnalogWrite.

Related links

@VojtechBartoska VojtechBartoska added Type: Feature request Feature request for Arduino ESP32 Status: Needs investigation We need to do some research before taking next steps on this issue labels Apr 6, 2022
@dsyleixa
Copy link

dsyleixa commented Apr 7, 2022

Just to make it absolutely clear what "full implementation of analogWrite() " would mean, acc to Arduino API definitions:

analogWrite()
Description

Writes an analog value (PWM wave) to a pin. Can be used to light a LED at varying brightnesses or drive a motor at various speeds. After a call to analogWrite(), the pin will generate a steady rectangular wave of the specified duty cycle until the next call to analogWrite() (or a call to digitalRead() or digitalWrite()) on the same pin.

  • In addition to PWM capabilities on the pins noted above, the MKR, Nano 33 IoT, and Zero boards have true analog output when using analogWrite() on the DAC0 (A0) pin.
    ** In addition to PWM capabilities on the pins noted above, the Due has true analog output when using analogWrite() on pins DAC0 and DAC1.

You do not need to call pinMode() to set the pin as an output before calling analogWrite().
The analogWrite function has nothing to do with the analog pins or the analogRead function.

ref.: https://www.arduino.cc/reference/en/language/functions/analog-io/analogwrite/

@bperrybap
Copy link
Contributor

bperrybap commented Apr 7, 2022

I would go further than @dsyleixa and say that analogWrite() is not really implemented until it works like it is specified and the way it works on other platforms.
i.e. just because there is some sort of partial implementation, it doesn't mean that it is actually implemented.

In addition to what @dsyleixa mentioned, there must be a macro digitalPinHasPWM(pin) that exists (as a macro not a function) that returns true if the pin supports analogWrite()
i.e. if the macro does not exist or the macro returns false the variant does not support AnalogWrite() for PWM on the given pin.
This is how code can tell a compile time if pwm on a pin is supported.
This allows code to compile time modify itself to deal with pwm capabilities in a portable way.

The problem in the past is that there was a digitalPinHasPWM(pin) macro that would always return true for all pins but the function analogWrite() didn't even exist at all which meant that user code that went to the effort to check for analogWrite() working would fail to compile due to misimplementation of the digitalPinHasPWM(pin) macro in all the variant header files.

This capability is trivial to implement correctly and can be implemented regardless of what/how the analogWrite() is implemented.
i.e. if the analogWrite() is not properly implemented as specified by arduino.cc, just always false.

I would also say that if there are potential failures of analogWrite() then analogWrite() should be changed from a void function to returning an integer status of zero if successful and non zero status like < 0 if there is a failure.
While this is a deviation from the standard API it would still be compatible with the API as existing code would not ever be looking at return value.

@dsyleixa
Copy link

dsyleixa commented Apr 7, 2022

@bperrybap :
as to

I would also say that if there are potential failures of analogWrite() then analogWrite() should be changed from a void function to returning an integer status of zero if successful and non zero status like < 0 if there is a failure.
While this is a deviation from the standard API it would still be compatible with the API as existing code would not ever be looking at return value.

I do understand your intentions and consider it to be a precious feature, but actually dare to disagree though:
As the original Arduino does not t provide a return value it may lead to further incompatibilities in case Arduino.cc once might provide a similar feature but then implement it in a different way (e.g., return 0 if there is a failure and 1 if it works)

Having said that I would suggest to discuss that issue with the Arduino development team to create a feature wich still keep cross-platform compatibility for future developments und API function upgrades.

Plus to mention the DAC thing about

true analog output when using analogWrite() on pins DAC0 (...)

@bperrybap
Copy link
Contributor

@dsyleixa
I get your point on portability and I am HUGE proponent of maintaining consistency.
I've had a few discussion with arduino.cc developers over the past 10 years with respect to analogWrite() and they have never been open to making any changes. So I don't intend to go down that path again.

Perhaps there are some alternatives to a return value that could work and yet still live within the existing APIs
If you look at the current analogWrite() implementation, there is a magic number of pwm pins supported.
If that is exceeded, the code silently fails and does nothing. To me that is a big issue.
(yeah there is an error log but user sketch code has no way of knowing that there was a failure)

One way work around this silent failure issue would be if the macro digitalPinHasPWM(pin) were made smart enough to not only look to see if the pin was capable of pwm but also take into consideration any s/w issues/limitations in the analogWrite() implementation, including any real-time issues.
i.e. digitalPinHasPWM(pin) returns false if immediately calling analogWrite() could not support pwm.
And then in analogWrite() if pwm could not be supported revert back to the non pwm pin behavior defined by arduino.cc
For non pwm pins the pin is set to LOW if the analogWrite() value is below half the maximum and HIGH otherwise.

This would resolve the silent failure issue without adding a return value to analogWrite()

Although, IMO, I don't care for no pwm pin behavior that Arduino.cc defined.
IMO, it makes more sense to set the pin to LOW when the analogWrite() value is zero and HIGH for any non zero value.
The reasoning for this is because it is implementing the dimmest it can do.
If PWM is not supported, then full on is the dimmest it can do.

@me-no-dev
Copy link
Member

@bperrybap all of those "silent fails" will be fixed for 2.1.0 through a new internal module that will manage the different peripherals and their functions, thus providing PWM through more than just LEDC and adding more options for different output modes (servo, etc.)

@dsyleixa
Copy link

dsyleixa commented Apr 8, 2022

@bperrybap
yes, I agree about the stubbornness of the Arduino.cc development team. Nonetheless compatibility is the crucial demand, and so I must speak out against any intentional change to the original API features.
OTOH, the pwm capabilities are not "magic" at all, they are defined and published in the board specs:
Uno, Nano, Mini 2-13
Arduino Mega 2- 3, 44-46
Leonardo, Micro, Yún 3, 5, 6, 9, 10, 11, 13
Arduino Due 2-13
Nano 33 IoT 2, 3, 5, 6, 9 - 12, A2, A3, A5

so anyone who wants to know just will have to take a look at the specs and manuals: https://www.arduino.cc/reference/en/language/functions/analog-io/analogwrite/

OTOH, you are not forbidden to create a new function
bool pinFeatPwm(int pin) // or whatever
which returns the uncertain capability for checking purposes.

@bperrybap
Copy link
Contributor

bperrybap commented Apr 8, 2022

@dsyleixa
I'm not sure what you meant by the need to look at specs/manuals and the creation of a new function.
A new function to do what?
There already is a portable way to detect PWM capabilities for any/all pin(s) that works today.
There is no requirement to look anything up or have to manually hard code things for each board or to create a new function to be able to tell if a pin supports PWM.

Each variant is supposed to provide the appropriate macros/defines to ensure this works.

Here is a portable sketch that will print out all the pins that support PWM / analogWrite() for any "duino" board.

void setup(void)
{
int pin;
	Serial.begin(115200);
	delay(1000);

	Serial.println();
	Serial.print("Total Digital Pins: ");
	Serial.println(NUM_DIGITAL_PINS);
	Serial.print("Pins that support PWM: ");
	for(pin = 0; pin < NUM_DIGITAL_PINS; pin++)
	{
		if(digitalPinHasPWM(pin))
		{
			Serial.print(" ");
			Serial.print(pin);
		}
	}
	Serial.println();
}

void loop(void){}

Running this sketch on a board should print out the pins that support PWM
This is portable code what should work on every arduino board, including all 3rd party "duino" boards.

And since both NUM_DIGITAL_PINS and digitalPinHasPWM() are macros they can even be used by compile time conditionals to control what code is compile in.

The problem with the ESP32 core (as of right now with the latest version 1.0.6) is that digitalPinHasPWM() says all pins support PWM but yet it has no analogWrite() function. (I mentioned this in the other analogWrite() issue)
No other core has this issue.
This breaks any code that may want control a built in LED and uses compile time checks to see if the built in LED exists and if it supports dimming.
i.e. the macro LED_BUILTIN exists (means there is a built in LED and pin LED_BUILTIN controls it) and digitalHasPWM(LED_BUILTIN) indicates that PWM is supported but yet analogWrite() does not exist in the esp32 core so code that tried to use a portable way to detect things breaks, does not even compile, with variants in the esp32 core.

And here is example sketch that will blink a built in LED.
If the pin that controls the LED supports pwm it will soft blink the LED.
If the built in led pin does not support pwm it will hard blink (simple on/off)
When a platform core and variant have the proper macros and core functions, then this code will compile and work
As an example of what this does. If you run it on an UNO it will hard blink the led and if you run it on the Mega board it will soft blink the board.
Currently, as of this post using the latest 1.0.6 board package, it won't even compile on the esp32 platform due the variants indicating that PWM is supported on the built in LED pin but yet there no analogWrite() function in the core.

void setup(void)
{
	Serial.begin(115200);
	delay(1000);
	Serial.println();
#if defined(LED_BUILTIN)
	Serial.print("Board has Built in LED on pin: ");
	Serial.println(LED_BUILTIN);
#if digitalPinHasPWM(LED_BUILTIN)
	Serial.println("Built in LED supports PWM");
#else
	Serial.println("Built in LED does not support PWM");
	pinMode(LED_BUILTIN, OUTPUT);
#endif
#else
	Serial.println("Board has no built in LED");
#endif
}

void loop(void)
{
	blinkled(LED_BUILTIN, 3); // soft blink or hard blink the built in led depending on capabilities.
	delay(1000);
}

// helper macros to turn on built in LED as some boards like the ESP8266 use
// active low LEDs.
// For years I have been unable to get Ardino.cc to be interested
// in providing a portable way to turn on/off the built in LED.
// at a minimum an active level macro needs to be configured by the variant header

#if defined(ARDUINO_ARCH_ESP8266)
#define ledBuiltinOn() digitalWrite(LED_BUILTIN, LOW)
#define ledBuiltinOff() digitalWrite(LED_BUILTIN, HIGH)
#else
#define ledBuiltinOn() digitalWrite(LED_BUILTIN, HIGH)
#define ledBuiltinOff() digitalWrite(LED_BUILTIN, LOW)
#endif


void blinkled(int led, int numblinks)
{
#if defined(LED_BUILTIN)
	// If possible, soft blink the led by ramping up then back down
	for(int times = 0; times < numblinks; times++)
	{
		for(int x = 1; x < 16; x++)
		{
#if digitalPinHasPWM(LED_BUILTIN)
			analogWrite(LED_BUILTIN, x * 16);
#else
			ledBuiltinOn();
#endif
			delay(25);
		}
		for(int x = 1; x <= 16; x++)
		{
#if digitalPinHasPWM(LED_BUILTIN)
			analogWrite(LED_BUILTIN, 256-x * 16);
#else
			ledBuiltinOff();
#endif
			delay(25);
		}
	}

#else
	// No built in LED so do nothing
#endif
	
}

IMO, these two sketches are useful to verify the platform core and variant for the board are properly implemented.

@dsyleixa
Copy link

dsyleixa commented Apr 8, 2022

There already is a portable way to detect PWM capabilities for any/all pin(s) that works today.

ok, I was missing that, because I never needed it actually - as I always knew all the pins which had pwm or perhaps not.

@drf5n
Copy link

drf5n commented Mar 5, 2023

That PWM pin detecting sketch works as expected on my ESP32:

Total Digital Pins: 40
Pins that support PWM:  0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33

The other sketch and the basic Blink sketch from the Arduino IDE 2.0.4 doesn't work for board = "ESP32 Dev Module" because LED_BUILTIN isn't defined.

It does work with the Arduino IDE 2.04 with the "esp32 by Espressif Systems Version 2.0.7" and the "DOIT ESP32 DEVKIT V1" board:

Board has Built in LED on pin: 2
Built in LED supports PWM

@VojtechBartoska VojtechBartoska added this to the 3.0.0 milestone Mar 9, 2023
@VojtechBartoska VojtechBartoska added the Status: Awaiting triage Issue is waiting for triage label Mar 9, 2023
@VojtechBartoska
Copy link
Contributor Author

Adding this to 3.0.0 milestone to evaluate this again.

@dsyleixa
Copy link

by chance I found this Arduino lib:
https://github.com/ERROPiX/ESP32_AnalogWrite

will it work with all ESP32 repo versions? has anyone checked it?

BTW,
finally the Basics Fade.ino example does not compile at all, and I always work with core 1.06 because of major code incompatibilities since 2.0 ff.

@iseanstevens
Copy link

Also, perhaps a full implementation of AnalogWrite would help make the standard Servo library work again?

Seems like it broke around 3.0. Sorry for the poor bug report but +1 for AnalogWrite PWM :)

@TD-er
Copy link
Contributor

TD-er commented Oct 5, 2024

@iseanstevens You can also have a look at the changes I made here: https://github.com/letscontrolit/ESPEasy/tree/mega/lib/ServoESP32

@me-no-dev
Copy link
Member

Also, perhaps a full implementation of AnalogWrite would help make the standard Servo library work again?

Servo library should use RMT on ESP32 chips, not PWM. This issue is more to track being able to use different peripherals (not just LEDC) to output PWM. analogWrite() itself is working as it should

@me-no-dev
Copy link
Member

@sivar2311 Servo uses 50Hz PWM with duty between 5% and 10%. Such things are much better handled with RMT than analogWrite. There is nothing preventing you from using analogWrite, it's just not optimal

@TD-er
Copy link
Contributor

TD-er commented Oct 7, 2024

@me-no-dev How many RMT channels can you address?
For a single servo it could make perfect sense to use RMT as its timing is hardly unbeatable.
But if you can only handle a single servo using RMT, then it can be a bit limiting.
Or are you thinking about quickly switching pins between sending pulses?

@me-no-dev
Copy link
Member

@TD-er you can use as many as there are TX RMT channels on the chip (it differs from chip to chip). You set it to output continuous for every channel and change as needed. vanilla ESP32 has 8 channels

@rftafas rftafas modified the milestones: 3.1.0, 3.1.1 Jan 6, 2025
@rftafas rftafas removed this from the 3.1.1 milestone Jan 14, 2025
@Parsaabasi Parsaabasi removed the Status: Awaiting triage Issue is waiting for triage label Jan 16, 2025
@Parsaabasi Parsaabasi removed the Status: Needs investigation We need to do some research before taking next steps on this issue label Jan 16, 2025
@Parsaabasi
Copy link

Hello,

Due to the overwhelming volume of issues currently being addressed, we have decided to close the previously received tickets. If you still require assistance or if the issue persists, please don't hesitate to reopen the ticket.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature request Feature request for Arduino ESP32
Projects
Development

No branches or pull requests

10 participants