Skip to content
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

LEDC - AnalogWrite new API + ledcAttachPin duty fix #7346

Merged
merged 8 commits into from
Nov 11, 2022

Conversation

P-R-O-C-H-Y
Copy link
Member

@P-R-O-C-H-Y P-R-O-C-H-Y commented Oct 12, 2022

Description of Change

  1. Function ledcAttachPin() now reads duty from channel before setup in order to keep its current duty.
  2. Added new API functions for analogWrite, which sets frequency or resolution for all led channels used by analogWrite.
  • analogWriteFrequency(uint32_t freq)
  • analogWriteResolution(uint8_t bits)

Tests scenarios

Tested on ESP32.

Sketch for testing the fix with analogWrite(connect led to pin 2 - should blink with 500ms delays):

uint8_t led = 2;

void setup() 
{            
  analogWrite(led, 128);
  delay(500);
}

void loop() 
{
  ledcDetachPin(led);
  delay(500);
  ledcAttachPin(led,analogGetChannel(led));
  analogWrite(led,128);
  delay(500);
}

Related links

Closes #7105
Closes #7306

@P-R-O-C-H-Y P-R-O-C-H-Y self-assigned this Oct 12, 2022
@P-R-O-C-H-Y P-R-O-C-H-Y added the Area: Peripherals API Relates to peripheral's APIs. label Oct 12, 2022
@P-R-O-C-H-Y P-R-O-C-H-Y added this to the 2.0.6 milestone Oct 12, 2022
@PilnyTomas
Copy link
Contributor

Tested on ESP32 with following sketch and works ok - signal is not terminated

void setup() {
  uint8_t pin = 14;
  analogWrite(pin, 128);
  delay(50);
  ledcDetachPin(pin);
  delay(50);
  ledcAttachPin(pin,analogGetChannel(pin)); //added
  delay(50);
  analogWrite(pin,128);
}
void loop() {}

Copy link
Collaborator

@SuGlider SuGlider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I See you added analogWriteResolution() and analogWriteFrequency().
The first is common with some Arduino boards and the sencond is an improvement.

if (cnt_channel == LEDC_CHANNELS) {
return; //No channel used for analogWrite
}
for (int channel = LEDC_CHANNELS - 1; channel >= cnt_channel; channel--) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is any uint32_t a valid frequency? What happens if we set it to 1,000,000,000 Hz?
Should it be validated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDF is checking the frequency input and validating if its valid to set or not. Same with resolution. IDF will return error that is checked in Arduino.

Copy link
Collaborator

@SuGlider SuGlider Nov 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it with wrong valus and I got no error message.

But if I test it with IDF I get the error...
E (30) ledc: requested frequency and duty resolution can not be achieved, try reducing freq_hz or duty_resolution. div_param=11

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the comments below :)

return; //No channel used for analogWrite
}
for (int channel = LEDC_CHANNELS - 1; channel >= cnt_channel; channel--) {
ledcChangeFrequency(channel, analog_frequency, bits);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is any uint8_t a valid resolution? What happens if we set it to 128 bits?
Should it be validated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will get an error that is not valid input.

Copy link
Collaborator

@SuGlider SuGlider Nov 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it with wrong value and I got no error message.
Looking into https://github.com/espressif/esp-idf/blob/release/v4.4/components/driver/ledc.c#L558
I see duty validation, but it doesn't fail...

But if I test it with pure IDF I get the error...
E (30) ledc: freq_hz=1000 duty_resolution=100

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the comments below :)

@SuGlider
Copy link
Collaborator

SuGlider commented Nov 9, 2022

@P-R-O-C-H-Y - Please try this sketch:

#include <driver/ledc.h>

void setup() {
  uint8_t chan = 0;
  uint8_t bit_num = 100;
  uint32_t freq = 1000000000;
  uint8_t group = (chan / 8), timer = ((chan / 2) % 4);

  ledc_timer_config_t ledc_timer = {
    .speed_mode       = (ledc_mode_t) group,
    .duty_resolution  = (ledc_timer_bit_t) bit_num,
    .timer_num        = (ledc_timer_t) timer,
    .freq_hz          = freq,
    .clk_cfg          = LEDC_AUTO_CLK //LEDC_DEFAULT_CLK
  };

  if (ledc_timer_config(&ledc_timer) != ESP_OK)
  {
    log_e("ledc setup failed!");
  } else {
    log_e("ledc setup ALL RIGHT!");
  }


  // no error happens in setup!
  analogWriteResolution(128);
  analogWriteFrequency(1000000000UL);
}

void loop() {
}

This is the output I get... With IDF it issues an error, but with Arduino Layer... nothing!

ets Jun  8 2016 00:22:57

rst:0x1 (POWERON_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
flash read err, 1000
ets_main.c 371 
ets Jun  8 2016 00:22:57

rst:0x10 (RTCWDT_RTC_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:1
load:0x3fff0030,len:1344
load:0x40078000,len:13864
load:0x40080400,len:3608
entry 0x400805f0
[     3][D][esp32-hal-cpu.c:244] setCpuFrequencyMhz(): PLL: 480 / 2 = 240 Mhz, APB: 80000000 Hz
E (30) ledc: freq_hz=1000000000 duty_resolution=100
[    32][E][Fading.ino:39] setup(): ledc setup failed!

@SuGlider
Copy link
Collaborator

SuGlider commented Nov 9, 2022

OK... the issue is because of
static int cnt_channel = LEDC_CHANNELS; that never changes...

Thus, if the sketch never calls analogWrite() before calling analogWritResolution(), no channel is allocated and it does nothing.

I think this isn't the expected behaviour. The example from Arduino Github says it should work.
https://github.com/arduino/reference-tr/blob/master/Language/Functions/Zero%2C%20Due%2C%20MKR%20Family/analogWriteResolution.adoc

Please read the notes at the end of the document from Arduino.cc - it says what should happen when the resolution parameter is higher than what the device can handle. We shall do the same.

Another point is that the resolution and frequency shall be applied to all channels/pins.
https://github.com/arduino/ArduinoCore-sam/blob/master/cores/arduino/wiring_analog.c
There you can see that there is only one resolution to every Analog output pin.

I think we shall try to comply always with Arduino MainStream.

@P-R-O-C-H-Y
Copy link
Member Author

OK... the issue is because of static int cnt_channel = LEDC_CHANNELS; that never changes...

Thus, if the sketch never calls analogWrite() before calling analogWritResolution(), no channel is allocated and it does nothing.

I think this isn't the expected behaviour. The example from Arduino Github says it should work. https://github.com/arduino/reference-tr/blob/master/Language/Functions/Zero%2C%20Due%2C%20MKR%20Family/analogWriteResolution.adoc

Please read the notes at the end of the document from Arduino.cc - it says what should happen when the resolution parameter is higher than what the device can handle. We shall do the same.

Another point is that the resolution and frequency shall be applied to all channels/pins. https://github.com/arduino/ArduinoCore-sam/blob/master/cores/arduino/wiring_analog.c There you can see that there is only one resolution to every Analog output pin.

I think we shall try to comply always with Arduino MainStream.

The problem is, that its our analogWrite is using LEDC. I think its not good to change Resolution or Frequency on all LEDC channels when not used by analogWrite. That can override some already setup led channels that are not used by analogWrite. Where I see a mistake is that return.
So we can change this:

void analogWriteResolution(uint8_t bits) {
    if (cnt_channel == LEDC_CHANNELS) {
        return; //No channel used for analogWrite
    }
    for (int channel = LEDC_CHANNELS - 1; channel >= cnt_channel; channel--) {
        ledcChangeFrequency(channel, analog_frequency, bits);
    }
    analog_resolution = bits;
}

To this:

void analogWriteResolution(uint8_t bits) {
    if (cnt_channel != LEDC_CHANNELS) {
        for (int channel = LEDC_CHANNELS - 1; channel >= cnt_channel; channel--) {
            ledcChangeFrequency(channel, analog_frequency, bits);
        }
    }
    analog_resolution = bits;
}

So even if no analogWrite was called, next analogWrite will use the analog_resolution set by analogWriteResolution.
And do the same for analogWriteFrequency. This seems to be the best approach.

@P-R-O-C-H-Y
Copy link
Member Author

P-R-O-C-H-Y commented Nov 10, 2022

The 2nd part is the error.
The bits resolution can be handled easily, but LEDC works a bit different than standart analogWrite on other ARMs.

Simple rule for using LEDC
lower frequencies -> higher resolution
higher frequencies -> lower resolution (example 40MHz freq can be set only with bit resolution 1)

I was able to set 1Hz frequency using 12 bits resolution ledcSetup(0,1,12);

So we don't know the combinations where it will work or not. Thats where the IDF error appears (there is a calculation for that). I think its like with the touch. Firstly you need to try some touchRead() to see the values of touched/untouched state and than after that you can set some threshold that will be the best for the case. Same here if user wants some frequency and resolution, there is the need of trying that. Most cases will run smooth on the 1st try without changing the resolution. But if you want some maximum or minimum frequencies (1Hz or 40MHz) you will not be able to set them without setting different resolutions.

We know the maximum resolution for each chip so adding a check is easy, but we don't know that the frequency will work with the resolution.

@SuGlider
Copy link
Collaborator

SuGlider commented Nov 10, 2022

@P-R-O-C-H-Y - I see your point with analogWrite() and LedC API. Thanks for clarifying. Let's leave the channel management as it is now.

I see these issues right now within the current implementation:

1- analogWriteResolution() and analogWriteFrequency() have no effect if executed before analogWrite(),
analog_resolution and analog_frequency are never set in this use case.
2- analogWriteResolution() shall verify the resolution parameter and trim it to the maximum allowed by the SoC whenever necessary
3- analogWriteResolution() shall verify the frequency parameter and trim it to the maximum allowed by the SoC as well
4- It would be good to issue a warning message when those parameters are invalid, in case they must be adjusted by the Arduino API.

Thanks!

@vortigont
Copy link
Contributor

btw, IDF API has some flaws in this area. I.e. duty (read as resolution) can be set but can't be read back by the user code to find out current regs configuration value.
I've hit this earlier working with LEDC espressif/esp-idf#8247

@P-R-O-C-H-Y
Copy link
Member Author

@P-R-O-C-H-Y - I see your point with analogWrite() and LedC API. Thanks for clarifying. Let's leave the channel management as it is now.

I see these issues right now within the current implementation:

1- analogWriteResolution() and analogWriteFrequency() have no effect if executed before analogWrite(), analog_resolution and analog_frequency are never set in this use case. 2- analogWriteResolution() shall verify the resolution parameter and trim it to the maximum allowed by the SoC whenever necessary 3- analogWriteResolution() shall verify the frequency parameter and trim it to the maximum allowed by the SoC as well 4- It would be good to issue a warning message when those parameters are invalid, in case they must be adjusted by the Arduino API.

Thanks!

@SuGlider I have fixed some of your point in last commit:

  1. Fixed, frequency and resolution are set even if no analogWrite() called so next analogWrite uses these values.
  2. analogWriteResolution() is checking resolution parameter and if its bigger than maximum possible -> trimming to the maximum possible resolution and prints a warning about that (point num. 4 ?)

@SuGlider
Copy link
Collaborator

btw, IDF API has some flaws in this area. I.e. duty (read as resolution) can be set but can't be read back by the user code to find out current regs configuration value. I've hit this earlier working with LEDC espressif/esp-idf#8247

@vortigont - I understood that you want to know the LEDC maximum resolution for a given channel, right?
Please check the change @P-R-O-C-H-Y has recently done.
There is a parameter LEDC_MAX_BIT_WIDTH when including<driver/ledc.h> in your sketch or IDF application.
The LEDC maximum resolution is fixed per SoC (ESP32/S2/S3/C3 etc).

@SuGlider
Copy link
Collaborator

@P-R-O-C-H-Y - I see your point with analogWrite() and LedC API. Thanks for clarifying. Let's leave the channel management as it is now.
I see these issues right now within the current implementation:
1- analogWriteResolution() and analogWriteFrequency() have no effect if executed before analogWrite(), analog_resolution and analog_frequency are never set in this use case. 2- analogWriteResolution() shall verify the resolution parameter and trim it to the maximum allowed by the SoC whenever necessary 3- analogWriteResolution() shall verify the frequency parameter and trim it to the maximum allowed by the SoC as well 4- It would be good to issue a warning message when those parameters are invalid, in case they must be adjusted by the Arduino API.
Thanks!

@SuGlider I have fixed some of your point in last commit:

  1. Fixed, frequency and resolution are set even if no analogWrite() called so next analogWrite uses these values.
  2. analogWriteResolution() is checking resolution parameter and if its bigger than maximum possible -> trimming to the maximum possible resolution and prints a warning about that (point num. 4 ?)

Excellent! The lastest commits solve those issues and make the API the same of Arduino mainstream.

Copy link
Collaborator

@SuGlider SuGlider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Work!

@vortigont
Copy link
Contributor

vortigont commented Nov 10, 2022

@vortigont - I understood that you want to know the LEDC maximum resolution for a given channel, right?
The LEDC maximum resolution is fixed per SoC (ESP32/S2/S3/C3 etc).

No, it's a bit different. There is a way to set bit resolution for a channel (not necessary maximum but any less than max), but there is no easy way to read current setting from the SOC back using IDF methods except accessing registers via hal.

For ex. I set some channel to 6 bit res (max duty is 63) and now I want to implement a setter to set a duty with error checking, but I forget what is my current channel bit resolution :)

void setDuty(unsigned duty, ledc_channel_t channel, ledc_mode_t speed_mode){
   uint32_t maxduty = ledc_get_max_duty(speed_mode, channel);    // this is not available in user code
   if ( maxduty < duty){
       Serial.printf("channel duty can't be more than %d\n", maxduty);
       return;
   }
   if (ledc_set_duty(speed_mode, channel, uint32_t duty)){      // only generic error is available
       Serial.printf("can't change duty somehow :(\n", );
   }
}

@SuGlider
Copy link
Collaborator

@P-R-O-C-H-Y - Do you think that what @vortigont wants is possible to achieve within this PR?

Something like a getCurrentResolution(ledcChannel)?

@SuGlider
Copy link
Collaborator

@P-R-O-C-H-Y - Do you think that what @vortigont wants is possible to achieve within this PR?

Something like a getCurrentResolution(ledcChannel)?

OK, let's leave it for a future PR. I'll merge it in order to move forward.

@SuGlider SuGlider merged commit 9762b23 into espressif:master Nov 11, 2022
@P-R-O-C-H-Y
Copy link
Member Author

@P-R-O-C-H-Y - Do you think that what @vortigont wants is possible to achieve within this PR?

Something like a getCurrentResolution(ledcChannel)?

This is not possible without using register hal.

Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Dec 17, 2022
* LEDC - AnalogWrite new API + ledcAttachPin duty fix (espressif#7346)

* Add enableScenes API in Rainmaker (espressif#7436)

* Added enableScenes API

* Added enableScenes API documentation

* Added enableScenes API to example

Co-authored-by: Jan Procházka <[email protected]>

* How to use USBHID classes with ESP32-S2 and ESP32-S3 (espressif#7214)

These instructions are based on esp32-arduino-lib-builder's build process, including https://github.com/espressif/esp32-arduino-lib-builder/blob/master/tools/update-components.sh which explains how to clone tinyusb.

* Add I2C and SPI pin definitions to wt32-eth01 pins configuration (espressif#7237)

* Add I2C and SPI pin definitions to wt32-eth01 pins

Added missing pins based on testing using a RTC (I2C) and SD card reader (SPI).

* Remove define macros

Co-authored-by: Rodrigo Garcia <[email protected]>
Co-authored-by: Jan Procházka <[email protected]>

* Changed Rainmaker WiFi/Factory reset time. (espressif#7514)

* Ipv6 support v2

* Update header

* Update IPAddress implementation to support V6

* Fix cut and paste error

* Explicit not equals

* Explicitly reference StreamString

* Remove != test (it is causing a conflict)

* IPv6 support: Add proper link-local and SLAAC in STA and WifiMulti

This patch partially depends on:
espressif/esp32-arduino-lib-builder#67
Without this patch we will get only Link local IPv6 (still useful for MDNS and etc).
With patch we will get also global IPv6 address by SLAAC.
By default IPv6 disabled, until it is properly tested.

Tested on BasicHttpClient by adding:
wifiMulti.IPv6(true);
before: wifiMulti.addAP() call

Enabling Core Debug Level: verbose
If IP6 obtained, in logs will be visible:
[  8028][V][WiFiGeneric.cpp:380] _arduino_event_cb(): IF[0] Got IPv6: IP Index: 0, Zone: 2, fe80:0000:0000:0000:xxxx:xxxx:xxxx:xxxx
[  8028][D][WiFiGeneric.cpp:852] _eventCallback(): Arduino Event: 8 - STA_GOT_IP6
[ 11028][V][WiFiGeneric.cpp:380] _arduino_event_cb(): IF[0] Got IPv6: IP Index: 1, Zone: 0, 2a0d:yyyy:0000:4000:yyyy:yyyy:yyyy:yyyy
[ 11028][D][WiFiGeneric.cpp:852] _eventCallback(): Arduino Event: 8 - STA_GOT_IP6

This is linked to: espressif#6242

Signed-off-by: Denys Fedoryshchenko <[email protected]>

* Add IPv6 support to WiFiServer

One of most useful features of IPv6 to have arduino accessible from internet,
without any port forward and etc.
Change is fairly trivial and backward compatible with old code, tested
with WiFiTelnetToSerial and AsyncUDPServer.

Signed-off-by: Denys Fedoryshchenko <[email protected]>

* WiFiClient::remoteIP and remoteIP6 IPv6 support

For RemoteIP and AF_INET6 socket i added support ip6 to ip4 mapping,
so .remoteIP will return IPv4 address on dual stack socket, if available.

Scenarios tested:
WiFiTelnetToSerial, wifiMulti.IPv6(true), connect both from IPv4 and IPv6
WiFiTelnetToSerial, wifiMulti.IPv6(true); but set to listen on IPv4 only.
WiFiTelnetToSerial, IPv6 disabled, with or without bind to specific IP4.
AsyncUDPServer, without IPv6 support.

Signed-off-by: Denys Fedoryshchenko <[email protected]>

* Add WiFiTelnetToSerialIPv6 example

To demonstrate new abilities of dual stack WiFiServer and
remoteIP6 we add this example.

Signed-off-by: Denys Fedoryshchenko <[email protected]>

* Add IPv6 to WifiClient (client)

We need to be able to connect to remote servers over IPv6 too,
and thats need different approach in DNS queries and connect().
As i'm trying to keep maximum compatibility, i introduce different
behaviour if IPv6 is enabled, and backward compatible (as much as possible),
if IPv6 is not enabled.
IN future when IPv6 functions are tested well enough, it can be simplified.

This implementation tested on esp32 in following scenarios using BasicHttpClient:

IPv6 true:
IPv6 only website (caveat 1) - OK
Website with A and AAAA is present (caveat 1) - OK
IPv4 only website - OK

IPv6 not enabled:
IPv6 only website - wont open (expected)
Website with A and AAAA is present - OK, opens over IPv4
IPv4 only website - OK

caveat 1 - sometimes SLAAC is slower than DHCPv4, so we might have
status WL_CONNECTED, but IPv6 global scope is not ready yet.

Signed-off-by: Denys Fedoryshchenko <[email protected]>

* WiFiTelnetToSerialIPv6.ino: fix obsolete remoteIP6 call to remoteIP

Example contained API from previous IPv6 implementation, fixing it.

Signed-off-by: Denys Fedoryshchenko <[email protected]>

Signed-off-by: Denys Fedoryshchenko <[email protected]>
Co-authored-by: Sly Gryphon <[email protected]>
Co-authored-by: Denys Fedoryshchenko <[email protected]>

* Fix toString() for IPv4

* Full support for IPv6

* Full support for IPv6

* Fixed ambiguous definitions

* Fix sizeof

* Simplify initilization and remove memset

* Enable support for IPv6 in UDP

* Update PlatformIO build script PR 7579

* Fix IPv6 size

Signed-off-by: Denys Fedoryshchenko <[email protected]>
Co-authored-by: Jan Procházka <[email protected]>
Co-authored-by: Sanket Wadekar <[email protected]>
Co-authored-by: RefactorFactory <[email protected]>
Co-authored-by: Sebastian Bergner <[email protected]>
Co-authored-by: Rodrigo Garcia <[email protected]>
Co-authored-by: Sly Gryphon <[email protected]>
Co-authored-by: Denys Fedoryshchenko <[email protected]>
Co-authored-by: s-hadinger <[email protected]>
@P-R-O-C-H-Y P-R-O-C-H-Y deleted the Ledc-fixes branch April 19, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Peripherals API Relates to peripheral's APIs.
Projects
5 participants