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

feature(ledc): Add output invert option for LEDC pin + minor fixes #9257

Merged
merged 10 commits into from
Feb 21, 2024

Conversation

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

@P-R-O-C-H-Y P-R-O-C-H-Y commented Feb 16, 2024

Description of Change

  • Added ledcOutputInvert(uint8_t pin, bool out_invert) function, to enable setting inverting LEDC output for selected pin. Can be called after ledcAttach() anytime.
  • Fixing ledc_handle.used_channels never initialised before use.
  • Documented all function the the header file.

Tests scenarios

Tested on ESP32. On screen below ledcOutputInvert on LED2 was called on the black line, and LED2 output was immediately inverted.
Snímek obrazovky 2024-02-16 v 13 54 14

Related links

Closes #7501
Closes #9213
Closes #9212

@P-R-O-C-H-Y P-R-O-C-H-Y added the Area: Peripherals API Relates to peripheral's APIs. label Feb 16, 2024
@P-R-O-C-H-Y P-R-O-C-H-Y added this to the 3.0.0-RC1 milestone Feb 16, 2024
@P-R-O-C-H-Y P-R-O-C-H-Y self-assigned this Feb 16, 2024
Copy link
Contributor

github-actions bot commented Feb 16, 2024

Warnings
⚠️ Please consider squashing your 10 commits (simplifying branch history).

👋 Hello P-R-O-C-H-Y, we appreciate your contribution to this project!


Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 25a785c

@P-R-O-C-H-Y P-R-O-C-H-Y force-pushed the feature/ledc-invert branch 3 times, most recently from eced014 to c15582b Compare February 16, 2024 18:24
@P-R-O-C-H-Y P-R-O-C-H-Y added the Status: Review needed Issue or PR is awaiting review label Feb 16, 2024
cores/esp32/esp32-hal-ledc.c Outdated Show resolved Hide resolved
@VojtechBartoska VojtechBartoska added Status: In Progress Issue is in progress and removed Status: Review needed Issue or PR is awaiting review labels Feb 19, 2024
@P-R-O-C-H-Y P-R-O-C-H-Y added Status: Review needed Issue or PR is awaiting review and removed Status: In Progress Issue is in progress labels Feb 19, 2024
docs/en/api/ledc.rst Show resolved Hide resolved
docs/en/api/ledc.rst Outdated Show resolved Hide resolved
@SuGlider SuGlider self-requested a review February 19, 2024 23:56
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.

A minor suggestion for the function parameter description.
A suggestion about testing resolution for Zero (which may be not allowed).
Check if this makes sense. Overall it is good.

@TD-er
Copy link
Contributor

TD-er commented Feb 20, 2024

Just as a suggestion about resolution being 0.
For some functions, there are multiple parameters given which also depend on eachother, like frequency and resolution.
There are some configurations possible where frequency and resolution exceed the max. clock frequency they depend on.

So a resolution of 0 could perhaps be interpreted like "leave as it is or lower resolution if needed".
Is this something that's useful? Or will it result in confusion to the user as the max. PWM duty then also changes?

If this is not practical, then maybe this class could have some static function to compute the max. frequency given some resolution and vice verse?
Especially if the max. depends on platform and/or the max. frequency can change at runtime for example in low power mode or when the CPU clock has changed.

Co-authored-by: Rodrigo Garcia <rodrigo.garcia@espressif.com>
@P-R-O-C-H-Y
Copy link
Member Author

Just as a suggestion about resolution being 0. For some functions, there are multiple parameters given which also depend on eachother, like frequency and resolution. There are some configurations possible where frequency and resolution exceed the max. clock frequency they depend on.

So a resolution of 0 could perhaps be interpreted like "leave as it is or lower resolution if needed". Is this something that's useful? Or will it result in confusion to the user as the max. PWM duty then also changes?

If this is not practical, then maybe this class could have some static function to compute the max. frequency given some resolution and vice verse? Especially if the max. depends on platform and/or the max. frequency can change at runtime for example in low power mode or when the CPU clock has changed.

I think this is not ideal, as the resolution will be kind of "unknown" to the user, so it will be hard for them to set right duty for the PWM. It will make sense, if we also add a function like ledcWritePercentage(uint8_t percentage). So the correct duty will be calculated according to the resolution set and the percentage automatically.

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.

Very good!

Copy link
Collaborator

@lucasssvaz lucasssvaz left a comment

Choose a reason for hiding this comment

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

LGTM

@TD-er
Copy link
Contributor

TD-er commented Feb 20, 2024

Should this also fix #9212 ?
Or couldn't you reproduce that one?

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

Should this also fix #9212 ? Or couldn't you reproduce that one?

I was not albe to reproduce that, but as I have fixed part, where if the pin was already attached to the LEDC was wrongly reattached. Now if the pin is already attached to the LEDC, the ledcAttach() will return false and print info of how the pin is set up (channel, frequency and resolution).

So you are not able to call ledcAttach() multiple times on same pin, without previously detaching the pin from LEDC.

@TD-er
Copy link
Contributor

TD-er commented Feb 20, 2024

That sounds good.

Just a question, is there a generic/preferred function to clear any assigned bus/peripheral from a pin, regardless of what was using it?
If there is one, then I could use it as a generic way to make sure a pin will not be assigned to the same peripheral without 'unregistering' it first.
Or maybe even better, to check if a pin is set to some bus/peripheral before trying to assign it to another one and just present the user with some error including how that pin was already assigned.

@SuGlider
Copy link
Collaborator

SuGlider commented Feb 20, 2024

That sounds good.

Just a question, is there a generic/preferred function to clear any assigned bus/peripheral from a pin, regardless of what was using it? If there is one, then I could use it as a generic way to make sure a pin will not be assigned to the same peripheral without 'unregistering' it first. Or maybe even better, to check if a pin is set to some bus/peripheral before trying to assign it to another one and just present the user with some error including how that pin was already assigned.

Arduino Core 3.0.0 has the Peripheral Manager API.
https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/esp32-hal-periman.h
https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/esp32-hal-periman.c

This is the way it works when a pin is already attached to a peripheral, like PWM (LEDC), and then it is used as RMT:

RMT rmtInit(...) call will attach the pin to RMT using perimanSetPinBus(pin, peripheralType, busPointer, channel). By turn, it will automaticaly call the LEDC detaching function before, and only then, run the RMT attachment routine, latter.
Therefore, it will work correctly, turning off all the drivers and resetting the previous peripheral before starting the same pin for the new peripheral.

#include "esp32-hal-periman.h"
// Turn `Core Debug Level: "Verbose"` to see all messages from Peripheral Manager

#define PIN 2
void setup() {
  // Peripheral Manager will attach pin 2 UART (RX) - TX is the UART0 default pin.
  Serial.begin(115200, SERIAL_8N1, PIN);
  Serial.println("Serial pins - RX (pin 2) and TX (default) are attached");
  delay(1000);

  //Peripheral manage will detach pin 2 from UART (TX) and attach it to LEDC
  analogWrite(PIN, 100); // by default it will use 1KHz, 8 bits resolution
  Serial.println("Pin 2 is attached to LEDC");
  delay(5000);

  // Peripheral manager will detach pin 2 from LEDC and attach it to SigmaDelta
  sigmaDeltaAttach(PIN, 312500);
  sigmaDeltaWrite(PIN, 10);     // Start SigmaDelta
  Serial.println("Pin 2 is attached to SigmaDelta");
  delay(5000);

  // Peripheral manager will detach pin 2 from SigmaDelta and attach it to GPIO Ouput
  pinMode(PIN, OUTPUT);
  digitalWrite(PIN, HIGH); // pin 2 in HIGH state.
  Serial.println("Pin 2 is attached to GPIO");
  delay(5000);
  digitalWrite(PIN, LOW); // pin 2 in LOW state.

  // General PIN detaching from what ever it was attached previously...
  perimanClearPinBus(PIN);
  Serial.println("Pin 2 is fully detached");
  delay(1000);

  // Detached UART0 TX default pin
  Serial.println("UART0 TX is fully detached");
  Serial.flush(); // message is out!
  Serial.end();
}

void loop() {}

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

That sounds good.

Just a question, is there a generic/preferred function to clear any assigned bus/peripheral from a pin, regardless of what was using it? If there is one, then I could use it as a generic way to make sure a pin will not be assigned to the same peripheral without 'unregistering' it first. Or maybe even better, to check if a pin is set to some bus/peripheral before trying to assign it to another one and just present the user with some error including how that pin was already assigned.

@TD-er To check what bus(peripheral) is assigned to a pin you can use peripheral_bus_type_t perimanGetPinBusType(uint8_t pin); which will return a bus type assigned to a pin. If the pin has no bus(peripheral) attached to it, you will get ESP32_BUS_TYPE_INIT as returned value.

There is no need to clear the bus first as @SuGlider mentioned. The detaching is automatic before attaching new bus(peripheral). But if you want to clear the pins bus(peripheral) without knowing, what peripheral is attached to the pin, you can use perimanClearPinBus(PIN);.

@P-R-O-C-H-Y P-R-O-C-H-Y added Status: Pending Merge Pull Request is ready to be merged and removed Status: Review needed Issue or PR is awaiting review labels Feb 20, 2024
@SuGlider
Copy link
Collaborator

SuGlider commented Feb 20, 2024

@P-R-O-C-H-Y - I found an issue... aparently with LEDC detaching, I think. SigmaDelta issue!

Try running the example from #9257 (comment)
I have a LED attached to GPIO2 (ESP32).
When LEDC runs before SigmaDelta, the LED doesn't light up with SigmaDelta.
But if I invert the order and run SigmaDelta before LEDC, both work fine.

--- NOTE: no... the issue is in SigmaDelta... it requires more debuging.

Final Note

LEDC is OK. Issue is in SigmaDelta Attaching.
Solved with #9268

@P-R-O-C-H-Y P-R-O-C-H-Y added Status: Test needed Issue needs testing and removed Status: Pending Merge Pull Request is ready to be merged labels Feb 20, 2024
@P-R-O-C-H-Y
Copy link
Member Author

@P-R-O-C-H-Y - I found an issue... aparently with LEDC detaching, I think.

Try running the example from #9257 (comment) I have a LED attached to GPIO2 (ESP32). When LEDC runs before SigmaDelta, the LED doesn't light up with SigmaDelta. But if I invert the order and run SigmaDelta before LEDC, both work fine.

--- NOTE: no... the issue is in SigmaDelta... it need more debuging.

@SuGlider Will do some testing tomorrow.

@SuGlider
Copy link
Collaborator

@P-R-O-C-H-Y - I found an issue... aparently with LEDC detaching, I think.
Try running the example from #9257 (comment) I have a LED attached to GPIO2 (ESP32). When LEDC runs before SigmaDelta, the LED doesn't light up with SigmaDelta. But if I invert the order and run SigmaDelta before LEDC, both work fine.
--- NOTE: no... the issue is in SigmaDelta... it need more debuging.

@SuGlider Will do some testing tomorrow.

Issue was in SigmaDelta. Solved with #9268
LEDC is fine. Ready for Merging.

@SuGlider SuGlider added Status: Pending Merge Pull Request is ready to be merged and removed Status: Test needed Issue needs testing labels Feb 20, 2024
@TD-er
Copy link
Contributor

TD-er commented Feb 20, 2024

Both thanks for the extensive explanation and apparently answering this led to another bug :)

I will have another close look at those periman functions to see how I can use them in ESPEasy.

@me-no-dev me-no-dev merged commit c4b55bb into espressif:master Feb 21, 2024
62 checks passed
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. Status: Pending Merge Pull Request is ready to be merged
Projects
6 participants