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: Do not drive output during init if the output is inverted (IDFGH-6875) #8497

Merged
merged 1 commit into from Mar 11, 2022

Conversation

nonoo
Copy link
Contributor

@nonoo nonoo commented Mar 4, 2022

If the output of LEDC GPIO is inverted, then the correct level must be set before initializing the GPIO, otherwise it will be driven until the next call to stop LEDC output.

@CLAassistant
Copy link

CLAassistant commented Mar 4, 2022

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Mar 4, 2022
@github-actions github-actions bot changed the title ledc: Do not drive output during init if the output is inverted ledc: Do not drive output during init if the output is inverted (IDFGH-6875) Mar 4, 2022
Copy link
Contributor

@KaeLL KaeLL left a comment

Choose a reason for hiding this comment

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

Shouldn't gpio_set_direction be called before gpio_set_level?

@nonoo
Copy link
Contributor Author

nonoo commented Mar 4, 2022 via email

@@ -638,6 +638,7 @@ esp_err_t ledc_channel_config(const ledc_channel_config_t *ledc_conf)
);
/*set LEDC signal in gpio matrix*/
gpio_hal_iomux_func_sel(GPIO_PIN_MUX_REG[gpio_num], PIN_FUNC_GPIO);
gpio_set_level(gpio_num, output_invert);
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense. You should set the pin H/L value before driving the pin as an output. I'm guessing esp_rom_gpio_connect_out_signal() also sets pin H/L, but by that point you would see a glitch on the pin without this fix. I've done the same thing in other hardware drivers I've written.

Copy link
Contributor

Choose a reason for hiding this comment

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

I should also mention esp_rom_gpio_connect_out_signal() is in the ESP32 ROM file, so I didn't find a source file.

@songruo songruo added the PR-Sync-Merge Pull request sync as merge commit label Mar 7, 2022
@songruo
Copy link
Collaborator

songruo commented Mar 7, 2022

sha=062f6164ae89d54df3f1a89a67f01e82ae56e0d8

@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new Resolution: NA Issue resolution is unavailable labels Mar 10, 2022
@espressif-bot espressif-bot merged commit f38c13a into espressif:master Mar 11, 2022
@KaeLL
Copy link
Contributor

KaeLL commented Mar 21, 2022

@nonoo Unrelated to the PR, but not completely, I found out I know nothing about GPIOs. Can you tell me why driving a GPIO before setting the direction works? And since it works and it's set to output, why set the direction at all?

@nonoo
Copy link
Contributor Author

nonoo commented Mar 22, 2022

@nonoo Unrelated to the PR, but not completely, I found out I know nothing about GPIOs. Can you tell me why driving a GPIO before setting the direction works? And since it works and it's set to output, why set the direction at all?

The set level call only sets the level internally, it will not be outputted to the pin until the internal pin matrix has the pin mapped to the GPIO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants