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

[pwm] Initial Contribution #10205

Merged
merged 9 commits into from
Jun 1, 2021
Merged

[pwm] Initial Contribution #10205

merged 9 commits into from
Jun 1, 2021

Conversation

fwolter
Copy link
Member

@fwolter fwolter commented Feb 20, 2021

This introduces a Pulse Width Modulation module for openHAB.

PWM can be used to control actuators continuously from 0 to 100% that only support ON/OFF commands. E.g. valves or heating burners. It accomplishes that by switching the actuator on and off with a fixed interval. The higher the control percentage (duty cycle), the longer the ON phase.

https://community.openhab.org/t/new-automation-pulse-width-modulation-pwm/117498

Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
@fwolter fwolter requested a review from a team February 20, 2021 18:32
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
The higher the control percentage (duty cycle), the longer the ON phase.

Example: If the fixed interval is 10 sec and the duty cycle is 30%, the output is ON for 3 sec and OFF for 7 sec.

Copy link
Contributor

@Skinah Skinah Feb 25, 2021

Choose a reason for hiding this comment

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

I feel a warning is necessary about relay contact life expectancy. If this example was done for a full year it would be over 1.3 Million contact cycles which would wear most cheap devices out and make even quality units fail.

@kaikreuzer kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Mar 31, 2021
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Hey @fwolter,

Many thanks for this contribution!
I am very sorry that this PR was unregarded for such a long time, despite you yourself reviewing and merging so many other PRs continuously. Feel free to nag all maintainers in such cases, you do not deserve to have your PRs not being processed quickly.

Overall, it looks good to me and I like the approach.
I was actually first wondering whether a profile might have been a better choice (and easier to use), but I think this solution here has the benefit that the input does not have to come from a binding/channel, but can be any item (like a totally virtual one that is just set by a slider in the UI).
I have a few comments below, though, happy to discuss.

bundles/org.openhab.automation.pwm/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.automation.pwm/README.md Outdated Show resolved Hide resolved
public static final String MODULE_TYPE_ID = AUTOMATION_NAME + ".trigger";
private static final Set<String> SUBSCRIBED_EVENT_TYPES = Set.of(ItemStateEvent.TYPE);
private final Logger logger = LoggerFactory.getLogger(PWMTriggerHandler.class);
private final ScheduledExecutorService scheduler = Executors
Copy link
Member

Choose a reason for hiding this comment

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

I see that we do not offer a scheduler for ModuleHandlers as we do for ThingHandlers.
I do not feel good about every ModuleHandler therefore creating it's own thread for scheduling tasks and wonder if we shouldn't change this in the framework. We could e.g. add some suitable method (like getScheduler() or maybe directly schedule()) to it and make it use a thread pool. Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking the same when implementing this. And I was dreaming of a schedule() that cancels all scheduled and still active tasks in dispose() of the super class :) Like in the modbus binding.

Copy link
Member

Choose a reason for hiding this comment

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

Just had a look. The TriggerHandlerCallbackImpl actually already has an executor internally, which is even shut down when the handler (or rather the callback) is disposed.
How about making it a scheduled executor and expose it through a method?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll file a PR.

bundles/org.openhab.automation.pwm/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.automation.pwm/README.md Outdated Show resolved Hide resolved
* @author Fabian Wolter - Initial Contribution
*/
@NonNullByDefault
public class PWMRuleTemplate extends RuleTemplate {
Copy link
Member

Choose a reason for hiding this comment

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

Nice to also see a rule template being defined.
I noticed that our Main UI is still lacking support for rule templates, is that correct?
If so, I think we should have that added asap as it is a very powerful feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I copied that from the PID controller, which copied it from Hilbrand's PID code without knowing what I was doing.

Copy link
Member

Choose a reason for hiding this comment

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

@ghys Did you already spend any thoughts on adding support for rule templates in the Main UI? After all, the Paper UI already had that feature - would be awesome to have it in the Main UI, too!

Copy link
Member

Choose a reason for hiding this comment

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

Vaguely, but I didn't look into them in detail, only saw a video or two about them and a couple examples on the Eclipse Marketplace. And didn't ever see anything but an empty list in Paper UI so kind of still considered them an experimental-grade/proof-of-concept feature, didn't know they were in actual use.

IMHO it's rather limiting to have only Java code able to contribute rule templates, they themselves should be able to be defined by the user and shared on whatever will become the new community marketplace.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO it's rather limiting to have only Java code able to contribute rule templates

This is not the case. You can also add templates as JSON files in your conf folder and it should be possible to also add a provider that stores stuff in the db, so that templates can also be managed through the UI. Would be actually cool to add a feature to convert an existing rule into a template and let the user share it (for the start as JSON on the forum, later than through a marketplace).
But the starting point would be to be able to at least use existing templates in the UI. This should actually be fairly easy to do. I can create an issue with some details, if that's ok.

Copy link
Member

Choose a reason for hiding this comment

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

Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
@fwolter
Copy link
Member Author

fwolter commented May 20, 2021

Thanks for your feedback and your words! Not a problem that it is in the backlog for some time.

fwolter added 4 commits May 29, 2021 16:15
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
@kaikreuzer kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels May 31, 2021
@kaikreuzer
Copy link
Member

Build failed with 1 SAT error. Could you please check?

Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
@kaikreuzer kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jun 1, 2021
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Hooray, finally the build is green - thanks for the contribution!

@kaikreuzer kaikreuzer merged commit 9d903c2 into openhab:main Jun 1, 2021
@kaikreuzer kaikreuzer added this to the 3.1 milestone Jun 1, 2021
@fwolter fwolter deleted the pwm branch June 1, 2021 20:18
@fwolter
Copy link
Member Author

fwolter commented Jun 1, 2021

Thank you for your review!

computergeek1507 pushed a commit to computergeek1507/openhab-addons that referenced this pull request Jul 13, 2021
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
lucacalcaterra pushed a commit to lucacalcaterra/openhab-addons that referenced this pull request Jul 26, 2021
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
Signed-off-by: Luca Calcaterra <calcaterra.luca@gmail.com>
lucacalcaterra pushed a commit to lucacalcaterra/openhab-addons that referenced this pull request Aug 3, 2021
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
Signed-off-by: Luca Calcaterra <calcaterra.luca@gmail.com>
frederictobiasc pushed a commit to frederictobiasc/openhab-addons that referenced this pull request Oct 26, 2021
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants