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

Add Linkind dimmer support #14004

Closed
wants to merge 1 commit into from
Closed

Conversation

pcdiem
Copy link
Contributor

@pcdiem pcdiem commented Dec 11, 2021

Description:

This PR adds support for Linkind (https://www.amazon.com/dp/B08DRF58T1) ESP32-based dimmer that uses I2C to communicate with the dimmer hardware.

Please check the changes to the build and template files and let me know if anything was not done the way you want them implemented.

Related issue (if applicable): fixes #

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works with Tasmota core ESP8266 V.2.7.4.9
  • The code change is tested and works with Tasmota core ESP32 V.2.0.1.1
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

@Jason2866
Copy link
Collaborator

Jason2866 commented Dec 11, 2021

What is the reason for adding the device in tasmota_template.h
New devices are added via a Template. Only reason for adding here is when a special new driver needs be activated. The needed config could be done via Autoconfig.
Why are new build env added? The linkind dimmer can be added to the solo1 env with a ifdef in user_config_override.h. Tasmota env are not ment to be used just for a single device. If a precompiled version is wanted which has the Linkind support enabled, I can add it to special builds solo1 variants.
Please remove everything from the PR which adds env to compile builds with github. You can add one env in platformio_tasmota_cenv_sample.ini

@pcdiem
Copy link
Contributor Author

pcdiem commented Dec 11, 2021

I thought I needed to add the device in tasmota_template.h because the PWM dimmer module does not exist for ESP32. Maybe the right thing is to add the PWM Dimmer module as an ESP32 template?

@Jason2866
Copy link
Collaborator

Jason2866 commented Dec 12, 2021

Why adding a template in general? Is there a specific function used which is not possible to call with a usual configuration Template provided?
So adding the needed changes in code (#ifdef...) and that's all.
Do I overlook something?

#endif // ESP8266
#ifdef USE_LINKIND
if (LINKIND == TasmotaGlobal.module_type) TasmotaGlobal.use_pwm_dimmer = true;
#endif // USE_LINKIND
}
Copy link
Owner

Choose a reason for hiding this comment

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

This won't work as module_types will be duplicated between esp8266 and esp32. I strongly suggest NOT to use the module_type for ESP32 specific code.

Your overall implemntation is much too invasive. There must be a better way to implement this specific hardware.

A possible solution is the way I implemented the Sonoff SPM hardware; it only uses one file and acts based on it's template name. The most preferred way is/was select functionality based on GPIO selection but for a POC using the template name is a much cleaner approach as it doesn't need small changes to many files.

@arendst arendst added the on hold by dev team Result - Feature request put on hold by member of development team label Dec 12, 2021
@pcdiem
Copy link
Contributor Author

pcdiem commented Dec 12, 2021

The only real extra code that is necessary to support the Linkind dimmer is the few lines of code in the light module that write the dimmer level through I2C instead of through analogWrite. There also needs to be a way to indicate that this device should use the PWM Module logic to respond to the buttons. Please let me know how you would like me to implement this so it fits in with your vision.

@arendst
Copy link
Owner

arendst commented Dec 12, 2021

Will have a look how to implement this and come back here.

@github-actions
Copy link

github-actions bot commented Jan 6, 2022

This PR has been automatically marked as stale because it hasn't any activity in last few weeks. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Action - Issue left behind - Used by the BOT to call for attention label Jan 6, 2022
@pcdiem
Copy link
Contributor Author

pcdiem commented Jan 6, 2022

Theo was going to look in to how to implement this. The code for the actual Linkind dimmer support is simply using I2C in the light module instead of setting the PWM value. In addition, there's the template. The issue that needs to be worked out is how to enable the PWM Dimmer code.

@github-actions github-actions bot removed the stale Action - Issue left behind - Used by the BOT to call for attention label Jan 6, 2022
@arendst
Copy link
Owner

arendst commented Jan 7, 2022

Will revisit ASAP

@arendst
Copy link
Owner

arendst commented Jan 7, 2022

I've been trying to implement this by using two new GPIO_OPTIONS.

During this I expect this LINKID device is not using ALL USE_PWM_DIMMER code so in addition to it's I2C code is does NOT use all PWM_DIMMER code. Am I correct.

If so we need to assume the LINKID device is a new device with some overlap of USE_PWM_DIMMER but definitly NOT full USE_PWM_DIMMER plus I2C code.

Pls enlighten me.

@tony-fav
Copy link

tony-fav commented Jan 7, 2022

The device has 2 buttons, a red led and a green led (located in the same place), and a secondary chip that manages the physical dimming and received the dimming value over I2C. So it is useful to use essentially the PWM Dimmer functionality for managing the buttons, the dimming, and the brightness of the green LED.

I suspect there may be 2 changes represented by this PR. The first change is enabling PWM Dimmer for ESP32 (could be useful to upgrade existing PWM Dimmer devices to Hardware PWM with a C3 for example). The second change is for the Linkind Dimmer device specifically, enabling the I2C write in place of the analogWrite.

arendst added a commit that referenced this pull request Jan 7, 2022
Add support for Linkind dimmer as GPIO ``Option A6`` (#14004)
@arendst
Copy link
Owner

arendst commented Jan 7, 2022

Merged my way.

For use on ESP32 Linkind load following template:

{"NAME":"ESP32-Linkind","GPIO":[6213,8448,0,0,640,0,0,0,0,288,0,0,0,0,0,0,0,608,0,0,0,544,0,0,0,0,0,0,33,32,0,0,0,0,0,0],"FLAG":0,"BASE":1}

@arendst arendst closed this Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold by dev team Result - Feature request put on hold by member of development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants