drivers: pwm_led: esp32: add inverted pwm polarity #80673

epc-ake · 2024-10-31T10:36:18Z

This PR enables the PWM to be inverted by making use of the PWM_POLARITY_INVERTED flag.
It also fixes an overflow bug when setting duty-cycle to 100 percent.

drivers/pwm/pwm_led_esp32.c

uLipe

LGTM so far, just consider the point from @wmrsouza and I will also approve it.

mmahadevan108 · 2024-11-04T19:48:44Z

@epc-ake Can you please add an issue with details and link that issue to this PR. After RC2 only bug fixes with an associated Github ssue will be merged into the upcoming 4.0 release.

raffarost · 2024-11-04T19:57:16Z

hi @epc-ake,
can you check that you really are able to get a 100% duty with this solution?
I checked with the oscilloscope and see a pulse (99% duty, which is expected), so we think this might not be the best solution.
can you confirm?

Please have a look at #80878 if possible

epc-ake · 2024-11-05T10:43:17Z

hi @epc-ake, can you check that you really are able to get a 100% duty with this solution? I checked with the oscilloscope and see a pulse (99% duty, which is expected), so we think this might not be the best solution. can you confirm?

Please have a look at #80878 if possible

Thanks for pointing this out. I didn't notice it actually.
I pushed a fix now but we can also use the changed from your PR.
However, I have an jet an other issue with this driver. It seems like the driver ignores the output states defined in the pinctrl definition of the devicetree. e.g. output-high; has no effect.
It's kind of important for an inverted pwm to also initialize with 100% dut i think.

@raffarost please tell me how you'd like to proceed with this and I'll adopt.

raffarost · 2024-11-05T13:43:22Z

@epc-ake I think the way you implemented is also alright, but I'd personally leave the possibility to set the value in the DT, including the max res (for applications which won't use duty = 100%). Probably not that relevant but I think leaving open the possibility of using the config is better.

@sylvioalves what do you think?

epc-ake · 2024-11-05T15:57:18Z

I suggest that I remove my changes regarding 100% pwm issue so we can use your PR for that. Looks cleaner to me.
Meanwile I will try to fix the initialization state issue for inverted pwm polarity in this PR.

raffarost · 2024-11-05T20:39:38Z

hi @epc-ake,
I spoke to Sylvio about this and we concluded you can keep the fix in your PR.
Would you mind adding a comment to explain why SOC_LEDC_TIMER_BIT_WIDTH - 1 is used?
thank you

drivers/pwm/pwm_led_esp32.c

epc-ake · 2024-11-06T16:53:45Z

Hi all
I needed to refactor some code here to be able to initialize the pwm with high state (needed for inverted pwm's)
This includes moving the channel and pin configuration out of pwm_led_esp32_set_cycles() to pwm_led_esp32_init().
The only way I found setting the initial state of the pwm's was by using the ledc_hal_set_idle_level() function in combination with a new dt-property init-high.

I intentionally made this changes in a separate commit but if you prefer, I can also squeeze them into one.
@raffarost and @sylvioalves, could you have a look?

raffarost

LGTM
you'll need to rebase it with latest main

raffarost · 2024-11-06T19:55:39Z

drivers/pwm/pwm_led_esp32.c

+		.speed_mode = DT_REG_ADDR(node_id) < SOC_LEDC_CHANNEL_NUM ? LEDC_LOW_SPEED_MODE    \
+									  : !LEDC_LOW_SPEED_MODE,  \
+		.clock_src = CLOCK_SOURCE,                                                         \
+		.inverted = DT_PROP(node_id, init_high),                                           \


I couldn't be sure this flag would fit in the field flags already foreseen in the DT (pwm_dt_spec / pwm_flags_t)
It would align better with the runtime check made in function set_cycles

Do you know how to get the value of the flags field here?
I‘d actually prefer that over adding an extra dt-property but I didn’t find a way to do it.

in include/zephyr/drivers/pwm.h stay all the macros to unpack dts values:

* const struct pwm_dt_spec spec = * PWM_DT_SPEC_GET_BY_NAME(DT_NODELABEL(n), alpha); * * // Initializes 'spec' to: * // { * // .dev = DEVICE_DT_GET(DT_NODELABEL(pwm1)), * // .channel = 1, * // .period = 1000, * // .flags = PWM_POLARITY_NORMAL, * // }

does it make sense to declare a pwms field for this driver (and include the flag)?
I don't know enough about the API to give a solid opinion now, but others might know better.

pwms = <&pwm2 3 2000 PWM_POLARITY_INVERTED>;

raffarost · 2024-12-09T12:24:33Z

@epc-ake do you intend to keep working on this PR?
we shall at some point add these functionalities to the driver, so just asking to have an idea

epc-ake · 2024-12-09T12:57:10Z

Hi @raffarost.
I'm a bit busy currently but I might find some time at the end of this week for this.
If you want to speed it up feel free to create a new PR on top of this :-)

raffarost · 2024-12-09T13:20:23Z

Hi @raffarost. I'm a bit busy currently but I might find some time at the end of this week for this. If you want to speed it up feel free to create a new PR on top of this :-)

no rush, if you'll submit it at some point it's fine
thank you

This PR enables the PWM to be inverted by making use of the `PWM_POLARITY_INVERTED` flag. It also fixes an overflow bug when setting duty-cycle to 100 percent. Signed-off-by: Armin Kessler <ake@espros.com>

Set pwm init state to value defined in flags field. Furthermore 100 percent is now actually 100 percent and not 99. Signed-off-by: Armin Kessler <ake@espros.com>

epc-ake · 2024-12-10T17:21:57Z

@raffarost
Could you please test it on your side?

raffarost · 2024-12-10T20:47:52Z

@epc-ake I did some testing and think a fix regarding frequency update is needed (please check #82820, last commit). I tested on all devices and it seems to work fine. If you agree and can also kindly test them, feel free to incorporate the changes in your PR.

@breiden can you check if this PR with these changes solve the problem you reported in #82177?

raffarost · 2024-12-10T20:57:09Z

drivers/pwm/pwm_led_esp32.c

+		.speed_mode = DT_REG_ADDR(node_id) < SOC_LEDC_CHANNEL_NUM ? LEDC_LOW_SPEED_MODE    \
+									  : !LEDC_LOW_SPEED_MODE,  \
+		.clock_src = CLOCK_SOURCE,                                                         \
+		.inverted = DT_PWMS_FLAGS(node_id),                                                \


regarding this field, I checked other PWM drivers and these macros and pwms field seems to be only used by application, not directly by the driver. Considering this apparent 'standard' for the drivers, I'm not sure we can use it here. Possibly your earlier solution was more appropriate.

Sounds the earlier solution is indeed more appropriate.

I don't see the issue with using 'DT_PWMS_FLAGS' here.
To me this seems to be better since we avoid having redundant information in the device tree.

epc-ake · 2024-12-12T12:22:19Z

@epc-ake I did some testing and think a fix regarding frequency update is needed (please check #82820, last commit). I tested on all devices and it seems to work fine. If you agree and can also kindly test them, feel free to incorporate the changes in your PR.

@raffarost Since you already made a PR on top of this one (#82820), would you take over this?
I wont find enough time this year any more.

raffarost · 2024-12-12T20:26:02Z

@epc-ake I did some testing and think a fix regarding frequency update is needed (please check #82820, last commit). I tested on all devices and it seems to work fine. If you agree and can also kindly test them, feel free to incorporate the changes in your PR.

@raffarost Since you already made a PR on top of this one (#82820), would you take over this? I wont find enough time this year any more.

hi @epc-ake @braiden
sure, I'll evaluate the issues you mentioned and work on it.
thanks for the input

epc-ake requested a review from LucasTambor as a code owner October 31, 2024 10:36

zephyrbot added area: PWM Pulse Width Modulation platform: ESP32 Espressif ESP32 labels Oct 31, 2024

zephyrbot requested review from anangl, henrikbrixandersen, marekmatej, raffarost, sylvioalves, uLipe and wmrsouza October 31, 2024 10:37

zephyrbot assigned sylvioalves Oct 31, 2024

epc-ake force-pushed the fix/drivers/pwm_polarity_invert branch from f37ba7c to fa5a4f5 Compare October 31, 2024 10:43

wmrsouza requested changes Oct 31, 2024

View reviewed changes

drivers/pwm/pwm_led_esp32.c Show resolved Hide resolved

uLipe reviewed Oct 31, 2024

View reviewed changes

wmrsouza previously approved these changes Oct 31, 2024

View reviewed changes

sylvioalves previously approved these changes Oct 31, 2024

View reviewed changes

uLipe previously approved these changes Oct 31, 2024

View reviewed changes

raffarost previously approved these changes Nov 1, 2024

View reviewed changes

sylvioalves modified the milestone: v4.0.0 Nov 4, 2024

mmahadevan108 added the bug The issue is a bug, or the PR is fixing a bug label Nov 4, 2024

epc-ake dismissed stale reviews from raffarost, uLipe, sylvioalves, and wmrsouza via 71a6c00 November 5, 2024 10:29

epc-ake force-pushed the fix/drivers/pwm_polarity_invert branch from fa5a4f5 to 71a6c00 Compare November 5, 2024 10:29

epc-ake marked this pull request as draft November 5, 2024 12:30

raffarost reviewed Nov 5, 2024

View reviewed changes

drivers/pwm/pwm_led_esp32.c Outdated Show resolved Hide resolved

epc-ake force-pushed the fix/drivers/pwm_polarity_invert branch from 3a14f4e to 231a780 Compare November 6, 2024 16:52

epc-ake force-pushed the fix/drivers/pwm_polarity_invert branch from 231a780 to 155b219 Compare November 6, 2024 17:07

raffarost reviewed Nov 6, 2024

View reviewed changes

uLipe previously approved these changes Nov 12, 2024

View reviewed changes

epc-ake added 2 commits December 10, 2024 18:19

drivers: pwm_led: esp32: add inverted pwm polarity

b033e40

This PR enables the PWM to be inverted by making use of the `PWM_POLARITY_INVERTED` flag. It also fixes an overflow bug when setting duty-cycle to 100 percent. Signed-off-by: Armin Kessler <ake@espros.com>

drivers: pwm_led: esp32: fix 100 percent pwm and init state

d788349

Set pwm init state to value defined in flags field. Furthermore 100 percent is now actually 100 percent and not 99. Signed-off-by: Armin Kessler <ake@espros.com>

epc-ake dismissed uLipe’s stale review via d788349 December 10, 2024 17:19

epc-ake force-pushed the fix/drivers/pwm_polarity_invert branch from 155b219 to d788349 Compare December 10, 2024 17:19

epc-ake marked this pull request as ready for review December 10, 2024 17:21

raffarost reviewed Dec 10, 2024

View reviewed changes

epc-ake closed this Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: pwm_led: esp32: add inverted pwm polarity #80673

drivers: pwm_led: esp32: add inverted pwm polarity #80673

epc-ake commented Oct 31, 2024

uLipe left a comment

mmahadevan108 commented Nov 4, 2024

raffarost commented Nov 4, 2024 •

edited

Loading

epc-ake commented Nov 5, 2024

raffarost commented Nov 5, 2024

epc-ake commented Nov 5, 2024

raffarost commented Nov 5, 2024

epc-ake commented Nov 6, 2024

raffarost left a comment

raffarost Nov 6, 2024

epc-ake Nov 6, 2024

raffarost Nov 7, 2024

raffarost commented Dec 9, 2024

epc-ake commented Dec 9, 2024

raffarost commented Dec 9, 2024

epc-ake commented Dec 10, 2024

raffarost commented Dec 10, 2024

raffarost Dec 10, 2024

sylvioalves Dec 12, 2024

epc-ake Dec 12, 2024

epc-ake commented Dec 12, 2024 •

edited

Loading

raffarost commented Dec 12, 2024

drivers: pwm_led: esp32: add inverted pwm polarity #80673

drivers: pwm_led: esp32: add inverted pwm polarity #80673

Conversation

epc-ake commented Oct 31, 2024

uLipe left a comment

Choose a reason for hiding this comment

mmahadevan108 commented Nov 4, 2024

raffarost commented Nov 4, 2024 • edited Loading

epc-ake commented Nov 5, 2024

raffarost commented Nov 5, 2024

epc-ake commented Nov 5, 2024

raffarost commented Nov 5, 2024

epc-ake commented Nov 6, 2024

raffarost left a comment

Choose a reason for hiding this comment

raffarost Nov 6, 2024

Choose a reason for hiding this comment

epc-ake Nov 6, 2024

Choose a reason for hiding this comment

raffarost Nov 7, 2024

Choose a reason for hiding this comment

raffarost commented Dec 9, 2024

epc-ake commented Dec 9, 2024

raffarost commented Dec 9, 2024

epc-ake commented Dec 10, 2024

raffarost commented Dec 10, 2024

raffarost Dec 10, 2024

Choose a reason for hiding this comment

sylvioalves Dec 12, 2024

Choose a reason for hiding this comment

epc-ake Dec 12, 2024

Choose a reason for hiding this comment

epc-ake commented Dec 12, 2024 • edited Loading

raffarost commented Dec 12, 2024

raffarost commented Nov 4, 2024 •

edited

Loading

epc-ake commented Dec 12, 2024 •

edited

Loading