[mffan] Initial contribution

[mark-brooks-180]

I've developed a new binding for "Modern Forms" and "WAC Lighting" ceiling fans. The binding implements communications , and control of these WIFI connected, smart fans.

[lsiepel]

Thanks for your contribution. I'm not familiair with this device, but it looks familiair to the bigassfan binding. Maybe that is something you can use for inspiration.

Anyway, besides the comments, you should add yourself to the codeowners file and add this binding to the bom/openhab-addons/pom.xml . I prefer that you respond (done is fine) to the comments and let me mark them as resolved.
Let me know if you need anything.

[mark-brooks-180]

Thanks for your prompt reply. I'll make the suggested changes. I'll let you know when I'm done. Thank, Mark B.

…

On Wed, May 22, 2024 at 2:37 PM lsiepel ***@***.***> wrote: ***@***.**** requested changes on this pull request. Thanks for your contribution. I'm not familiair with this device, but it looks familiair to the bigassfan <https://www.openhab.org/addons/bindings/bigassfan/> binding. Maybe that is something you can use for inspiration. Anyway, besides the comments, you should add yourself to the codeowners file and add this binding to the bom/openhab-addons/pom.xml . I prefer that you respond (done is fine) to the comments and let me mark them as resolved. Let me know if you need anything. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/MfFanBindingConstants.java <#16786 (comment)> : > + * used across the whole binding. + * + * @author Mark Brooks - Initial contribution + */ ***@***.*** +public class MfFanBindingConstants { + + private static final String BINDING_ID = "mffan"; + + // List of all Thing Type UIDs + public static final ThingTypeUID THING_TYPE_MFFAN = new ThingTypeUID(BINDING_ID, BINDING_ID); + + public static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Set.of(THING_TYPE_MFFAN); + + // List of all Channel ids + public static final String CHANNEL_FAN_ON = "fanOn"; Please use lower-case-hyphen naming convention for channel id's For reference: https://www.openhab.org/docs/developer/guidelines.html#naming-convention ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/MfFanBindingConstants.java <#16786 (comment)> : > +import org.eclipse.jdt.annotation.NonNullByDefault; +import org.openhab.core.thing.ThingTypeUID; + +/** + * The ***@***.*** MfFanBindingConstants} class defines common constants, which are + * used across the whole binding. + * + * @author Mark Brooks - Initial contribution + */ ***@***.*** +public class MfFanBindingConstants { + + private static final String BINDING_ID = "mffan"; + + // List of all Thing Type UIDs + public static final ThingTypeUID THING_TYPE_MFFAN = new ThingTypeUID(BINDING_ID, BINDING_ID); The Thing id might be the same as the binding, but for completenes it is better to have them split into two constants: BINDING_ID and THING_MFFAN_ID ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + + @nullable + public ShadowBufferDto setLightPower(boolean power) { + return doPost(String.format("{\"lightOn\" : %s}", String.valueOf(power))); + } + + @nullable + public ShadowBufferDto setLightIntensity(int intensity) { + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); A start / stop should not be needed for each call. Can you make use of the common http client ? (injected from the factory class) ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + @nullable + public ShadowBufferDto setLightPower(boolean power) { + return doPost(String.format("{\"lightOn\" : %s}", String.valueOf(power))); + } + + @nullable + public ShadowBufferDto setLightIntensity(int intensity) { + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); + Request post = this.client.POST(this.url); ⬇️ Suggested change - Request post = this.client.POST(this.url); + Request postRequest= this.client.POST(this.url); ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + } + + @nullable + public ShadowBufferDto setLightIntensity(int intensity) { + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); + Request post = this.client.POST(this.url); + post.header(HttpHeader.ACCEPT, MediaType.APPLICATION_JSON); + post.header(HttpHeader.CONTENT_TYPE, MediaType.APPLICATION_JSON); + post.content(new StringContentProvider(payloadJson, Charset.forName(StandardCharsets.UTF_8.name()))); can you add an explicit timeout to the postRequest postRequest.timeout(10,SECONDS) (pseudo code) ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + @nullable + public ShadowBufferDto setLightIntensity(int intensity) { + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); + Request post = this.client.POST(this.url); + post.header(HttpHeader.ACCEPT, MediaType.APPLICATION_JSON); + post.header(HttpHeader.CONTENT_TYPE, MediaType.APPLICATION_JSON); + post.content(new StringContentProvider(payloadJson, Charset.forName(StandardCharsets.UTF_8.name()))); + ContentResponse resp = post.send(); + this.logger.debug("Response = '{}'", resp.getContentAsString()); Not sure if any authentication is invoilved, but you might check resp for isSuccess (or http statuscode) before you proceed. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + @nullable + public ShadowBufferDto setLightIntensity(int intensity) { + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); + Request post = this.client.POST(this.url); + post.header(HttpHeader.ACCEPT, MediaType.APPLICATION_JSON); + post.header(HttpHeader.CONTENT_TYPE, MediaType.APPLICATION_JSON); + post.content(new StringContentProvider(payloadJson, Charset.forName(StandardCharsets.UTF_8.name()))); + ContentResponse resp = post.send(); + this.logger.debug("Response = '{}'", resp.getContentAsString()); Debug can be used to log on what is happening. If you want to log the content it is better to use trace (or combine both) For reference: https://www.openhab.org/docs/developer/guidelines.html#f-logging ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); + Request post = this.client.POST(this.url); + post.header(HttpHeader.ACCEPT, MediaType.APPLICATION_JSON); + post.header(HttpHeader.CONTENT_TYPE, MediaType.APPLICATION_JSON); + post.content(new StringContentProvider(payloadJson, Charset.forName(StandardCharsets.UTF_8.name()))); + ContentResponse resp = post.send(); + this.logger.debug("Response = '{}'", resp.getContentAsString()); + return this.gson.fromJson(resp.getContentAsString(), ShadowBufferDto.class); + } catch (Exception e) { ⬇️ Suggested change - } catch (Exception e) { + } catch (TimeOutException | InterruptException | IOException e) { ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + try { + this.client.stop(); + } catch (Exception e) { + this.logger.warn("Exception on client stop: {}", e.getMessage()); see other common http client comment. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + + private @nullable MfFanConfiguration config; + private @nullable FanRestApi api; + private @nullable ScheduledFuture<?> pollingJob; + private ExecutorService executor; + + public MfFanHandler(Thing thing) { + super(thing); + this.executor = Executors.newSingleThreadExecutor(); + } + + @SuppressWarnings("null") + @OverRide + public void initialize() { + this.logger.debug("Initializing MfFan handler '{}'", getThing().getUID()); + this.config = getConfigAs(MfFanConfiguration.class); A common pattern is to validat4e the config before you proceed. if (!config.isValid()) { updatethingstatus(offline, CONFIG_ERROR, "detailed message"); } (pseueod code) ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + + public MfFanHandler(Thing thing) { + super(thing); + this.executor = Executors.newSingleThreadExecutor(); + } + + @SuppressWarnings("null") + @OverRide + public void initialize() { + this.logger.debug("Initializing MfFan handler '{}'", getThing().getUID()); + this.config = getConfigAs(MfFanConfiguration.class); + + updateStatus(ThingStatus.UNKNOWN); + try { + this.api = new FanRestApi(this.config.getIpAddress().trim()); + handleCommand(RefreshType.REFRESH); In the initialize() method should return fast. don;t start any http call. starting the pollingJob should be enough. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/MfFanConfiguration.java <#16786 (comment)> : > +import org.eclipse.jdt.annotation.NonNullByDefault; + +/** + * The ***@***.*** MfFanConfiguration} class contains fields mapping thing configuration parameters. + * + * @author Mark Brooks - Initial contribution + */ ***@***.*** +public class MfFanConfiguration { + + private String ipAddress; + private Integer pollingPeriod; + + public MfFanConfiguration() { + this.ipAddress = ""; + this.pollingPeriod = 0; ⬇️ Suggested change - this.pollingPeriod = 0; + this.pollingPeriod = 120; Better to set this to the default ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + + @SuppressWarnings("null") + @OverRide + public void initialize() { + this.logger.debug("Initializing MfFan handler '{}'", getThing().getUID()); + this.config = getConfigAs(MfFanConfiguration.class); + + updateStatus(ThingStatus.UNKNOWN); + try { + this.api = new FanRestApi(this.config.getIpAddress().trim()); + handleCommand(RefreshType.REFRESH); + } catch (Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR, e.getMessage()); + } + + int pollingPeriod = (this.config.getPollingPeriod() == null) ? 120 : this.config.getPollingPeriod(); this can be removed if the default is set. Maybe add this to the config.isValid() method to do some sanity checks that match the thing-typ.xml min/max values for this parameter. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + // this.pollingJob = this.scheduler.scheduleWithFixedDelay(() -> update(this.api.getShadowBuffer()), 0, + // pollingPeriod, TimeUnit.SECONDS); ⬇️ Suggested change - // this.pollingJob = this.scheduler.scheduleWithFixedDelay(() -> update(this.api.getShadowBuffer()), 0, - // pollingPeriod, TimeUnit.SECONDS); ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > ***@***.*** +public class MfFanHandler extends BaseThingHandler { + + private final Logger logger = LoggerFactory.getLogger(MfFanHandler.class); + + private @nullable MfFanConfiguration config; + private @nullable FanRestApi api; + private @nullable ScheduledFuture<?> pollingJob; + private ExecutorService executor; + + public MfFanHandler(Thing thing) { + super(thing); + this.executor = Executors.newSingleThreadExecutor(); + } + + @SuppressWarnings("null") Try to prevent this null suppression ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + // + // Cancel the polling job. + // + if (this.pollingJob != null) { + this.pollingJob.cancel(true); ⬇️ Suggested change - // - // Cancel the polling job. - // - if (this.pollingJob != null) { - this.pollingJob.cancel(true); + ScheduledFuture<?> pollingJob = this.pollingJob; + if (pollingJob != null) { + pollingJob.cancel(true); ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + // + // Shutdown the executor. + // + this.executor.shutdown(); + try { + this.executor.awaitTermination(2, TimeUnit.SECONDS); + } catch ***@***.***("unused") InterruptedException e) { + this.executor.shutdownNow(); + } the exact same pattern as pollingJob can be used for the executor ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + this.pollingJob.cancel(true); + this.pollingJob = null; + } + + // + // Shutdown the executor. + // + this.executor.shutdown(); + try { + this.executor.awaitTermination(2, TimeUnit.SECONDS); + } catch ***@***.***("unused") InterruptedException e) { + this.executor.shutdownNow(); + } + } + + @SuppressWarnings("null") ⬇️ Suggested change - @SuppressWarnings("null") ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + this.executor.execute(new CommandHandlerRunnable(channelUID, command)); + } catch ***@***.***("unused") Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, see earlier comment ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + @SuppressWarnings("null") + @OverRide + public void handleCommand(ChannelUID channelUID, Command command) { + try { + this.executor.execute(new CommandHandlerRunnable(channelUID, command)); + } catch ***@***.***("unused") Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + String.format("Could not control device at IP address %s", this.config.getIpAddress())); + } + } + + private void handleCommand(Command command) { + handleCommand(new ChannelUID("Null:Null:Null:Null"), command); + } + + private final class CommandHandlerRunnable implements Runnable { Might be valid, but this is not a common pattern i see in most bindings, can you explain a bit on why it is moddled this way and not like many other bindings handling the commands inside the handdler class? ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/RestApiException.java <#16786 (comment)> : > + * http://www.eclipse.org/legal/epl-2.0 + * + * SPDX-License-Identifier: EPL-2.0 + */ +package org.openhab.binding.mffan.internal.handler; + +import org.eclipse.jdt.annotation.NonNullByDefault; + +/** + * The ***@***.*** RestApiException} is an exception thrown from the REST API. + * + * @author Mark Brooks - Initial contribution + */ ***@***.*** +public class RestApiException extends RuntimeException { + private static final long serialVersionUID = 1L; Would be nice if you can create/generate a serialVersionUID other than 1L ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/ShadowBufferDto.java <#16786 (comment)> : > @@ -0,0 +1,223 @@ +/** I would suggest to move the API to a different folder and package. bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/api/ Files FanRestApi.java, RestApiException.java and ShadowBufferDto.java ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/addon/addon.xml <#16786 (comment)> : > @@ -0,0 +1,10 @@ +<?xml version="1.0" encoding="UTF-8"?> +<addon:addon id="mffan" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xmlns:addon="https://openhab.org/schemas/addon/v1.0.0" + xsi:schemaLocation="https://openhab.org/schemas/addon/v1.0.0 https://openhab.org/schemas/addon-1.0.0.xsd"> + + <type>binding</type> + <name>Modern Forms Fan Binding</name> + <description>This is the binding for "Modern Forms", and "WAC Lighting" smart ceiling fans.</description> + Can you add connection tag and maybe country and discovery ? (last two might be more difficult) ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/i18n/mffan.properties <#16786 (comment)> : > @@ -0,0 +1,3 @@ +# FIXME: please add all English translations to this file so the texts can be translated using Crowdin FIXME :-) ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/thing/mffan.xml <#16786 (comment)> : > @@ -0,0 +1,97 @@ +<?xml version="1.0" encoding="UTF-8"?> +<thing:thing-descriptions bindingId="mffan" + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xmlns:thing="https://openhab.org/schemas/thing-description/v1.0.0" + xsi:schemaLocation="https://openhab.org/schemas/thing-description/v1.0.0 https://openhab.org/schemas/thing-description-1.0.0.xsd"> + + <thing-type id="mffan"> + + <label>Modern Forms Fan</label> + <description>Modern Forms and WAC Lighting Smart Ceiling Fans</description> + + <channels> + <channel id="fanOn" typeId="fanOn"/> See earlier comment about channel id's that should be lower-case-hyphen ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/thing/mffan.xml <#16786 (comment)> : > + <config-description> + <parameter name="ipAddress" type="text" required="true"> + <label>IP or Host</label> + <description>IP address or host name of the fan.</description> + </parameter> + + <parameter name="pollingPeriod" type="integer" unit="s" min="1"> + <label>Refresh Interval</label> + <description>Interval the device is polled in seconds.</description> + <default>120</default> + <advanced>true</advanced> + </parameter> + </config-description> + </thing-type> + + <channel-type id="fanOn"> lower-case-hyphen naming convention also applies to channel type id's ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/thing/mffan.xml <#16786 (comment)> : > + <description>The amount of the wind being produced.</description> + <state readOnly="false" pattern="Wind %s"> + <options> + <option value="1">Level 1</option> + <option value="2">Level 2</option> + <option value="3">Level 3</option> + </options> + </state> + </channel-type> + <channel-type id="lightOn"> + <item-type>Switch</item-type> + <label>Light</label> + <description>Light on/off</description> + </channel-type> + <channel-type id="lightIntensity"> + <item-type>Dimmer</item-type> ⬇️ Suggested change - <item-type>Dimmer</item-type> + <item-type>Number:Dimensionless</item-type> ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/thing/mffan.xml <#16786 (comment)> : > + <option value="1">Level 1</option> + <option value="2">Level 2</option> + <option value="3">Level 3</option> + </options> + </state> + </channel-type> + <channel-type id="lightOn"> + <item-type>Switch</item-type> + <label>Light</label> + <description>Light on/off</description> + </channel-type> + <channel-type id="lightIntensity"> + <item-type>Dimmer</item-type> + <label>Light Intensity</label> + <description>Light intensity</description> + <state readOnly="false" min="1" max="100" step="1"/> ⬇️ Suggested change - <state readOnly="false" min="1" max="100" step="1"/> + <state readOnly="false" min="1" max="100" step="1" pattern="%.0f %unit%" /> ------------------------------ In bundles/pom.xml <#16786 (comment)> : > @@ -1,690 +1,2706 @@ <?xml version="1.0" encoding="UTF-8" standalone="no"?> Something weird happend to this file, please revert / only change the line to add this binding. ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > @@ -0,0 +1,69 @@ +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + ⬇️ Suggested change - ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > @@ -0,0 +1,69 @@ +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + + +## Supported Things + +Supported things include: "Modern Forms" and "WAC Lighting" smart ceiling fans. The binding has been tested with both. This are the supported devices, not the supported Things. Maybe add these devices as tested or somehting and use the Thing Id listed in the thing-type.xml ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > @@ -0,0 +1,69 @@ +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + + +## Supported Things + +Supported things include: "Modern Forms" and "WAC Lighting" smart ceiling fans. The binding has been tested with both. + +## Discovery + +Auto discovery is not supported at this time. +To bind with a smart fan either the IP address of the fan, or the local host name of the fan must be provided. ⬇️ Suggested change -To bind with a smart fan either the IP address of the fan, or the local host name of the fan must be provided. Those details belong to the configuration section ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > @@ -0,0 +1,69 @@ +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + + +## Supported Things + +Supported things include: "Modern Forms" and "WAC Lighting" smart ceiling fans. The binding has been tested with both. + +## Discovery + +Auto discovery is not supported at this time. +To bind with a smart fan either the IP address of the fan, or the local host name of the fan must be provided. +The binding has been tested using an IPV4 local network address, and a host name served by a local DNS. ⬇️ Suggested change -The binding has been tested using an IPV4 local network address, and a host name served by a local DNS. Not sure if this should be mentioned at all as the configuration explains hsotname/ipadres (maybe add IPV4 to that descritpion) ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + + +## Supported Things + +Supported things include: "Modern Forms" and "WAC Lighting" smart ceiling fans. The binding has been tested with both. + +## Discovery + +Auto discovery is not supported at this time. +To bind with a smart fan either the IP address of the fan, or the local host name of the fan must be provided. +The binding has been tested using an IPV4 local network address, and a host name served by a local DNS. + +## Binding Configuration ⬇️ Suggested change -## Binding Configuration +## Thing Configuration ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > +To bind with a smart fan either the IP address of the fan, or the local host name of the fan must be provided. +The binding has been tested using an IPV4 local network address, and a host name served by a local DNS. + +## Binding Configuration + + +| Name | Type | Description | Default | Required | Advanced | +|-----------------|---------|---------------------------------------|---------|----------|----------| +| hostname | text | Hostname or IP address of the device | N/A | yes | no | +| refreshInterval | integer | Interval the device is polled in sec. | 120 | no | yes | + +## Channels + +| Channel | Type | Read/Write | Description | +|----------------|--------|------------|-------------------------------------| +| fanOn | Switch | RW | Channel that turns the fan on/off. | See other comment about lower-case-hyphen convention — Reply to this email directly, view it on GitHub <#16786 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI3WWNR3QV4V3SZLMIYKOBDZDTQXHAVCNFSM6AAAAABIDWOQMKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZRHEZTQNZTGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[mark-brooks-180]

------------------------------ lsiepel, I believe I have all of the changes you recommended completed. The corruption of file In bundles/pom.xml <#16786 (comment)> was resolved by resynchronizing my fork with the upstream and then recommitting my changes. The two issues listed below require some explanation: - I'm using a single thread executor to serialize command handler execution. - The actual commands are executed by runnables submitted to the executor. This ensures the commands are executed in the order they are received, and potential delays resulting from calls to the REST API functions don't impact the performance of calls to handleCommand(ChannelUID channelUID, Command command). - In a similar manner the polling thread calls a private version of handleCommand to submit a REFRESH command. This ensures commands are serialized by the executor, and avoids any thread contention when updating the state of the Thing. Thanks, Mark B. In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> :

+ // this.pollingJob = this.scheduler.scheduleWithFixedDelay(() -> update(this.api.getShadowBuffer()), 0,

+ // pollingPeriod, TimeUnit.SECONDS); ⬇️ Suggested change - // this.pollingJob = this.scheduler.scheduleWithFixedDelay(() -> update(this.api.getShadowBuffer()), 0, - // pollingPeriod, TimeUnit.SECONDS); In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> :

+ @SuppressWarnings("null")

+ @OverRide + public void handleCommand(ChannelUID channelUID, Command command) { + try { + this.executor.execute(new CommandHandlerRunnable(channelUID, command)); + } catch ***@***.***("unused") Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + String.format("Could not control device at IP address %s", this.config.getIpAddress())); + } + } + + private void handleCommand(Command command) { + handleCommand(new ChannelUID("Null:Null:Null:Null"), command); + } + + private final class CommandHandlerRunnable implements Runnable { Might be valid, but this is not a common pattern i see in most bindings, can you explain a bit on why it is moddled this way and not like many other bindings handling the commands inside the handdler class?

…

On Wed, May 22, 2024 at 2:37 PM lsiepel ***@***.***> wrote: ***@***.**** requested changes on this pull request. Thanks for your contribution. I'm not familiair with this device, but it looks familiair to the bigassfan <https://www.openhab.org/addons/bindings/bigassfan/> binding. Maybe that is something you can use for inspiration. Anyway, besides the comments, you should add yourself to the codeowners file and add this binding to the bom/openhab-addons/pom.xml . I prefer that you respond (done is fine) to the comments and let me mark them as resolved. Let me know if you need anything. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/MfFanBindingConstants.java <#16786 (comment)> : > + * used across the whole binding. + * + * @author Mark Brooks - Initial contribution + */ ***@***.*** +public class MfFanBindingConstants { + + private static final String BINDING_ID = "mffan"; + + // List of all Thing Type UIDs + public static final ThingTypeUID THING_TYPE_MFFAN = new ThingTypeUID(BINDING_ID, BINDING_ID); + + public static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Set.of(THING_TYPE_MFFAN); + + // List of all Channel ids + public static final String CHANNEL_FAN_ON = "fanOn"; Please use lower-case-hyphen naming convention for channel id's For reference: https://www.openhab.org/docs/developer/guidelines.html#naming-convention ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/MfFanBindingConstants.java <#16786 (comment)> : > +import org.eclipse.jdt.annotation.NonNullByDefault; +import org.openhab.core.thing.ThingTypeUID; + +/** + * The ***@***.*** MfFanBindingConstants} class defines common constants, which are + * used across the whole binding. + * + * @author Mark Brooks - Initial contribution + */ ***@***.*** +public class MfFanBindingConstants { + + private static final String BINDING_ID = "mffan"; + + // List of all Thing Type UIDs + public static final ThingTypeUID THING_TYPE_MFFAN = new ThingTypeUID(BINDING_ID, BINDING_ID); The Thing id might be the same as the binding, but for completenes it is better to have them split into two constants: BINDING_ID and THING_MFFAN_ID ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + + @nullable + public ShadowBufferDto setLightPower(boolean power) { + return doPost(String.format("{\"lightOn\" : %s}", String.valueOf(power))); + } + + @nullable + public ShadowBufferDto setLightIntensity(int intensity) { + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); A start / stop should not be needed for each call. Can you make use of the common http client ? (injected from the factory class) ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + @nullable + public ShadowBufferDto setLightPower(boolean power) { + return doPost(String.format("{\"lightOn\" : %s}", String.valueOf(power))); + } + + @nullable + public ShadowBufferDto setLightIntensity(int intensity) { + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); + Request post = this.client.POST(this.url); ⬇️ Suggested change - Request post = this.client.POST(this.url); + Request postRequest= this.client.POST(this.url); ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + } + + @nullable + public ShadowBufferDto setLightIntensity(int intensity) { + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); + Request post = this.client.POST(this.url); + post.header(HttpHeader.ACCEPT, MediaType.APPLICATION_JSON); + post.header(HttpHeader.CONTENT_TYPE, MediaType.APPLICATION_JSON); + post.content(new StringContentProvider(payloadJson, Charset.forName(StandardCharsets.UTF_8.name()))); can you add an explicit timeout to the postRequest postRequest.timeout(10,SECONDS) (pseudo code) ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + @nullable + public ShadowBufferDto setLightIntensity(int intensity) { + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); + Request post = this.client.POST(this.url); + post.header(HttpHeader.ACCEPT, MediaType.APPLICATION_JSON); + post.header(HttpHeader.CONTENT_TYPE, MediaType.APPLICATION_JSON); + post.content(new StringContentProvider(payloadJson, Charset.forName(StandardCharsets.UTF_8.name()))); + ContentResponse resp = post.send(); + this.logger.debug("Response = '{}'", resp.getContentAsString()); Not sure if any authentication is invoilved, but you might check resp for isSuccess (or http statuscode) before you proceed. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + @nullable + public ShadowBufferDto setLightIntensity(int intensity) { + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); + Request post = this.client.POST(this.url); + post.header(HttpHeader.ACCEPT, MediaType.APPLICATION_JSON); + post.header(HttpHeader.CONTENT_TYPE, MediaType.APPLICATION_JSON); + post.content(new StringContentProvider(payloadJson, Charset.forName(StandardCharsets.UTF_8.name()))); + ContentResponse resp = post.send(); + this.logger.debug("Response = '{}'", resp.getContentAsString()); Debug can be used to log on what is happening. If you want to log the content it is better to use trace (or combine both) For reference: https://www.openhab.org/docs/developer/guidelines.html#f-logging ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + return doPost(String.format("{\"lightBrightness\" : %d}", intensity)); + } + + @nullable + private synchronized ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + this.client.start(); + Request post = this.client.POST(this.url); + post.header(HttpHeader.ACCEPT, MediaType.APPLICATION_JSON); + post.header(HttpHeader.CONTENT_TYPE, MediaType.APPLICATION_JSON); + post.content(new StringContentProvider(payloadJson, Charset.forName(StandardCharsets.UTF_8.name()))); + ContentResponse resp = post.send(); + this.logger.debug("Response = '{}'", resp.getContentAsString()); + return this.gson.fromJson(resp.getContentAsString(), ShadowBufferDto.class); + } catch (Exception e) { ⬇️ Suggested change - } catch (Exception e) { + } catch (TimeOutException | InterruptException | IOException e) { ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/FanRestApi.java <#16786 (comment)> : > + try { + this.client.stop(); + } catch (Exception e) { + this.logger.warn("Exception on client stop: {}", e.getMessage()); see other common http client comment. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + + private @nullable MfFanConfiguration config; + private @nullable FanRestApi api; + private @nullable ScheduledFuture<?> pollingJob; + private ExecutorService executor; + + public MfFanHandler(Thing thing) { + super(thing); + this.executor = Executors.newSingleThreadExecutor(); + } + + @SuppressWarnings("null") + @OverRide + public void initialize() { + this.logger.debug("Initializing MfFan handler '{}'", getThing().getUID()); + this.config = getConfigAs(MfFanConfiguration.class); A common pattern is to validat4e the config before you proceed. if (!config.isValid()) { updatethingstatus(offline, CONFIG_ERROR, "detailed message"); } (pseueod code) ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + + public MfFanHandler(Thing thing) { + super(thing); + this.executor = Executors.newSingleThreadExecutor(); + } + + @SuppressWarnings("null") + @OverRide + public void initialize() { + this.logger.debug("Initializing MfFan handler '{}'", getThing().getUID()); + this.config = getConfigAs(MfFanConfiguration.class); + + updateStatus(ThingStatus.UNKNOWN); + try { + this.api = new FanRestApi(this.config.getIpAddress().trim()); + handleCommand(RefreshType.REFRESH); In the initialize() method should return fast. don;t start any http call. starting the pollingJob should be enough. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/MfFanConfiguration.java <#16786 (comment)> : > +import org.eclipse.jdt.annotation.NonNullByDefault; + +/** + * The ***@***.*** MfFanConfiguration} class contains fields mapping thing configuration parameters. + * + * @author Mark Brooks - Initial contribution + */ ***@***.*** +public class MfFanConfiguration { + + private String ipAddress; + private Integer pollingPeriod; + + public MfFanConfiguration() { + this.ipAddress = ""; + this.pollingPeriod = 0; ⬇️ Suggested change - this.pollingPeriod = 0; + this.pollingPeriod = 120; Better to set this to the default ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + + @SuppressWarnings("null") + @OverRide + public void initialize() { + this.logger.debug("Initializing MfFan handler '{}'", getThing().getUID()); + this.config = getConfigAs(MfFanConfiguration.class); + + updateStatus(ThingStatus.UNKNOWN); + try { + this.api = new FanRestApi(this.config.getIpAddress().trim()); + handleCommand(RefreshType.REFRESH); + } catch (Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR, e.getMessage()); + } + + int pollingPeriod = (this.config.getPollingPeriod() == null) ? 120 : this.config.getPollingPeriod(); this can be removed if the default is set. Maybe add this to the config.isValid() method to do some sanity checks that match the thing-typ.xml min/max values for this parameter. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + // this.pollingJob = this.scheduler.scheduleWithFixedDelay(() -> update(this.api.getShadowBuffer()), 0, + // pollingPeriod, TimeUnit.SECONDS); ⬇️ Suggested change - // this.pollingJob = this.scheduler.scheduleWithFixedDelay(() -> update(this.api.getShadowBuffer()), 0, - // pollingPeriod, TimeUnit.SECONDS); ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > ***@***.*** +public class MfFanHandler extends BaseThingHandler { + + private final Logger logger = LoggerFactory.getLogger(MfFanHandler.class); + + private @nullable MfFanConfiguration config; + private @nullable FanRestApi api; + private @nullable ScheduledFuture<?> pollingJob; + private ExecutorService executor; + + public MfFanHandler(Thing thing) { + super(thing); + this.executor = Executors.newSingleThreadExecutor(); + } + + @SuppressWarnings("null") Try to prevent this null suppression ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + // + // Cancel the polling job. + // + if (this.pollingJob != null) { + this.pollingJob.cancel(true); ⬇️ Suggested change - // - // Cancel the polling job. - // - if (this.pollingJob != null) { - this.pollingJob.cancel(true); + ScheduledFuture<?> pollingJob = this.pollingJob; + if (pollingJob != null) { + pollingJob.cancel(true); ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + // + // Shutdown the executor. + // + this.executor.shutdown(); + try { + this.executor.awaitTermination(2, TimeUnit.SECONDS); + } catch ***@***.***("unused") InterruptedException e) { + this.executor.shutdownNow(); + } the exact same pattern as pollingJob can be used for the executor ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + this.pollingJob.cancel(true); + this.pollingJob = null; + } + + // + // Shutdown the executor. + // + this.executor.shutdown(); + try { + this.executor.awaitTermination(2, TimeUnit.SECONDS); + } catch ***@***.***("unused") InterruptedException e) { + this.executor.shutdownNow(); + } + } + + @SuppressWarnings("null") ⬇️ Suggested change - @SuppressWarnings("null") ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + this.executor.execute(new CommandHandlerRunnable(channelUID, command)); + } catch ***@***.***("unused") Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, see earlier comment ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + @SuppressWarnings("null") + @OverRide + public void handleCommand(ChannelUID channelUID, Command command) { + try { + this.executor.execute(new CommandHandlerRunnable(channelUID, command)); + } catch ***@***.***("unused") Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + String.format("Could not control device at IP address %s", this.config.getIpAddress())); + } + } + + private void handleCommand(Command command) { + handleCommand(new ChannelUID("Null:Null:Null:Null"), command); + } + + private final class CommandHandlerRunnable implements Runnable { Might be valid, but this is not a common pattern i see in most bindings, can you explain a bit on why it is moddled this way and not like many other bindings handling the commands inside the handdler class? ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/RestApiException.java <#16786 (comment)> : > + * http://www.eclipse.org/legal/epl-2.0 + * + * SPDX-License-Identifier: EPL-2.0 + */ +package org.openhab.binding.mffan.internal.handler; + +import org.eclipse.jdt.annotation.NonNullByDefault; + +/** + * The ***@***.*** RestApiException} is an exception thrown from the REST API. + * + * @author Mark Brooks - Initial contribution + */ ***@***.*** +public class RestApiException extends RuntimeException { + private static final long serialVersionUID = 1L; Would be nice if you can create/generate a serialVersionUID other than 1L ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/ShadowBufferDto.java <#16786 (comment)> : > @@ -0,0 +1,223 @@ +/** I would suggest to move the API to a different folder and package. bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/api/ Files FanRestApi.java, RestApiException.java and ShadowBufferDto.java ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/addon/addon.xml <#16786 (comment)> : > @@ -0,0 +1,10 @@ +<?xml version="1.0" encoding="UTF-8"?> +<addon:addon id="mffan" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xmlns:addon="https://openhab.org/schemas/addon/v1.0.0" + xsi:schemaLocation="https://openhab.org/schemas/addon/v1.0.0 https://openhab.org/schemas/addon-1.0.0.xsd"> + + <type>binding</type> + <name>Modern Forms Fan Binding</name> + <description>This is the binding for "Modern Forms", and "WAC Lighting" smart ceiling fans.</description> + Can you add connection tag and maybe country and discovery ? (last two might be more difficult) ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/i18n/mffan.properties <#16786 (comment)> : > @@ -0,0 +1,3 @@ +# FIXME: please add all English translations to this file so the texts can be translated using Crowdin FIXME :-) ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/thing/mffan.xml <#16786 (comment)> : > @@ -0,0 +1,97 @@ +<?xml version="1.0" encoding="UTF-8"?> +<thing:thing-descriptions bindingId="mffan" + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xmlns:thing="https://openhab.org/schemas/thing-description/v1.0.0" + xsi:schemaLocation="https://openhab.org/schemas/thing-description/v1.0.0 https://openhab.org/schemas/thing-description-1.0.0.xsd"> + + <thing-type id="mffan"> + + <label>Modern Forms Fan</label> + <description>Modern Forms and WAC Lighting Smart Ceiling Fans</description> + + <channels> + <channel id="fanOn" typeId="fanOn"/> See earlier comment about channel id's that should be lower-case-hyphen ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/thing/mffan.xml <#16786 (comment)> : > + <config-description> + <parameter name="ipAddress" type="text" required="true"> + <label>IP or Host</label> + <description>IP address or host name of the fan.</description> + </parameter> + + <parameter name="pollingPeriod" type="integer" unit="s" min="1"> + <label>Refresh Interval</label> + <description>Interval the device is polled in seconds.</description> + <default>120</default> + <advanced>true</advanced> + </parameter> + </config-description> + </thing-type> + + <channel-type id="fanOn"> lower-case-hyphen naming convention also applies to channel type id's ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/thing/mffan.xml <#16786 (comment)> : > + <description>The amount of the wind being produced.</description> + <state readOnly="false" pattern="Wind %s"> + <options> + <option value="1">Level 1</option> + <option value="2">Level 2</option> + <option value="3">Level 3</option> + </options> + </state> + </channel-type> + <channel-type id="lightOn"> + <item-type>Switch</item-type> + <label>Light</label> + <description>Light on/off</description> + </channel-type> + <channel-type id="lightIntensity"> + <item-type>Dimmer</item-type> ⬇️ Suggested change - <item-type>Dimmer</item-type> + <item-type>Number:Dimensionless</item-type> ------------------------------ In bundles/org.openhab.binding.mffan/src/main/resources/OH-INF/thing/mffan.xml <#16786 (comment)> : > + <option value="1">Level 1</option> + <option value="2">Level 2</option> + <option value="3">Level 3</option> + </options> + </state> + </channel-type> + <channel-type id="lightOn"> + <item-type>Switch</item-type> + <label>Light</label> + <description>Light on/off</description> + </channel-type> + <channel-type id="lightIntensity"> + <item-type>Dimmer</item-type> + <label>Light Intensity</label> + <description>Light intensity</description> + <state readOnly="false" min="1" max="100" step="1"/> ⬇️ Suggested change - <state readOnly="false" min="1" max="100" step="1"/> + <state readOnly="false" min="1" max="100" step="1" pattern="%.0f %unit%" /> ------------------------------ In bundles/pom.xml <#16786 (comment)> : > @@ -1,690 +1,2706 @@ <?xml version="1.0" encoding="UTF-8" standalone="no"?> Something weird happend to this file, please revert / only change the line to add this binding. ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > @@ -0,0 +1,69 @@ +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + ⬇️ Suggested change - ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > @@ -0,0 +1,69 @@ +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + + +## Supported Things + +Supported things include: "Modern Forms" and "WAC Lighting" smart ceiling fans. The binding has been tested with both. This are the supported devices, not the supported Things. Maybe add these devices as tested or somehting and use the Thing Id listed in the thing-type.xml ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > @@ -0,0 +1,69 @@ +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + + +## Supported Things + +Supported things include: "Modern Forms" and "WAC Lighting" smart ceiling fans. The binding has been tested with both. + +## Discovery + +Auto discovery is not supported at this time. +To bind with a smart fan either the IP address of the fan, or the local host name of the fan must be provided. ⬇️ Suggested change -To bind with a smart fan either the IP address of the fan, or the local host name of the fan must be provided. Those details belong to the configuration section ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > @@ -0,0 +1,69 @@ +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + + +## Supported Things + +Supported things include: "Modern Forms" and "WAC Lighting" smart ceiling fans. The binding has been tested with both. + +## Discovery + +Auto discovery is not supported at this time. +To bind with a smart fan either the IP address of the fan, or the local host name of the fan must be provided. +The binding has been tested using an IPV4 local network address, and a host name served by a local DNS. ⬇️ Suggested change -The binding has been tested using an IPV4 local network address, and a host name served by a local DNS. Not sure if this should be mentioned at all as the configuration explains hsotname/ipadres (maybe add IPV4 to that descritpion) ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + + +## Supported Things + +Supported things include: "Modern Forms" and "WAC Lighting" smart ceiling fans. The binding has been tested with both. + +## Discovery + +Auto discovery is not supported at this time. +To bind with a smart fan either the IP address of the fan, or the local host name of the fan must be provided. +The binding has been tested using an IPV4 local network address, and a host name served by a local DNS. + +## Binding Configuration ⬇️ Suggested change -## Binding Configuration +## Thing Configuration ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > +To bind with a smart fan either the IP address of the fan, or the local host name of the fan must be provided. +The binding has been tested using an IPV4 local network address, and a host name served by a local DNS. + +## Binding Configuration + + +| Name | Type | Description | Default | Required | Advanced | +|-----------------|---------|---------------------------------------|---------|----------|----------| +| hostname | text | Hostname or IP address of the device | N/A | yes | no | +| refreshInterval | integer | Interval the device is polled in sec. | 120 | no | yes | + +## Channels + +| Channel | Type | Read/Write | Description | +|----------------|--------|------------|-------------------------------------| +| fanOn | Switch | RW | Channel that turns the fan on/off. | See other comment about lower-case-hyphen convention — Reply to this email directly, view it on GitHub <#16786 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI3WWNR3QV4V3SZLMIYKOBDZDTQXHAVCNFSM6AAAAABIDWOQMKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZRHEZTQNZTGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[lsiepel]

I think something went wrong as the pr is closed due to no files. It should not be needed to do a force push. Can you fix this?

[mark-brooks-180]

Isiepel, I believe I fixed the issue. The pull request has been reopened. Thanks, Mark B.

…

On Thu, May 23, 2024 at 3:12 PM lsiepel ***@***.***> wrote: I think something went wrong as the pr is closed due to no files. It should not be needed to do a force push. Can you fix this? — Reply to this email directly, view it on GitHub <#16786 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI3WWNUXLJ5WT2CMSUHFYRTZDY5RJAVCNFSM6AAAAABIDWOQMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRXHA2TMNZWG4> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[lsiepel]

Some, but not all previous comments are addressed. I also see some comments being lost after the changes. Ping me when your are ready.

[mark-brooks-180]

Isiepel, I believe I've made all the recommended changes and pushed them to main. I tested the binding. Everything looks good on my end. Thanks, Mark B.

…

On Fri, May 24, 2024 at 2:46 PM lsiepel ***@***.***> wrote: ***@***.**** requested changes on this pull request. Some, but not all previous comments are addressed. I also see som ecomments being lost after the changes. Ping me when your are ready. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/api/FanRestApi.java <#16786 (comment)> : > + private ShadowBufferDto doPost(String payloadJson) { + try { + this.logger.debug("Performing Post: 'URL: {}, Payload: '{}'", this.url, payloadJson); + Request postRequest = this.client.POST(this.url); + postRequest.timeout(10, TimeUnit.SECONDS); + postRequest.header(HttpHeader.ACCEPT, MediaType.APPLICATION_JSON); + postRequest.header(HttpHeader.CONTENT_TYPE, MediaType.APPLICATION_JSON); + postRequest.content(new StringContentProvider(payloadJson, Charset.forName(StandardCharsets.UTF_8.name()))); + ContentResponse postResponse = postRequest.send(); + this.logger.debug("Response status: {}", postResponse.getStatus()); + if (postResponse.getStatus() == 200) { + this.logger.trace("Post Response Content = '{}'", postResponse.getContentAsString()); + return this.gson.fromJson(postResponse.getContentAsString(), ShadowBufferDto.class); + } + } catch (JsonSyntaxException | InterruptedException | TimeoutException | ExecutionException e) { + ⬇️ Suggested change - ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/api/RestApiException.java <#16786 (comment)> : > + * terms of the Eclipse Public License 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0 + * + * SPDX-License-Identifier: EPL-2.0 + */ +package org.openhab.binding.mffan.internal.api; + +import org.eclipse.jdt.annotation.NonNullByDefault; + +/** + * The ***@***.*** RestApiException} is an exception thrown from the REST API. + * + * @author Mark Brooks - Initial contribution + */ ***@***.*** +public class RestApiException extends RuntimeException { ⬇️ Suggested change -public class RestApiException extends RuntimeException { +public class RestApiException extends Exception { ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + + public MfFanHandler(Thing thing, HttpClientFactory httpClientFactory) { + super(thing); + this.httpClientFactory = httpClientFactory; + this.executor = Executors.newSingleThreadExecutor(); + } + + @OverRide + public void initialize() { + this.logger.debug("Initializing MfFan handler '{}'", getThing().getUID()); + updateStatus(ThingStatus.UNKNOWN); + this.config = getConfigAs(MfFanConfiguration.class); + if (validateConfig(this.config)) { + try { + this.api = new FanRestApi(this.config.getIpAddress().trim(), this.httpClientFactory); + // handleCommand(RefreshType.REFRESH); ⬇️ Suggested change - // handleCommand(RefreshType.REFRESH); ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + } catch (Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR, e.getMessage()); + } this try/catch block is not needed as no exception is thrown. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + private final ExecutorService executor; + + private final HttpClientFactory httpClientFactory; + + public MfFanHandler(Thing thing, HttpClientFactory httpClientFactory) { + super(thing); + this.httpClientFactory = httpClientFactory; + this.executor = Executors.newSingleThreadExecutor(); + } + + @OverRide + public void initialize() { + this.logger.debug("Initializing MfFan handler '{}'", getThing().getUID()); + updateStatus(ThingStatus.UNKNOWN); + this.config = getConfigAs(MfFanConfiguration.class); + if (validateConfig(this.config)) { Don't want to be too rigid, so this comment is for your consideration, but i think the MfFanConfiguration class should be responsible for validating itself, not the handler. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + this.config = getConfigAs(MfFanConfiguration.class); + if (validateConfig(this.config)) { + try { + this.api = new FanRestApi(this.config.getIpAddress().trim(), this.httpClientFactory); + // handleCommand(RefreshType.REFRESH); + } catch (Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR, e.getMessage()); + } + + this.pollingJob = this.scheduler.scheduleWithFixedDelay(() -> handleCommand(RefreshType.REFRESH), 0, + this.config.getPollingPeriod(), TimeUnit.SECONDS); + + this.logger.debug("Polling job scheduled to run every {} sec. for '{}'", this.config.getPollingPeriod(), + getThing().getUID()); + } else { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "Invalid configuration detected."); I would recommend to change this if/else block if (validateConfig(this.config)) { // set thing state; return; // exit this method } // set pollingjob etc. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + + this.logger.debug("Polling job scheduled to run every {} sec. for '{}'", this.config.getPollingPeriod(), + getThing().getUID()); + } else { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "Invalid configuration detected."); + } + } + + @OverRide + public void dispose() { + this.logger.debug("Disposing MF fan handler '{}'", getThing().getUID()); + + ScheduledFuture<?> job = this.pollingJob; + if (job != null) { + job.cancel(true); + } ⬇️ Suggested change - } + this.pollingJob = null; + } ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + ExecutorService exec = this.executor; + exec.shutdown(); + try { + exec.awaitTermination(2, TimeUnit.SECONDS); + } catch ***@***.***("unused") InterruptedException e) { + exec.shutdownNow(); + } Why is this pattern not the same is with the pollingjob? ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + } + } else if (channelUID.getId().equals(MfFanBindingConstants.CHANNEL_LIGHT_INTENSITY)) { + if (command instanceof PercentType percentCommand) { + update(MfFanHandler.this.api.setLightIntensity(percentCommand.intValue())); + } + } else if (channelUID.getId().equals(MfFanBindingConstants.CHANNEL_WIND_ON)) { + if (command instanceof OnOffType onOffCommand) { + update(MfFanHandler.this.api.setWindPower(onOffCommand == OnOffType.ON)); + } + } else if (channelUID.getId().equals(MfFanBindingConstants.CHANNEL_WIND_LEVEL)) { + if (command instanceof StringType stringCommand) { + update(MfFanHandler.this.api.setWindSpeed(Integer.valueOf(stringCommand.toString()))); + } + } else { + MfFanHandler.this.logger.warn("Skipping command. Unidentified channel id '{}'", channelUID.getId()); + ⬇️ Suggested change - ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + + if (!getThing().getStatus().equals(ThingStatus.ONLINE)) { + updateStatus(ThingStatus.ONLINE); + } + } else { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR, + "Null shadow buffer returned."); + } + } catch (Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR, e.getMessage()); + } + } + } + + private static boolean ***@***.*** MfFanConfiguration config) { + if ((config == null) || (config.getIpAddress().trim().length() == 0)) { ⬇️ Suggested change - if ((config == null) || (config.getIpAddress().trim().length() == 0)) { + if (config == null || config.getIpAddress().isBlank()) { ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + if (config.getPollingPeriod() < 10) { + return false; + } + return true; ⬇️ Suggested change - if (config.getPollingPeriod() < 10) { - return false; - } - return true; + return config.getPollingPeriod() >= 10; ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + if (!getThing().getStatus().equals(ThingStatus.ONLINE)) { + updateStatus(ThingStatus.ONLINE); + } Not 100% sure, but if the binding is allready online, the framework already does a similar check. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + updateState(MfFanBindingConstants.CHANNEL_WIND_ON, OnOffType.from(dto.getWind().booleanValue())); + updateState(MfFanBindingConstants.CHANNEL_WIND_LEVEL, + StringType.valueOf(String.valueOf(dto.getWindSpeed()))); + updateState(MfFanBindingConstants.CHANNEL_LIGHT_ON, + OnOffType.from(dto.getLightOn().booleanValue())); + updateState(MfFanBindingConstants.CHANNEL_LIGHT_INTENSITY, + new PercentType(dto.getLightBrightness())); + + if (!getThing().getStatus().equals(ThingStatus.ONLINE)) { + updateStatus(ThingStatus.ONLINE); + } + } else { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR, + "Null shadow buffer returned."); + } + } catch (Exception e) { Where do you expect an expcetion to be thrown? I can't see a api call in this block, so a communcation exception is also not expected. And what other issue could be happening? — Reply to this email directly, view it on GitHub <#16786 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI3WWNT42TFFAXA2T34RTHTZD6DKDAVCNFSM6AAAAABIDWOQMKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZXGU2TINBVGI> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[lsiepel]

Making some progress! But the review summary is the same:
Some, but not all previous comments are addressed. I also see some comments being lost after the changes. Ping me when your are ready.

Also:

• fix the build (see bottom of this page)
• fix dco, some new commits are not signed off
• add yourself to the codeowners file
• add this binding to the bom/openhab-addons/pom.xml

I think i might need one last (small) round after all these are fixed.

[mark-brooks-180]

Isiepel, I believe I have made the recommended changes. Furthermore, I eliminated the use of the single thread executor from the MfFanHandler class. Your email had a bullet item that said - fix the build (see bottom of this page) I didn't find anything at the bottom of the page indicating the build error you are experiencing. Thanks, Mark B.

…

On Sun, May 26, 2024 at 4:35 PM lsiepel ***@***.***> wrote: ***@***.**** requested changes on this pull request. Making some progress! But the review summary is the same: Some, but not all previous comments are addressed. I also see some comments being lost after the changes. Ping me when your are ready. Also: - fix the build (see bottom of this page) - fix dco, some new commits are not signed off - add yourself to the codeowners file - add this binding to the bom/openhab-addons/pom.xml I think i might need one last (small) round after all these are fixed. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > @@ -72,22 +73,15 @@ public void initialize() { this.logger.debug("Initializing MfFan handler '{}'", getThing().getUID()); updateStatus(ThingStatus.UNKNOWN); this.config = getConfigAs(MfFanConfiguration.class); - if (validateConfig(this.config)) { - try { - this.api = new FanRestApi(this.config.getIpAddress().trim(), this.httpClientFactory); - // handleCommand(RefreshType.REFRESH); - } catch (Exception e) { - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR, e.getMessage()); - } - + if (MfFanConfiguration.validateConfig(this.config)) { + this.api = new FanRestApi(this.config.getIpAddress().trim(), this.httpClientFactory); Minor: The trim() should be part of the getIpAddress() as that method is responsible for returning a proper IpAddress ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + private ExecutorService executor; + + private HttpClientFactory httpClientFactory; + + public MfFanHandler(Thing thing, HttpClientFactory httpClientFactory) { + super(thing); + this.httpClientFactory = httpClientFactory; + this.executor = Executors.newSingleThreadExecutor(); + } + + @OverRide + public void initialize() { + this.logger.debug("Initializing MfFan handler '{}'", getThing().getUID()); + updateStatus(ThingStatus.UNKNOWN); + this.config = getConfigAs(MfFanConfiguration.class); + if (MfFanConfiguration.validateConfig(this.config)) { ⬇️ Suggested change - if (MfFanConfiguration.validateConfig(this.config)) { + if (!MfFanConfiguration.validateConfig(this.config)) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "Invalid configuration detected."); + return; + } Better to abort immediate. (last 3 lines of this method can be removed. You could also consider to use a custom i18n key. Reference: https://www.openhab.org/docs/developer/utils/i18n.html#using-custom-keys (last example) ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + ExecutorService exec = this.executor; + if (exec != null) { + exec.shutdown(); + } + this.executor = null; Why is this pattern not the same is with the pollingjob? Reference: #16786 (comment) <#16786 (comment)> ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/MfFanConfiguration.java <#16786 (comment)> : > + } + + public void setIpAddress(String ipAddress) { + this.ipAddress = ipAddress; + } + + public Integer getPollingPeriod() { + return this.pollingPeriod; + } + + public void setPollingPeriod(Integer pollingPeriod) { + this.pollingPeriod = pollingPeriod; + } + + public static boolean ***@***.*** MfFanConfiguration config) { + if ((config == null) || config.getIpAddress().isBlank()) { ⬇️ Suggested change - if ((config == null) || config.getIpAddress().isBlank()) { + if (config == null || config.getIpAddress().isBlank()) { ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + this.executor.execute(new CommandHandlerRunnable(channelUID, command)); + } catch ***@***.***("unused") Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, Not commented on or resolved yet. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + @SuppressWarnings("null") + @OverRide + public void handleCommand(ChannelUID channelUID, Command command) { + try { + this.executor.execute(new CommandHandlerRunnable(channelUID, command)); + } catch ***@***.***("unused") Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + String.format("Could not control device at IP address %s", this.config.getIpAddress())); + } + } + + private void handleCommand(Command command) { + handleCommand(new ChannelUID("Null:Null:Null:Null"), command); + } + + private final class CommandHandlerRunnable implements Runnable { Not commented on or resolved yet. ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > @@ -0,0 +1,69 @@ +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + Not fixed yet ------------------------------ In bundles/org.openhab.binding.mffan/README.md <#16786 (comment)> : > +# MfFan Binding + +This binding is used to enable communications between openHAB and "Modern Forms" or "WAC Lighting" WIFI connected, smart, ceiling fans. + + +## Supported Things + +Supported things include: "Modern Forms" and "WAC Lighting" smart ceiling fans. The binding has been tested with both. + +## Discovery + +Auto discovery is not supported at this time. +To bind with a smart fan either the IP address of the fan, or the local host name of the fan must be provided. +The binding has been tested using an IPV4 local network address, and a host name served by a local DNS. + +## Binding Configuration Not fixed yet ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + this.config = getConfigAs(MfFanConfiguration.class); + if (validateConfig(this.config)) { + try { + this.api = new FanRestApi(this.config.getIpAddress().trim(), this.httpClientFactory); + // handleCommand(RefreshType.REFRESH); + } catch (Exception e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR, e.getMessage()); + } + + this.pollingJob = this.scheduler.scheduleWithFixedDelay(() -> handleCommand(RefreshType.REFRESH), 0, + this.config.getPollingPeriod(), TimeUnit.SECONDS); + + this.logger.debug("Polling job scheduled to run every {} sec. for '{}'", this.config.getPollingPeriod(), + getThing().getUID()); + } else { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "Invalid configuration detected."); Was not fixed, due to changing this method, i created a new comment. ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + + this.logger.debug("Polling job scheduled to run every {} sec. for '{}'", this.config.getPollingPeriod(), + getThing().getUID()); + } else { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "Invalid configuration detected."); + } + } + + @OverRide + public void dispose() { + this.logger.debug("Disposing MF fan handler '{}'", getThing().getUID()); + + ScheduledFuture<?> job = this.pollingJob; + if (job != null) { + job.cancel(true); + } this is not fixed yet. — Reply to this email directly, view it on GitHub <#16786 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI3WWNTP5PKO3W6LHZM5IRLZEJBQ5AVCNFSM6AAAAABIDWOQMKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZZG43TQOBWHE> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[lsiepel]

Many comments are fixed, thanks. Left one new comment and besides that

• DCO is not signed (mandatory)
• build failed (run spotless)
• Unalbe to check now, but fix any SAT errors
• Unable to check now but fix any compile warnings

[mark-brooks-180]

Isiepel, I've pushed the most recent set of changes. Let me know if there is anything else I need to do. Thanks, Mark B.

…

On Mon, May 27, 2024 at 4:16 PM lsiepel ***@***.***> wrote: ***@***.**** requested changes on this pull request. Many comments are fixed, thanks. Left one new comment and besides that - DCO is not signed (mandatory) - build failed (run spotless) - Unalbe to check now, but fix any SAT errors - Unable to check now but fix any compile warnings ------------------------------ In bundles/org.openhab.binding.mffan/src/main/java/org/openhab/binding/mffan/internal/handler/MfFanHandler.java <#16786 (comment)> : > + } + this.pollingJob = null; ⬇️ Suggested change - } - this.pollingJob = null; + this.pollingJob = null; + } — Reply to this email directly, view it on GitHub <#16786 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI3WWNSVQVL7DH5E6VJXSCLZEOIB5AVCNFSM6AAAAABIDWOQMKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOBRGQZDINJYGA> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[lsiepel]

You could fix the errors below. (Also listed in my previous comment)

[mark-brooks-180]

Fixed.

…

On Tue, May 28, 2024 at 5:31 PM lsiepel ***@***.***> wrote: You could fix the errors below. (Also listed in my previous comment) — Reply to this email directly, view it on GitHub <#16786 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI3WWNUC55FMG5LDGP3HHM3ZETZT5AVCNFSM6AAAAABIDWOQMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZWGE2DIMZTG4> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[lsiepel]

Thanks, LGTM.

Only remaining issue is the DCO, if you click details next to the DCO you can see the related commits and how to fix those.

[lsiepel]

Thanks, LGTM.

Only remaining issue is the DCO, if you click details next to the DCO you can see the related commits and how to fix those.

@mark-brooks-180 you need any help with the DCO?

[mark-brooks-180]

Isiepel, I believe the DCO issue is resolved. All the commits are signed off. M

…

On Fri, Jun 14, 2024 at 3:38 PM lsiepel ***@***.***> wrote: Thanks, LGTM. Only remaining issue is the DCO, if you click details next to the DCO you can see the related commits and how to fix those. @mark-brooks-180 <https://github.com/mark-brooks-180> you need any help with the DCO? — Reply to this email directly, view it on GitHub <#16786 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI3WWNQ2XX6GX7AYFYEERQTZHNBE5AVCNFSM6AAAAABIDWOQMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRYGY2DOOBTGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[lsiepel]

Now, you could add your binding's logo to the openHAB website. See https://next.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website-and-the-ui

[mark-brooks-180]

Isiepel, Thanks for your help. M

…

On Sat, Jun 15, 2024 at 12:41 PM lsiepel ***@***.***> wrote: Now, you could add your binding's logo to the openHAB website. See https://next.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website-and-the-ui — Reply to this email directly, view it on GitHub <#16786 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI3WWNWKTCJOYU4PZBWJPVTZHRVB7AVCNFSM6AAAAABIDWOQMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZQGI3TSOJRGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>