[airvisualnode] Initial contribution for AirVisual Node Binding #2805

3cky · 2017-10-25T13:37:39Z

This binding integrates AirVisual Node Air Quality Monitor with openHAB.

For complete information about this binding, please check Readme.md.

Signed-off-by: Victor Antonovich v.antonovich@gmail.com

Hilbrand · 2017-10-25T20:56:30Z

addons/binding/org.openhab.binding.airvisualnode/ESH-INF/thing/thing-types.xml

+				<description>Node network password</description>
+			</parameter>
+			<!-- Advanced parameters -->
+            <parameter name="share" type="text">


Indentation went wrong. Spaces instead of tabs used?

Hilbrand · 2017-10-25T20:57:34Z

addons/binding/org.openhab.binding.airvisualnode/README.md

+| Channel ID                   | Item Type | Description                 |
+|------------------------------|-----------|-----------------------------|
+| measurements#co2_ppm         | Number    | CO₂ level, ppm               |
+| measurements#humidity        | Number    | Humidity, %                 |


Indentation of last bar | (also same with last entries of this table).

Hilbrand · 2017-10-25T21:00:14Z

addons/binding/org.openhab.binding.airvisualnode/about.html

+<p>March 30, 2017</p>
+<h3>License</h3>
+
+<p>


I thing you need to add a Third Party Content section here regarding the license of the lib cifs in this file.

Hilbrand · 2017-10-25T21:01:33Z

...visualnode/src/main/java/org/openhab/binding/airvisualnode/handler/AirVisualNodeHandler.java

+
+        schedulePoll();
+
+        updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.NONE, "Node poll scheduled");


This will always hide the 2 previous configuration errors.

since you added the return statements above you will always end up with an offline thing which in reality is still in INITIALIZING state, I would suggest to remove this line and set the thing to ONLINE or OFFLINE inside your polling job.

Hilbrand · 2017-10-25T21:01:53Z

...visualnode/src/main/java/org/openhab/binding/airvisualnode/handler/AirVisualNodeHandler.java

+
+        this.refreshInterval = config.refresh * 1000L;
+
+        schedulePoll();


Do you want to start the polling when there is a configuration error?

Hilbrand · 2017-10-25T21:07:18Z

...visualnode/src/main/java/org/openhab/binding/airvisualnode/handler/AirVisualNodeHandler.java

