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

Hbridge christmas light #1251

Merged
merged 11 commits into from Nov 8, 2020
Merged

Conversation

DotNetDann
Copy link
Contributor

bringing Christmas lights into esphome

Description:

Little Tuya modules are now in Christmas lights. These normally have a hbringe so they can skimp on the long cable costs.
e.g. https://mirabellagenio.net.au/festive-lights

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#733

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder). (NOT TESTED!!)

If user exposed functionality or configuration variables are added/changed:

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

Hi @DotNetDann Please check your branching as you are pulling in commits that are unrelated. Try rebasing your branch from the latest esphome dev branch.
Thanks

Can be used for Christmas lights that use 2 wires to run 2 different strings of lights using a hbridge driver.
NOTE: I am unable to test this via the docker image
@jesserockz jesserockz dismissed their stale review November 3, 2020 02:55

Branching fixed

cv.GenerateID(CONF_OUTPUT_ID): cv.declare_id(HBRIDGELightOutput),
cv.Required(CONF_PIN_A): cv.use_id(output.FloatOutput),
cv.Required(CONF_PIN_B): cv.use_id(output.FloatOutput),
})
Copy link
Member

Choose a reason for hiding this comment

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

Im not 100% sure about this, but you could try also extend polling_component_schema so users can override the update_interval in config to speed up and slow down the switching since you are already extending PollingComponent. You can set the default 1s in the config and remove the constructor from the .h file

Suggested change
})
}).extend(cv.polling_component_schema('1s'))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried polling_component_schema originally however it is not fast enough. I actually need the loop to run as fast as it can

Copy link
Member

Choose a reason for hiding this comment

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

Ah I was getting confused about the PollingComponent and how you were using it.
So with this, both "sets" of lights would always be on?
It loops about 60 times a second because the ESPHome loop is 16ms intervals, so your update method will only be called once every 16ms even with the 1ms time on the PollingComponent

Copy link
Member

Choose a reason for hiding this comment

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

Have you tried changing it to extend Component instead of PollingComponent and then override loop instead of update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the recommended approach?
I am happy with the refresh rate. The only problem is that it can flicker when it is running other background tasks like MQTT or WIFI things. However, I don’t want to impact these tasks so was happy with it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, there is no point in setting a PollingComponent unless it can be overridden by the user in yaml. And if you want it to switch between them as fast as possible, then using loop is better as there is more logic around the PollingComponent update times etc that has to do calculations. I guess you could tie into effects like string A on for 1 second then string B on for 1 second (looping) later on. But this currently would just appear as all lights being on right? Can you post a video of what it currently looks like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PollingComponent is used for consistency. I was unable to stop flickering with the Component.
Here is a video of what it is and does - https://youtu.be/_sU-UKU6fV8

@jesserockz jesserockz self-assigned this Nov 3, 2020
Copy link
Contributor Author

@DotNetDann DotNetDann left a comment

Choose a reason for hiding this comment

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

I have changed my flag to a boolean and also fixed a bug that was caused from #947

cv.GenerateID(CONF_OUTPUT_ID): cv.declare_id(HBRIDGELightOutput),
cv.Required(CONF_PIN_A): cv.use_id(output.FloatOutput),
cv.Required(CONF_PIN_B): cv.use_id(output.FloatOutput),
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried polling_component_schema originally however it is not fast enough. I actually need the loop to run as fast as it can

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

Ill merge for now with some investigations later into the polling etc

@jesserockz jesserockz merged commit fc01a70 into esphome:dev Nov 8, 2020
@jesserockz jesserockz mentioned this pull request Jan 7, 2021
This was referenced Feb 3, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants