Fixed bug of using wrong pin for detach in setup() + another bug + improvement #66
Conversation
Fixed bug of using wrong pin for detach in setup(). Fixed bug of calling ledcAttach() twice for same pin.
src/ESP32PWM.h
Outdated
| @@ -24,6 +24,13 @@ | |||
| #define USABLE_ESP32_PWM (NUM_PWM-PWM_BASE_INDEX) | |||
| #include <cstdint> | |||
|
|
|||
| #if !defined(ESP_ARDUINO_VERSION) | |||
There was a problem hiding this comment.
no, please do not do this, this will cause major problems in dependant libraries that use the standard version checking to check what toolchain is compiling the code. Most of the changes you made that depend on this need to be changed back before merging.
There was a problem hiding this comment.
Sorry for being late, but GitHub does not allow me to subscribe for this PR 😞 -> fixed now 😀 .
You are totally right!
I moved the code to ESP32PWM.cpp.
src/ESP32PWM.h
Outdated
| @@ -50,9 +57,9 @@ class ESP32PWM { | |||
| return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min; | |||
| } | |||
|
|
|||
| double setup(double freq, uint8_t resolution_bits=10); | |||
| bool setup(uint8_t pin, double freq, uint8_t resolution_bits=10); | |||
There was a problem hiding this comment.
the pin is specified in the attach method not in the setup method. Please overload methods to add additions to the API and do not change existing API's. This library is used by thousands of other people, you can not change published API's.
There was a problem hiding this comment.
Thanks for the library and for reviewing this PR, and sorry for my quick hack!
I commited the changes now.
| @@ -8,6 +8,14 @@ | |||
| #include <ESP32PWM.h> | |||
| #include "esp32-hal-ledc.h" | |||
|
|
|||
| #if !defined(ESP_ARDUINO_VERSION) | |||
There was a problem hiding this comment.
this will mess up toolchain detection on other libraries imported after this header. Just add the version detection gaurdes around each section of code where differing behavior is needed for different versions of the toolchian
There was a problem hiding this comment.
Sorry I do not really understand.
I think, this code is not in a header, so it is strictly locally to the code below.
|
Hi, |
Without this fixed I got:
[ 7634][E][esp32-hal-periman.c:180] perimanGetPinBus(): Invalid pin: 255
[ 7641][E][esp32-hal-periman.c:122] perimanSetPinBus(): Invalid pin: 255
[ 7647][E][esp32-hal-ledc.c:116] ledcAttachChannel(): Pin 255 is already attached to another bus and failed to detach