+        String url = "smb://" + nodeAddress +"/" + nodeShareName + "/" + NODE_JSON_FILE;
+        NtlmPasswordAuthentication auth = new NtlmPasswordAuthentication(null, nodeUsername, nodePassword);
+        try (SmbFileInputStream in = new SmbFileInputStream(new SmbFile(url, auth))) {
+            return IOUtils.toString(new SmbFileInputStream(new SmbFile(url, auth)), StandardCharsets.UTF_8.name());


You probably ment to use the variable in here that you moved to the line above.

Hilbrand · 2017-10-25T21:11:38Z

...java/org/openhab/binding/airvisualnode/internal/discovery/AirVisualNodeDiscoveryService.java

+    @Override
+    protected void startBackgroundDiscovery() {
+        logger.debug("Starting background discovery");
+        cancelBackgroundDiscoveryFuture();


I assume stop is always called before start so there is probably no need to call this cancel?

Hilbrand · 2017-10-25T21:18:19Z

...java/org/openhab/binding/airvisualnode/internal/discovery/AirVisualNodeDiscoveryService.java

+    @Override
+    protected void startScan() {
+        logger.debug("Starting scan");
+        scheduler.execute(scannerRunnable);


Maybe rewrite this as: scheduler.execute(this::createScannerRunnable()); and remove the return part from createScannerRunnable()? (Although then the method name probably should be something more related to the running itself instead of creating the runnable`)

Hilbrand · 2017-10-25T21:21:42Z

...java/org/openhab/binding/airvisualnode/internal/discovery/AirVisualNodeDiscoveryService.java

+                String workgroupUrl = "smb://" + AVISUAL_WORKGROUP_NAME +"/";
+                workgroupMembers = new SmbFile(workgroupUrl).listFiles();
+            } catch (Exception e) {
+                // Can't get workgroup member list


Are you sure it's not needed to log here? These exceptions will go undetected and might hide bugs.

Hilbrand · 2017-10-25T21:25:39Z

addons/binding/org.openhab.binding.airvisualnode/README.md

+| status#used_memory           | Number    | Used memory                 |
+| status#timestamp             | DateTime  | Timestamp                   |
+
+## Example


This can also use an example of a Thing with parameter configuration.

3cky · 2017-10-26T07:52:18Z

Thank you very much @Hilbrand for prompt PR reviewing, I'll fix that.

triller-telekom

Thank you very much for your contribution. And sorry that it took so long until you got another (more-detailed) review.

The overall code looks pretty good, except for that you should get rid of the channel groups.

Please have a look at my comments in the code.

If you have any questions, feel free to ask!

triller-telekom · 2018-04-27T10:47:50Z

addons/binding/org.openhab.binding.airvisualnode/README.md

+## Discovery
+
+Binding will do autodiscovery for AirVisual Node by searching for a host advertised with the NetBIOS name 'AVISUAL-<SerialNumber>'.
+All discovered devices will be added to the inbox. Please note you will need to set the Node username and password in the configuration


Please add a line break after each sentence, this makes merging a lot easier afterwards.

triller-telekom · 2018-04-27T10:50:33Z

addons/binding/org.openhab.binding.airvisualnode/README.md

+
+## Channels
+
+The binding support the following channels:


triller-telekom · 2018-04-27T10:52:44Z

addons/binding/org.openhab.binding.airvisualnode/README.md

+| measurements#aqi_us          | Number    | Air Quality Index (US)      |
+| measurements#pm_25           | Number    | PM2.5 level, μg/m³          |
+| measurements#temp_celsius    | Number    | Temperature, Celsius        |
+| measurements#temp_fahrenheit | Number    | Temperature, Fahrenheit     |


Since a few weeks we have a great new feature called units of measurement which would be a great fit for your binding. You can specify your channels with the correct unit and transformations will be done automatically by the framework. Please have a look at it.

triller-telekom · 2018-04-27T10:55:08Z

addons/binding/org.openhab.binding.airvisualnode/README.md

+
+| Channel ID                   | Item Type | Description                 |
+|------------------------------|-----------|-----------------------------|
+| status#battery_level         | Number    | Battery level, %            |


We do have a system channel available for this, please make use of it. System channels are for harmonizing the way multiple bindings provide their information. See https://www.eclipse.org/smarthome/documentation/development/bindings/thing-definition.html#channels

triller-telekom · 2018-04-27T10:55:13Z

addons/binding/org.openhab.binding.airvisualnode/README.md

+| Channel ID                   | Item Type | Description                 |
+|------------------------------|-----------|-----------------------------|
+| status#battery_level         | Number    | Battery level, %            |
+| status#wifi_strength         | Number    | Wi-Fi signal strength       |


We do have a system channel available for this, please make use of it. System channels are for harmonizing the way multiple bindings provide their information. See https://www.eclipse.org/smarthome/documentation/development/bindings/thing-definition.html#channels

triller-telekom · 2018-04-27T11:20:13Z

addons/binding/org.openhab.binding.airvisualnode/ESH-INF/thing/thing-types.xml

+		<label>Status</label>
+		<channels>
+			<channel id="battery_level" typeId="system.battery-level" />
+			<channel id="wifi_strength" typeId="system.signal-strength" />


See my comment below, use a system channel for this.

Hmm, could you clarify please, do you mean I should change these channel ids? Because both channels are already referenced to a system channel typeIds. Should I just rename channel ids to battery-level and signal-strength, or some more changes are required?

Upps, sorry apparently I stopped reading after <channel id="battery_level"... Sorry my fault.

triller-telekom · 2018-04-27T11:25:27Z

...visualnode/src/main/java/org/openhab/binding/airvisualnode/handler/AirVisualNodeHandler.java

+        Object value = null;
+
+        if (nodeData != null) {
+            String[] fullChannelIdFields = StringUtils.split(fullChannelId, "#");


I do not understand why you have to split the channelID by #. You should pass the correct ID to this method instead, splitting at # is not reliable.

triller-telekom · 2018-04-27T11:29:53Z

...java/org/openhab/binding/airvisualnode/internal/discovery/AirVisualNodeDiscoveryService.java

+    @Override
+    protected void startBackgroundDiscovery() {
+        logger.debug("Starting background discovery");
+        backgroundDiscoveryFuture = scheduler.scheduleWithFixedDelay(this::scan, 0, 300, TimeUnit.SECONDS);


Are you sure that the library you are using is doing the scan in the same way as windows clients do it? Because if you do a periodic scan every 5 minutes of the whole network your machine could be subject to be blocked by a net intrusion system in certain network setups. So I am unsure whether we should keep the automatic scan without user interaction in here.

Well, the discovery process just requests SMB servers list from the local master browser using JCIFS library. Not sure how this operation will be treated by a net intrusion systems if repeated though. Do you recommend to disable periodic autodiscovery here?

I am no 100% sure. I just have in mind that we do not want discovery processes to "flood" the network without a user-interaction. A background scan works without a user interaction. If you would move it to startScan the user has to initiate the scan. But in your scenario is a corner case I guess and I am not a windows expert. Querying the master browser for the list of servers every 5min doesn't sound too bad though. Let's leave it in for now, I just wanted to understand what is actually happening here.

triller-telekom · 2018-04-27T11:30:38Z

...java/org/openhab/binding/airvisualnode/internal/discovery/AirVisualNodeDiscoveryService.java

+        try {
+            String workgroupUrl = "smb://" + AVISUAL_WORKGROUP_NAME +"/";
+            workgroupMembers = new SmbFile(workgroupUrl).listFiles();
+        } catch (Exception e) {


See above, please do not catch Exception

triller-telekom · 2018-04-27T11:34:30Z

...java/org/openhab/binding/airvisualnode/internal/discovery/AirVisualNodeDiscoveryService.java

+                // Create discovery result
+                String nodeAddress = nodeNbtAddress.getInetAddress().getHostAddress();
+                DiscoveryResult result = DiscoveryResultBuilder.create(thingUID)
+                        .withProperty(AirVisualNodeConfig.ADDRESS, nodeAddress)


Since this is also unique for all devices, you should make it a representationProperty, see https://www.eclipse.org/smarthome/documentation/development/bindings/discovery-services.html

3cky · 2018-04-28T09:53:57Z

Hello @triller-telekom, thank you for reviewing this PR!

Got some questions about applying UOM to the Air quality index (AQI). AQI US and AQI CN values are both dimensionless and differs only by method used for their computation (please see corresponding Wikipedia article if you're interested). Is the UOM still applicable in this case?

htreu · 2018-05-02T13:31:09Z

Hi @3cky, regarding AQI I suggest not to use UOM in this case. AQI is a scale which maps to a combination of quantitative measurements. Its is itself not a quantitative property. You could use ScaleTransformationService to convert from one index to another.

Edit: Link to scale transformation

3cky · 2018-05-10T10:06:16Z

Hello @triller-telekom @htreu,

Thanks for your assistance! I've pushed updated PR, now UoM are used by all Node measurement channels. Not quite sure about the PM2.5 Number:Density channel though, I've used custom μg/m³ measurement unit https://github.com/openhab/openhab2-addons/pull/2805/files#diff-44b0bf6e12ae2c757dc0f1a4f2a50e0aR201 and it seems to work well in Items and Sitemaps, but not in Rules:

avnode.rules:

...
if (Livingroom_Pm25_Level.state > 25.0 [μg/m³]) {
    logInfo("avnode.rules", "PM2.5 level is above 25 μg/m³")
}

openhab.log:

2018-05-07 17:38:22.176 [WARN ] [el.core.internal.ModelRepositoryImpl] - Configuration model 'avnode.rules' has errors, therefore ignoring it: [40,42]: missing ')' at '['
[40,43]: no viable alternative at input 'μ'
[40,47]: no viable alternative at input '³'

Is this expected behaviour?

htreu · 2018-05-14T11:16:25Z

@3cky unfortunately right now this is expected. The unit symbols µ and ³ are currently not supported by the script engine. Its missing in the DSL description. I will report back with a corresponding pull request in Eclipse SmartHome to fix this.

Addresses https://github.com/openhab/openhab2-addons/pull/2805#issuecomment-388782058. Signed-off-by: Henning Treu <henning.treu@telekom.de>

htreu · 2018-05-14T12:27:58Z

@3cky once eclipse-archived/smarthome#5574 is merged and available for openHAB your scripts should work as expected.

3cky · 2018-05-14T12:48:41Z

Hi @htreu,

Great! I'll wait for it, thank you!

Addresses https://github.com/openhab/openhab2-addons/pull/2805#issuecomment-388782058. Signed-off-by: Henning Treu <henning.treu@telekom.de>

htreu · 2018-05-15T11:44:46Z

@3cky the corresponding PR already got merged and a new Eclipse SmartHome stable build also is available: DISCLAIMER: it introduced a new bug when triggering rules, so while µ and ^2/^3 are available it might nonetheless break your rule.

3cky · 2018-05-22T15:27:08Z

Hello @htreu, probably I did miss something but I still expiriencing the very same error ("no viable alternative at input" etc) in OpenHAB 2.3.0-SNAPSHOT build 1284.

htreu · 2018-06-11T14:40:44Z

Please note that unit support in scripts had to be refactored just recently. It now uses | instead of [] to add the unit. Like 20|°C. Please try again and report back. Thanks.

3cky · 2018-06-13T15:51:33Z

@htreu

I've tried this rule in openHAB 2.4.0-SNAPSHOT build 1299:

rule "AirVisual Node PM2.5 Level Rule"
when
    Item Livingroom_Pm25_Level changed
then
    if (Livingroom_Pm25_Level.state > 25.0|μg/m³) {
        logInfo("avnode.rules", "PM2.5 level is above 25 μg/m³")
    }
end

and I'm still getting the error:

2018-06-13 18:34:28.232 [WARN ] [el.core.internal.ModelRepositoryImpl] - Configuration model 'avnode.rules' has errors, therefore ignoring it: [40,44]: no viable alternative at input 'μ'
[40,45]: missing ')' at 'g'
[40,47]: no viable alternative at input 'm³'

3cky · 2018-06-14T13:44:15Z

Update: I've changed 25.0|μg/m³ to 25.0|"μg/m³" according to this documentation page https://www.openhab.org/docs/configuration/rules-dsl.html#number-item. Now 'no viable alternative at input' error is gone, but when Livingroom_Pm25_Level item value is changed I'm getting the next error:

2018-06-14 15:29:05.746 [ERROR] [ntime.internal.engine.RuleEngineImpl] - Rule 'AirVisual Node PM2.5 Level Rule': An error occurred during the script execution: μg not recognized (in 25.0 μg/m³ at index 5)

htreu · 2018-06-14T14:45:47Z

I can for now confirm this issue. Testing with 1|"µm" works fine, while 1|"µg" produces the same error. Stay tuned.

htreu · 2018-06-15T13:45:31Z

Here are my findings about this:

The ESH internal tec.uom library does define a lot of µ prefixed units, also µg.
There are two different characters µ:
- The greek mu, unicode \u03bc
- The micro sign, unicode \u00b5
Only the micro sign is a valid character in terms of UoM.

Please try a script like 25.0|"\u00b5g" which uses the afore mentioned micro sign. It worked out in my unit tests.

I'm not sure why and when a keyboard input produces which character. On my mac OS the combination alt+m always seems to produce the micro sign.

kaikreuzer · 2018-06-15T13:55:32Z

Wow, not that is really nasty! @keilw, are you aware of this? What's the suggested UoM way to deal with it?

kaikreuzer · 2018-06-15T13:56:45Z

@htreu Any explanation why you found 1|"µm" working, though? What makes the difference?

keilw · 2018-06-15T13:59:56Z

Have to check, but I know when I created JUnit tests for the temperature question, it occurred to me again, there are 2 ways to render °C, one with 2 separate characters, while there is an Unicode char that combines both ;-O

kaikreuzer · 2018-06-15T14:04:47Z

We know, we just got rid off that "feature"... eclipse-archived/smarthome#5669

htreu · 2018-06-15T14:22:00Z

When I author the unit test with my setup everything works as expected. After @3cky's last report I did copy his script from the comment and reproduced the error. All µ characters I type on my keyboard seem to be valid micro signs.
If you look closely in my comment (https://github.com/openhab/openhab2-addons/pull/2805#issuecomment-397321886) you will see a difference in rendering of both µ chars.

keilw · 2018-06-15T14:22:09Z

There are other formatters/parsers, especially in the UCUM bundle, but that would have to be used via CQ.

htreu · 2018-06-15T14:25:36Z

As for the visual differences: I can only see it on the external HD screen. On the MacBook retina screen it makes zero to none difference.

triller-telekom

Thank you for addressing the review commits.

I had a look at them and except for a few minor issues they look good to me. Please have a look at my comments in the code.

triller-telekom · 2018-06-29T09:18:39Z

addons/binding/org.openhab.binding.airvisualnode/ESH-INF/thing/thing-types.xml

 		<description>Air Quality Index (US)</description>
+		<category>Flow</category>


I am not sure if this is the right category. What is flowing here? Better leave it out.

triller-telekom · 2018-06-29T09:18:44Z

addons/binding/org.openhab.binding.airvisualnode/ESH-INF/thing/thing-types.xml

 		<label>PM2.5</label>
 		<description>PM2.5 level, μg/m&#179;</description>
-		<state readOnly="true" pattern="%.1f μg/m&#179;" />
+		<category>Flow</category>


I am not sure if this is the right category. What is flowing here? Better leave it out.

triller-telekom · 2018-06-29T09:19:55Z

addons/binding/org.openhab.binding.airvisualnode/META-INF/MANIFEST.MF

+ org.eclipse.smarthome.core.thing.binding.builder,
+ org.eclipse.smarthome.core.thing.type,
+ org.eclipse.smarthome.core.types,
+ org.osgi.service.component.annotations;resolution:=optional,


Please incorporate my comment and remove this line. It is not needed.

triller-telekom · 2018-06-29T09:23:01Z

addons/binding/org.openhab.binding.airvisualnode/README.md

+Number:Dimensionless Livingroom_Humidity "Humidity [%d %unit%]" <humidity> {channel="airvisualnode:avnode:1a2b3c4:humidity"}
+Number:Dimensionless Livingroom_CO2_Level "CO₂" <flow> {channel="airvisualnode:avnode:1a2b3c4:co2"}
+Number:Dimensionless Livingroom_Aqi_Level "Air Quality Index" <flow> { channel="airvisualnode:avnode:1a2b3c4:aqi" }
+Number:Density Livingroom_Pm25_Level  "PM2.5 Level" <line> { channel="airvisualnode:avnode:1a2b3c4:pm_25" }


Where do you have the category "line" from? I cannot find it in our documentation: https://www.eclipse.org/smarthome/documentation/concepts/categories.html

triller-telekom · 2018-06-29T09:23:11Z

addons/binding/org.openhab.binding.airvisualnode/README.md

+Number:Dimensionless Livingroom_CO2_Level "CO₂" <flow> {channel="airvisualnode:avnode:1a2b3c4:co2"}
+Number:Dimensionless Livingroom_Aqi_Level "Air Quality Index" <flow> { channel="airvisualnode:avnode:1a2b3c4:aqi" }
+Number:Density Livingroom_Pm25_Level  "PM2.5 Level" <line> { channel="airvisualnode:avnode:1a2b3c4:pm_25" }
+DateTime Livingroom_Aqi_Timestamp "AQI Timestamp [%1$tH:%1$tM]" <clock> { channel="airvisualnode:avnode:1a2b3c4:timestamp" }


Where do you have the category "clock" from? I cannot find it in our documentation: https://www.eclipse.org/smarthome/documentation/concepts/categories.html

triller-telekom · 2018-06-29T09:31:02Z

...isualnode/src/main/java/org/openhab/binding/airvisualnode/AirVisualNodeBindingConstants.java

    public static final String CHANNEL_BATTERY_LEVEL = "battery_level";
-    public static final String CHANNEL_WIFI_STRENGTH = "wifi_strength";
+    public static final String CHANNEL_WIFI_STRENGTH = "signal_strength";


Since you now use the system channels, its probably better to use their definitions directly, i.e.: DefaultSystemChannelTypeProvider.SYSTEM_SYSTEM_CHANNEL_SIGNAL_STRENGTH.getUID().getId()

Same applies to other system channels of course.

3cky · 2018-07-24T07:52:24Z

Hello @triller-telekom,

Thank you for reviewing this PR and please sorry for my late response. I removed problematic item categories, cleaned up MANIFEST.MF from all non-essential import packages and did changes in system channels handling code. Please let me know if there are any other fixes needed.

triller-telekom · 2018-07-24T09:25:36Z

Thank you for the changes. Looks good, except that you missed on of my comments: https://github.com/openhab/openhab2-addons/pull/2805#discussion_r199102470

3cky · 2018-07-24T09:52:48Z

@triller-telekom do you mean this line? org.osgi.service.component.annotations;resolution:=optional,
When I remove it, Eclipse reports the error below:

Is this normal or I did miss something?

kaikreuzer · 2018-07-24T10:05:04Z

@3cky Are you using the official openHAB IDE? If not, you will need to enable the DS annotation generator, I just commented this on eclipse-archived/smarthome#5954 (comment).

3cky · 2018-07-24T11:00:48Z

@kaikreuzer you're right, I'm using not openHAB IDE but regular Eclipse IDE installation. I followed your instructions and error is gone now. Thanks! So I'll remove annotations package import from the manifest file.

triller-telekom

Thank you very much for the binding and all the incorporated changes.

I leave it to @kaikreuzer and @martinvw to have a look and merge the binding.

martinvw

It looks great indeed, I added some minor comments, I will also queue you up at @kaikreuzer

martinvw · 2018-07-24T19:10:52Z

addons/binding/org.openhab.binding.airvisualnode/.project

+		</buildCommand>
+	</buildSpec>
+	<natures>
+		<nature>org.eclipse.m2e.core.maven2Nature</nature>


This should not be here?

martinvw · 2018-07-24T19:13:50Z

...isualnode/src/main/java/org/openhab/binding/airvisualnode/AirVisualNodeBindingConstants.java

+            .getUID().getId();
+
+    // List of all supported Thing UIDs
+    public static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Collections


Afair, Set's should be annotated to have a nullable generic, because otherwise the compiler might assume that a set cannot return null but it can.

@maggu2810 or am I mistaken now?

@martinvw You are right, but where is that @Nullable annotation that you are referring to?

Missing and the class is annotated to be not Null

Ah now I understand my misunderstanding of your comment...

Sets should not be annotated to have a @Nullable generic, because that would express that we allow to add null to the collection.

The problem you are mentioning is that a get(item) on a Set might always return null if the requested item is not in the Set. But that should be covered by external annotations which annotate (in this case) the JDT. But we do not have them, both PRs eclipse-archived/smarthome#4513 and eclipse-archived/smarthome#4217 were closed.

Long story short: The line here is correct.

Nothing to add to @triller-telekom's explanation. 😉

Great thanks guys!

martinvw · 2018-07-24T19:14:10Z

...visualnode/src/main/java/org/openhab/binding/airvisualnode/handler/AirVisualNodeHandler.java

+
+    public AirVisualNodeHandler(Thing thing) {
+        super(thing);
+        gson = new GsonBuilder().setFieldNamingPolicy(FieldNamingPolicy.LOWER_CASE_WITH_UNDERSCORES).create();


martinvw · 2018-07-24T19:20:03Z

...visualnode/src/main/java/org/openhab/binding/airvisualnode/handler/AirVisualNodeHandler.java

+    }
+
+    private String getNodeJsonData() throws IOException {
+        String url = "smb://" + nodeAddress +"/" + nodeShareName + "/" + NODE_JSON_FILE;


Very small formatting error, if in doubt format all your (Java) files

martinvw · 2018-07-24T19:20:56Z

...visualnode/src/main/java/org/openhab/binding/airvisualnode/handler/AirVisualNodeHandler.java

+                case CHANNEL_TIMESTAMP:
+                    // It seem the Node timestamp is Unix timestamp converted from UTC time plus timezone offset.
+                    // Not sure about DST though, but it's best guess at now
+                    Calendar cal = Calendar.getInstance(TimeZone.getTimeZone("UTC"));


We have more modern options since Java 8, I would prefer if you use them instead of the calendar

martinvw · 2018-07-24T19:22:25Z

...java/org/openhab/binding/airvisualnode/internal/discovery/AirVisualNodeDiscoveryService.java

+    @Override
+    protected void startBackgroundDiscovery() {
+        logger.debug("Starting background discovery");
+        backgroundDiscoveryFuture = scheduler.scheduleWithFixedDelay(this::scan, 0, 300, TimeUnit.SECONDS);


Why not specify the much more explicit

..., 0, 5, TimeUnit.MINUTES);

3cky · 2018-07-26T09:48:27Z

Hello @martinvw, thank you for your comments! I did some fixes, please check them out.

kaikreuzer

Many thanks, this looks wonderful!
Just some very minor last comments before merging, see below.

kaikreuzer · 2018-07-26T14:14:38Z

addons/binding/org.openhab.binding.airvisualnode/ESH-INF/thing/thing-types.xml

+		<config-description>
+			<!-- Required parameters -->
+			<parameter name="address" type="text" required="true">
+				<context>network_address</context>


Should be network-address.

kaikreuzer · 2018-07-26T14:27:55Z

addons/binding/org.openhab.binding.airvisualnode/ESH-INF/thing/thing-types.xml

+		<state readOnly="true" />
+	</channel-type>
+
+	<channel-type id="Used_memory">


Sounds like a good candidate to mark as advanced=true.

kaikreuzer · 2018-07-26T14:28:32Z

addons/binding/org.openhab.binding.airvisualnode/README.md

+
+## Supported Things
+
+There is one supported Thing, the "AirVisual Node".


You should mention its ID here.

kaikreuzer · 2018-08-08T07:45:57Z

@3cky Please ping me once you have pushed an update - thanks!

Signed-off-by: Victor Antonovich <v.antonovich@gmail.com> (github: 3cky)

Signed-off-by: Victor Antonovich <v.antonovich@gmail.com>

3cky · 2018-08-10T10:59:48Z

Hello @kaikreuzer, thank you for your patience. I made fixes in accordance with your comments, please check.

kaikreuzer

Great, thanks a lot!

3cky · 2018-08-10T15:12:15Z

Great, thank you all guys for your assistance!

…hab#2805) Signed-off-by: Victor Antonovich <v.antonovich@gmail.com> (github: 3cky)

3cky force-pushed the airvisual-node-binding branch from d847482 to c74701e Compare October 25, 2017 15:05

Hilbrand reviewed Oct 25, 2017

View reviewed changes

martinvw added the new binding If someone has started to work on a binding. For a new binding PR. label Nov 10, 2017

3cky force-pushed the airvisual-node-binding branch from 4f1d2d8 to 949f0da Compare December 7, 2017 13:06

triller-telekom requested changes Apr 27, 2018

View reviewed changes

3cky force-pushed the airvisual-node-binding branch from 949f0da to 2ed89b2 Compare May 10, 2018 09:44

htreu referenced this pull request in htreu/smarthome May 14, 2018

Add unit symbols µ, ^2, ^3 and ** (exponent)

87b646c

Addresses https://github.com/openhab/openhab2-addons/pull/2805#issuecomment-388782058. Signed-off-by: Henning Treu <henning.treu@telekom.de>

htreu mentioned this pull request May 14, 2018

Add unit symbols µ, ^2, ^3 and ** (exponent) eclipse-archived/smarthome#5574

Merged

kaikreuzer referenced this pull request in eclipse-archived/smarthome May 14, 2018

Add unit symbols µ, ^2, ^3 and ** (exponent) (#5574)

f2fd2d7

Addresses https://github.com/openhab/openhab2-addons/pull/2805#issuecomment-388782058. Signed-off-by: Henning Treu <henning.treu@telekom.de>

triller-telekom reviewed Jun 29, 2018

View reviewed changes

3cky force-pushed the airvisual-node-binding branch from f0ba894 to 9840120 Compare July 24, 2018 07:33

triller-telekom approved these changes Jul 24, 2018

View reviewed changes

martinvw reviewed Jul 24, 2018

View reviewed changes

martinvw assigned kaikreuzer Jul 24, 2018

martinvw requested a review from kaikreuzer July 24, 2018 19:24

kaikreuzer requested changes Jul 27, 2018

View reviewed changes

3cky added 9 commits August 10, 2018 13:38

Add AirVisual Node air quality monitor binding

e7be6be

Signed-off-by: Victor Antonovich <v.antonovich@gmail.com> (github: 3cky)

[airvisualnode] Fix PR review problems

5c0c21a

Signed-off-by: Victor Antonovich <v.antonovich@gmail.com>

[airvisualnode] Implement UoM for device channels

42f4805

Signed-off-by: Victor Antonovich <v.antonovich@gmail.com>

[airvisualnode] Version updated to 2.4.0

318b7bf

Signed-off-by: Victor Antonovich <v.antonovich@gmail.com>

[airvisualnode] Update example rules according to the changes in UoM

2eb7a99

Signed-off-by: Victor Antonovich <v.antonovich@gmail.com>

[airvisualnode] Fix PR review issues

9bf7d50

Signed-off-by: Victor Antonovich <v.antonovich@gmail.com>

[airvisualnode] Remove unneeded package import from MANIFEST.MF

cfc6a18

Signed-off-by: Victor Antonovich <v.antonovich@gmail.com>

[airvisualnode] Use ZonedDateTime to handle timestamp. Minor fixes

8663f35

Signed-off-by: Victor Antonovich <v.antonovich@gmail.com>

[airvisualnode] Address PR review comments

d63e3eb

Signed-off-by: Victor Antonovich <v.antonovich@gmail.com>

3cky force-pushed the airvisual-node-binding branch from a88f7b5 to d63e3eb Compare August 10, 2018 10:43

kaikreuzer approved these changes Aug 10, 2018

View reviewed changes

kaikreuzer merged commit 1fc95e5 into openhab:master Aug 10, 2018

mkatter pushed a commit to mkatter/openhab2-addons that referenced this pull request Aug 25, 2018

[airvisualnode] Initial contribution for AirVisual Node Binding (open…

828367e

…hab#2805) Signed-off-by: Victor Antonovich <v.antonovich@gmail.com> (github: 3cky)

martinvw added this to the 2.4 milestone Dec 9, 2018


		schedulePoll();

		updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.NONE, "Node poll scheduled");


		this.refreshInterval = config.refresh * 1000L;

		schedulePoll();

		<description>Air Quality Index (US)</description>
		<category>Flow</category>


		## Supported Things

		There is one supported Thing, the "AirVisual Node".

[airvisualnode] Initial contribution for AirVisual Node Binding #2805

[airvisualnode] Initial contribution for AirVisual Node Binding #2805

Conversation

3cky commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3cky commented Oct 26, 2017

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3cky commented Apr 28, 2018

htreu commented May 2, 2018 • edited

3cky commented May 10, 2018

htreu commented May 14, 2018

htreu commented May 14, 2018

3cky commented May 14, 2018

htreu commented May 15, 2018

3cky commented May 22, 2018

htreu commented Jun 11, 2018

3cky commented Jun 13, 2018

3cky commented Jun 14, 2018

htreu commented Jun 14, 2018 • edited

htreu commented Jun 15, 2018

kaikreuzer commented Jun 15, 2018

kaikreuzer commented Jun 15, 2018

keilw commented Jun 15, 2018 • edited

kaikreuzer commented Jun 15, 2018

htreu commented Jun 15, 2018

keilw commented Jun 15, 2018

htreu commented Jun 15, 2018

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3cky commented Jul 24, 2018

triller-telekom commented Jul 24, 2018

3cky commented Jul 24, 2018

kaikreuzer commented Jul 24, 2018

3cky commented Jul 24, 2018

triller-telekom left a comment

Choose a reason for hiding this comment

martinvw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htreu commented May 2, 2018 •

edited

htreu commented Jun 14, 2018 •

edited

keilw commented Jun 15, 2018 •

edited