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

Adds Relay PWM support #376

Merged
merged 2 commits into from Nov 11, 2021
Merged

Adds Relay PWM support #376

merged 2 commits into from Nov 11, 2021

Conversation

garymueller
Copy link
Contributor

there is a checkbox added to the relay configuration allowing the selection of a
pwm support. If pwm support enabled then the code will perform an
analogwrite to the GPIO pin instead of a digitalwrite.

there is a checkbox added to the relay configuration allowing the selection of a
pwm support.  If pwm support enabled then the code will perform an
analogwrite to the GPIO pin instead of a digitalwrite.
@forkineye
Copy link
Owner

I'll take a look at this tomorrow. Has it been tested with all channels running via e131?

@garymueller
Copy link
Contributor Author

I tested it across 8 relays in multiple configurations with some pwm and some not. I then ran e131 through a tester app and xlights and observed the relays behaving as expected. Thanks for your review and consideration.

@patdelaney
Copy link
Collaborator

@garymueller I'm not really a low level HW guy, I'm more on the side of QA. But how would one test this? analogwrite to the GPIO pin instead of a digitalwrite. ?? DO I need special hardware?

@garymueller
Copy link
Contributor Author

@garymueller I'm not really a low level HW guy, I'm more on the side of QA. But how would one test this? analogwrite to the GPIO pin instead of a digitalwrite. ?? DO I need special hardware?

No special hardware is needed to test as verification consisted of printouts and direct observation. analogwrite and digitalwrite are function calls where analogwrite is used to support pwm and digitalwrite is absolute using the values of LOW and HIGH. In my test cases I added printouts that displayed the values written to the pins as well as observing the results on the relay board. The relay board I used has led indicators that would dim/brighten with the analogwrite and be fully bright(on) or off with digitalwrite.

@garymueller
Copy link
Contributor Author

I just saw my email that the build failed on ESP32. I only built and tested on ESP8266. I'll try to look at this tonight (pending free time) and see what is different between the two systems.

Analogwrite is not available on the Esp32 and it is a known issue
espressif/arduino-esp32#4

There are alternate methods to analogwrite on the Esp32 to achieve the same functionality
  ledcAttachPin
  ledcSetup
  ledcWrite

supported the two different architectures through ifdefs
@garymueller
Copy link
Contributor Author

I just saw my email that the build failed on ESP32. I only built and tested on ESP8266. I'll try to look at this tonight (pending free time) and see what is different between the two systems.

I pushed a new fix for the esp32 that compiles across both plaforms. Note that the ESP32 version is untested as I dont have one available at the moment. It uses #ifdefs to accomplish the pwm functionality across both architectures. I personally dont like ifdefs in the code. I leave the code for your review and feedback but you wont hurt my feelings if it gets rejected. I'm really surprised that analogwrite is not supported across both architectures.

Copy link
Collaborator

@MartinMueller2003 MartinMueller2003 left a comment

Choose a reason for hiding this comment

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

Overall this looks good and can be committed as is. The one change I would make is to rename Pwm to OutputUsingPwm but that is a minor change and would not stop this merge.

@forkineye forkineye merged commit 37e30bf into forkineye:main Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants