New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AirQuality binding #1738

Merged
merged 2 commits into from Feb 16, 2017

Conversation

Projects
None yet
8 participants
@kubawolanin
Contributor

kubawolanin commented Jan 20, 2017

So here's my attempt to utilize World Air Quality Index data in our home automation systems :)
It's my first binding, so I'm sure there are some things to improve.
i hope you'll like it!

I'm attaching compiled JAR file to this post. It will be updated accordingly.
Please make sure you change extension from *.zip to *.jar before you drop it to the \addons\ folder.

Updated JAR with latest (04.02.2017) code review follow-up:
org.openhab.binding.airquality-2.1.0-SNAPSHOT.zip

Big shoutout to @lolodomo whose Wunderground binding helped me a lot with understanding all the nuances in building one :) Thank you!

Signed-off-by: Kuba Wolanin hi@kubawolanin.com
Also-by: Łukasz Dywicki luke@code-house.org

@kubawolanin kubawolanin referenced this pull request Jan 20, 2017

Closed

Air Quality binding #2794

@hakan42

A few quick comments here and there 😄

Overall a nice binding, thank you for your efforts.

@@ -0,0 +1,5 @@
.classpath

This comment has been minimized.

@hakan42

hakan42 Jan 20, 2017

Contributor

Please remove binding-specific gitignore file from the PR. We should depend on the overall .gitignore file in the root of the project ( @kaikreuzer : I know we are using eclipse, but could we add the .idea and .*.iml for IntelliJ anyway?)

</state>
</channel-type>
</thing:thing-descriptions>

This comment has been minimized.

@hakan42

hakan42 Jan 20, 2017

Contributor

Could you add a line feed at the end of the file? Makes things nicer to read on the shell.

This comment has been minimized.

@kubawolanin

kubawolanin Jan 22, 2017

Contributor

Done! Thanks

airquality.things:
```
airquality:aqi:Krakow "AirQuality in Krakow" [ apikey="XXXXXXXXXXXX", location="50.06465,19.94498", refresh=60 ]

This comment has been minimized.

@hakan42

hakan42 Jan 20, 2017

Contributor

I just discovered that you can add a Location in the thing description 😄

Could you change the description to "AirQualitry" @ "Krakow" so we have examples demostrating this syntax?

```
sitemap demo label="Air Quality"
{

This comment has been minimized.

@hakan42

hakan42 Jan 20, 2017

Contributor

opening brace to the upper line please (so all files look similiar)

airquality.sitemap:
```
sitemap demo label="Air Quality"

This comment has been minimized.

@hakan42

hakan42 Jan 20, 2017

Contributor

Please call the sitemap "airquality" so it matches the filename. The Android app (or maybe Rotini or 3House?) was pretty picky about it.

This comment has been minimized.

@ThomDietrich

ThomDietrich Jan 20, 2017

Member

I believe this to be a general requirement. Please anyone correct me if that's not strictly correct.

This comment has been minimized.

@metbril

metbril Jan 21, 2017

Contributor

@ThomDietrich @hakan42 you are correct

* Iterates through the fields and returns the getter value.
*/
@SuppressWarnings("all")
private static Object getValue(Object data, String[] fields, int index) throws Exception {

This comment has been minimized.

@hakan42

hakan42 Jan 20, 2017

Contributor

Is reflection really necessary to fetch attributes from the json object? You might run into security manager problems...

*
* @author Kuba Wolanin - Initial contribution
*/
public class AirQualityConfiguration {

This comment has been minimized.

@hakan42

hakan42 Jan 20, 2017

Contributor

Use private members and public getter methods on the Configuration object please. This construct is slightly surprising for other Java programmers 😈

This comment has been minimized.

@kubawolanin

kubawolanin Jan 21, 2017

Contributor

You got it! :) Thank you

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 13, 2017

Member

No, the idea of the Configuration classes is actually to have public fields and no need for boilerplate methods. This is also done in other bindings that way.

This comment has been minimized.

@kubawolanin

kubawolanin Feb 16, 2017

Contributor

Reverted the chnge here so we have only public fields in Configuration class.

public String getAqiDescription() {
int aqi = data.getAqi();
if (aqi > 0 && aqi <= 50) {

This comment has been minimized.

@hakan42

hakan42 Jan 20, 2017

Contributor

Could you introduce String constants for those values?

public Calendar getS() throws Exception {
Calendar calendar = Calendar.getInstance();
SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");

This comment has been minimized.

@hakan42

hakan42 Jan 20, 2017

Contributor

Please create a static member. Creating the SimpleDateFormat every time might be costly.

@@ -71,4 +72,4 @@
<module>org.openhab.binding.zway</module>
</modules>
</project>
</project>

This comment has been minimized.

@hakan42

hakan42 Jan 20, 2017

Contributor

Please do not remove the empty line 😄

<parent>
<groupId>org.openhab.binding</groupId>
<artifactId>pom</artifactId>
<version>2.0.0-SNAPSHOT</version>

This comment has been minimized.

@hakan42

hakan42 Jan 20, 2017

Contributor

Oh, by the way, we are at 2.1.0-SNAPSHOT as of four hours ago:

af6fe5b

I guess I need to update my pull requests as well 😄

| Channel ID | Item Type | Description | |
|------------|--------------|------------------------- |---|
| aqiLevel | Number | Air Quality Index | |

This comment has been minimized.

@ThomDietrich

ThomDietrich Jan 21, 2017

Member

Is the last column reserved for something or a mistake?

This comment has been minimized.

@kubawolanin

kubawolanin Jan 21, 2017

Contributor

It was certainly a mistake :) Thanks Thomas!

super.initialize();
AirQualityConfiguration config = getConfigAs(AirQualityConfiguration.class);
logger.debug("config apikey = {}", config.apikey);

This comment has been minimized.

@ThomDietrich

ThomDietrich Jan 21, 2017

Member

Should this be omitted from log files?

@metbril

This comment has been minimized.

Contributor

metbril commented Jan 21, 2017

Thanks for your hard work.

These are some of my initial findings:

  • README: Example things file uses airquality:aqi:Krakow but example items use airquality:aqi:krk. This is inconsistent. I would use home for both anyway for the example.
  • README: You could move the .map example to the full example chapter.
  • README: The .map file is missing an entry for value -. This raises an error in the console when used in the UI. I added an additional line -=- to fix this.
  • README: The example things uses location="50.06465,19.94498" but the documentation says the 37.8;-122.4 syntax is allowed. This is confusing or even wrong?
  • README: The example .items could use specification of icons
  • README: You could prefix any item in the example with Aqi_ for better readability
  • README: It would be awesome if the text color of the AQI level and description matches the index. This could be achieved in a sitemap using visibility.

I would gladly help you get the docs right, but am not into git too much to know what is the best way to clone your work and add to the PR.

`AQI Description` item provides a human-readable output that can be interpreted e.g. by MAP transformation:
airquality.map

This comment has been minimized.

@metbril

metbril Jan 21, 2017

Contributor

You could move the .map example to the full example section

airquality.things:
```
airquality:aqi:Krakow "AirQuality in Krakow" [ apikey="XXXXXXXXXXXX", location="50.06465,19.94498", refresh=60 ]

This comment has been minimized.

@metbril

metbril Jan 21, 2017

Contributor

The thing uses 'Krakow' where the items use 'krk'

UNHEALTHY=Unhealthy
VERY_UNHEALTHY=Very unhealthy
HAZARDOUS=Hazardous
```

This comment has been minimized.

@metbril

metbril Jan 21, 2017

Contributor

The mapping -=- is missing. This raises an error when needed in a sitemap.

This comment has been minimized.

@kubawolanin

kubawolanin Jan 21, 2017

Contributor

Done. Thanks!

Number AqiLevel "Air Quality Index" (AirQuality) { channel="airquality:aqi:krk:aqiLevel" }
String AqiDescription "AQI Level [MAP(airquality.map):%s]" (AirQuality) { channel="airquality:aqi:krk:aqiDescription" }
Number Pm25 "PM25 Level" (AirQuality) { channel="airquality:aqi:krk:pm25" }

This comment has been minimized.

@metbril

metbril Jan 21, 2017

Contributor

These numbers could be enhanced with <line> icon.

airquality.sitemap:
```
sitemap demo label="Air Quality"

This comment has been minimized.

@metbril

metbril Jan 21, 2017

Contributor

@ThomDietrich @hakan42 you are correct

airquality.things:
```
airquality:aqi:Krakow "AirQuality in Krakow" [ apikey="XXXXXXXXXXXX", location="50.06465,19.94498", refresh=60 ]

This comment has been minimized.

@metbril

metbril Jan 21, 2017

Contributor

I would use airquality:aqi:home for easy copy and pasting by users.

```
airquality:aqi:Krakow "AirQuality in Krakow" [ apikey="XXXXXXXXXXXX", location="50.06465,19.94498", refresh=60 ]
```

This comment has been minimized.

@metbril

metbril Jan 21, 2017

Contributor

Suggesting to prepend all items with 'Aqi_'

@kubawolanin kubawolanin force-pushed the kubawolanin:feature/airquality branch from 83b315c to e0a865a Jan 21, 2017

@kubawolanin

This comment has been minimized.

Contributor

kubawolanin commented Jan 21, 2017

@rtvb @ThomDietrich @hakan42 @splatch Gentlemen, thank you so much for your tremendous help!
I think I've applied all of the changes needed.

I've updated my first post with compiled JAR file.

Thanks again! 😃

AirQualityConfiguration config = getConfigAs(AirQualityConfiguration.class);
logger.debug("config location = {}", config.getLocation());
logger.debug("config refresh = {}", config.getRefresh());

This comment has been minimized.

@ThomDietrich

ThomDietrich Jan 21, 2017

Member

Hey! You've removed the line, which is totally fine. What I actually meant to suggest was to output a hardcoded "config apikey = (omitted from logging)". This way the user still knows/has the feeling his configuration was taken into account. I believe that is the actual purpose of these logger lines!?
This is a mere unimportant detail. Do as you feel it's right ;)

This comment has been minimized.

@kubawolanin

kubawolanin Jan 22, 2017

Contributor

Done! Thank you:)

case AirQualityBindingConstants.LOCATIONURL:
return data.getData().getCity().getUrl();
case AirQualityBindingConstants.OBSERVATIONTIME:
return data.getData().getTime().getS();

This comment has been minimized.

@metbril

metbril Jan 22, 2017

Contributor

To respect time zones you should probably parse the 'v' tag (Epoch) or add 'tz'

This comment has been minimized.

@kubawolanin

kubawolanin Jan 22, 2017

Contributor

Done!

@metbril

Suggested to rename channel ids to better match their contents.

</config-description>
</thing-type>
<channel-type id="aqiLevel">

This comment has been minimized.

@metbril

metbril Jan 22, 2017

Contributor

Channel id matches actual value better if it is called aqi or aqiId

<state readOnly="true" pattern="%d"></state>
</channel-type>
<channel-type id="aqiDescription">

This comment has been minimized.

@metbril

metbril Jan 22, 2017

Contributor

According to the website, this value is referred to as "scale" or "level". Channel id could perhaps better match this, so instead of aqiDescription use aqiLevel. Please note: you are using level for the actual index.
In my opinion an index is a value where a level is a range of values.

This comment has been minimized.

@kubawolanin

kubawolanin Jan 22, 2017

Contributor

I don't think it makes much of a difference. I've had aqiLevel named as aqi but this conflicted with the thing type I wanted to keep short.
Let's leave it like it is now, I think it's descriptive.

This comment has been minimized.

@metbril

metbril Jan 22, 2017

Contributor

Your use of the word level is different than that from the website. This might lead to confusion. But hey, it's your binding, so do as you feel is best.

@metbril

Add unicode characters to fancy up sitemap. 😉

Number Aqi_Pm25 "PM25 Level" <line> (AirQuality) { channel="airquality:aqi:home:pm25" }
Number Aqi_Pm10 "PM10 Level" <line> (AirQuality) { channel="airquality:aqi:home:pm10" }
Number Aqi_O3 "O3 Level" <line> (AirQuality) { channel="airquality:aqi:home:o3" }

This comment has been minimized.

@metbril

metbril Jan 22, 2017

Contributor

Replace label with "O\u2083 Level" to show subscript 3

https://en.wikipedia.org/wiki/Unicode_subscripts_and_superscripts

Number Aqi_Pm25 "PM25 Level" <line> (AirQuality) { channel="airquality:aqi:home:pm25" }
Number Aqi_Pm10 "PM10 Level" <line> (AirQuality) { channel="airquality:aqi:home:pm10" }
Number Aqi_O3 "O3 Level" <line> (AirQuality) { channel="airquality:aqi:home:o3" }
Number Aqi_No2 "NO2 Level" <line> (AirQuality) { channel="airquality:aqi:home:no2" }

This comment has been minimized.

@metbril

metbril Jan 22, 2017

Contributor

Replace label with "NO\u2082 Level" to show subscript 2

https://en.wikipedia.org/wiki/Unicode_subscripts_and_superscripts

This comment has been minimized.

@kubawolanin

kubawolanin Jan 22, 2017

Contributor

Sure, why not!:)

This comment has been minimized.

@metbril

metbril Jan 22, 2017

Contributor

Same goes for Pm25 and pm10. Too bad I haven't been able to locate a subscript dot (for subscript 2.5).

case AirQualityBindingConstants.LOCATIONURL:
return data.getData().getCity().getUrl();
case AirQualityBindingConstants.OBSERVATIONTIME:
return data.getData().getTime().getS();

This comment has been minimized.

@metbril

metbril Jan 22, 2017

Contributor

Specifically for the purpose that @kaikreuzer explicitly asked for reviewers' approval, I've started a new review with required changes. The timezone bug should be finished before merging.

This comment has been minimized.

@kubawolanin

kubawolanin Jan 22, 2017

Contributor

Done! Thanks for review!

* @return {String}
*/
public String getTz() {
return tz.replace(":", "");

This comment has been minimized.

@ThomDietrich

ThomDietrich Jan 22, 2017

Member

I'm looking at this on GitHub only, so sorry if I missed that: Where it tz initialized?

This comment has been minimized.

@kubawolanin

kubawolanin Jan 22, 2017

Contributor

Right there :) (line :25)

This comment has been minimized.

@ThomDietrich

ThomDietrich Jan 22, 2017

Member

😃 okay, let me be more precise: where is it set?

This comment has been minimized.

@kubawolanin

kubawolanin Jan 22, 2017

Contributor

It's being transformed by Gson from the response to a property.

This comment has been minimized.

@ThomDietrich

ThomDietrich Jan 22, 2017

Member

Ah, I see it now.
To be honest, I have little experience with serialization. Is there a better way to make that clear in source code? One has to go three classes "up" to even find the gson conversion.

This comment has been minimized.

@kubawolanin

kubawolanin Jan 23, 2017

Contributor

Well, It's a good practice for classes to be responsible for one thing.
In this case @splatch moved all JSON parsing-related logic to org.openhab.airquality.intrernal.json package.
In Handler itself, we're executing Gson so it knows where to put all data
This gives ability to scale the classes in case of e.g. API change or new entry points coming with the payload :)
Cheers

This comment has been minimized.

@splatch

splatch Jan 23, 2017

I was checking that and there is no nice way to convert time representation given by aqicn to java.util.Date or Calendar directly. Sadly there is none. With given data
{v: int, s: string, tz:"01:00"} we could convert unix timestamp coded in v field of json to java timestamp (coded in millis), but then we have to do dance with timezone. This means that only custom Gson TypeAdapter will cover that. In this way we would close time related handling in separate type, but since there is no other uses of this structure benefits are minor. Without joda time or java 8 date-time api it will always be counter intuitive.
One thing we could do is change of simple date fromat to yyyy-MM-dd HH:mm:ssX to parse concatenated s+ tz fields easily. X allows to specify time zone offset in format returned by service.

This comment has been minimized.

@metbril

metbril Jan 23, 2017

Contributor

Taking into account my note below, I think getting the proper time is to simply concatenate the s and tz values and parse them with DateTimeFormatter with ISO_OFFSET_DATE_TIME or something similar?

https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html#ISO_OFFSET_DATE_TIME

@metbril

lgtm

@metbril

This comment has been minimized.

Contributor

metbril commented Jan 23, 2017

I did some additional testing with time zones. And notice some differences for Rotterdam and Moscow. Perhaps the date in the JSON is local time and does not need modification after all. See issue:

kubawolanin#2

@ThomDietrich

This comment has been minimized.

Member

ThomDietrich commented Jan 24, 2017

@kubawolanin my "where is tz set" question was still bugging me.
Here is why: Besides the small comment above the class definition, there is no clear hint to any special nature of that class. All I was seeing was a uninitialized (null) String and a replace call - which last time I checked will throw an exception. The actual meaning got clear to me only after you pointed out the usage with gson, way outside of the class scope.

The class should be annotated in some way to make the gson relation clear. It seems, SerializedName would be an approach here. @splatch are there better approaches to make the software design clear and the class self-contained and sane?

@kubawolanin kubawolanin force-pushed the kubawolanin:feature/airquality branch from a155982 to 6ccc124 Jan 28, 2017

@kubawolanin

This comment has been minimized.

Contributor

kubawolanin commented Jan 28, 2017

@ThomDietrich I've added some more annotations and SerializedName for the values as well. Hope that'll do :)

@rtvb thank you for this. I'm no longer modifying it.

BTW. I've updated my first post with latest build.
Per discussion on the forum - there was an update in the API that caused trouble in displaying PM2.5, PM10 etc.

The latest build also includes:

  • More descriptive error display (e.g. if there's something wrong with config)
  • Fixed docs + added Unicode chars
  • Removed Station ID from the channels (it's no longer there in the payload)
  • Better Location description (I've deleted " from the example to avoid confusion)
  • Changed measurements from int to floats (per API change)

Thank you all for the cooperation!

@metbril

This comment has been minimized.

Contributor

metbril commented Jan 29, 2017

I will do some additional testing for NYC, London and Moscow re. time zones.

That was not my advice. I meant MY local time, not that of the station. As far as I'm concerned I still consider this a bug since a DateTime item in OH is always considered to be in the time zone of the OH installation and NOT in any other time zone.

@metbril

metbril suggested changes Jan 29, 2017 edited

The observationTime channel still returns the local date of the station without the time zone. Instead it should return the time WITH the timezone, since the DateTime item in the OH2 configuration would then be represented in the localized time of the OH system itself.

See: http://stackoverflow.com/a/19112487

My testing setup:

things

airquality:aqi:rtd "AirQuality at RTD" [ apikey="XXX", location="51.92, 4.48", refresh=10 ]
airquality:aqi:lon "AirQuality at LON" [ apikey="XXX", location="51.51, -0.13", refresh=10 ]
airquality:aqi:mow "AirQuality at MOW" [ apikey="XXX", location="55.75,37.62", refresh=10 ]
airquality:aqi:nyc "AirQuality at NYC" [ apikey="XXX", location="40.76,-74.02", refresh=10 ]

items

Group AirQuality

// CET
Group AqiRTD "Rotterdam (CET)" (AirQuality)
Number   Aqi_RTD_Index           "Index [%d]"                <line>  (AqiRTD) { channel="airquality:aqi:rtd:aqiLevel" }
String   Aqi_RTD_LocationName        "Location [%s]"         <text>  (AqiRTD) { channel="airquality:aqi:rtd:locationName" }
DateTime Aqi_RTD_ObservationTime "Observation [%1$tY-%1$tm-%1$td %1$tH:%1$tM %1$tz]" <clock> (AqiRTD) { channel="airquality:aqi:rtd:observationTime" }

// GMT
Group AqiLON "London (GMT)" (AirQuality)
Number   Aqi_LON_Index           "Index [%d]"                <line>  (AqiLON) { channel="airquality:aqi:lon:aqiLevel" }
String   Aqi_LON_LocationName        "Location [%s]"         <text>  (AqiLON) { channel="airquality:aqi:lon:locationName" }
DateTime Aqi_LON_ObservationTime "Observation [%1$tY-%1$tm-%1$td %1$tH:%1$tM %1$tz]" <clock> (AqiLON) { channel="airquality:aqi:lon:observationTime" }

// MSK
Group    AqiMOW "Moscow (MSK)" (AirQuality)
Number   Aqi_MOW_Index           "Index [%d]"                <line>  (AqiMOW) { channel="airquality:aqi:mow:aqiLevel" }
String   Aqi_MOW_LocationName    "Location [%s]"             <text>  (AqiMOW) { channel="airquality:aqi:mow:locationName" }
DateTime Aqi_MOW_ObservationTime "Observation [%1$tY-%1$tm-%1$td %1$tH:%1$tM %1$tz]" <clock> (AqiMOW) { channel="airquality:aqi:mow:observationTime" }

// EST
Group AqiNYC "New York (EST)" (AirQuality)
Number   Aqi_NYC_Index           "Index [%d]"                <line>  (AqiNYC) { channel="airquality:aqi:nyc:aqiLevel" }
String   Aqi_NYC_LocationName    "Location [%s]"             <text>  (AqiNYC) { channel="airquality:aqi:nyc:locationName" }
DateTime Aqi_NYC_ObservationTime "Observation [%1$tY-%1$tm-%1$td %1$tH:%1$tM %1$tz]" <clock> (AqiNYC) { channel="airquality:aqi:nyc:observationTime" }

sitemap

sitemap airquality label="Air Quality Index" {

	Frame item=AqiNYC {
		Text item=Aqi_NYC_Index
		Text item=Aqi_NYC_LocationName
		Text item=Aqi_NYC_ObservationTime
	}
	Frame item=AqiLON {
		Text item=Aqi_LON_Index
		Text item=Aqi_LON_LocationName
		Text item=Aqi_LON_ObservationTime
	}
	Frame item=AqiRTD {
		Text item=Aqi_RTD_Index
		Text item=Aqi_RTD_LocationName
		Text item=Aqi_RTD_ObservationTime
	}
	Frame item=AqiMOW {
		Text item=Aqi_MOW_Index
		Text item=Aqi_MOW_LocationName
		Text item=Aqi_MOW_ObservationTime
	}

}
@SerializedName("timeZone")
private String tz;
private final static SimpleDateFormat SDF = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");

This comment has been minimized.

@metbril

metbril Jan 29, 2017

Contributor

Why not use "yyyy-MM-dd'T'HH:mm:ssZ" as the pattern and add the tz to the string when creating the date?

*/
public Calendar getDateString() throws Exception {
Calendar calendar = Calendar.getInstance();
Date date = SDF.parse(dateString);

This comment has been minimized.

@metbril

metbril Jan 29, 2017

Contributor

Perhaps replace with:

Date date = SDF.parse(dateString + tz);

If adding the timezone to the formatter and the build string, shouldn't you get the observation time in the correct time zone?

@kubawolanin

This comment has been minimized.

Contributor

kubawolanin commented Jan 29, 2017

@rtvb then I misunderstood your post.
Here's your whole setup on my environment - I've applied date+timezone concatenation again and used yyyy-MM-dd'T'HH:mm:ssZ format.
Here's the result. Would that satisfy your requrement?
clipboard02

@metbril

This comment has been minimized.

Contributor

metbril commented Jan 29, 2017

All times now seem to be in my local zone. That would satisfy my needs

@kubawolanin kubawolanin force-pushed the kubawolanin:feature/airquality branch from 6ccc124 to 207f83c Jan 29, 2017

@kubawolanin

This comment has been minimized.

Contributor

kubawolanin commented Jan 29, 2017

Hey @rtvb - I've pushed my latest changes to the repo!
Feel free to test it - I hope it's ok now! 😃

@metbril

This comment has been minimized.

Contributor

metbril commented Jan 31, 2017

Cool updates to units and colors you did in that commit 612a21d

I will try the new binding the next coming days and report back.

@hakan42

This comment has been minimized.

Contributor

hakan42 commented Jan 31, 2017

https://airvisual.com/world seems to use the same data, the stations in my area of the world are exactly the same as shown by http://aqicn.org/ ... Maybe something to add to the README.md as "Conrad Connect" in Germany advertises that they have a channel for airvisual.com now?

@kubawolanin

This comment has been minimized.

Contributor

kubawolanin commented Feb 1, 2017

Maybe something to add to the README.md as "Conrad Connect" in Germany advertises that they have a channel for airvisual.com now?

You propose adding info about alternative API to the README? I don't think I understand.
Maybe in the future we could choose (in config?) where you want to get the data from, but I think it won't not useful for now.

@ThomDietrich

LGTM

@kubawolanin kubawolanin force-pushed the kubawolanin:feature/airquality branch from 612a21d to 9fb895c Feb 4, 2017

@kubawolanin

This comment has been minimized.

Contributor

kubawolanin commented Feb 4, 2017

I've updated my first post with the latest build.
What's new:

Thank you!

@kaikreuzer the PR gained two approvals so pinging you for awareness ;-)

@kaikreuzer

Thanks for this contribution - please find my review comments below!

Please also add:

  • .project and .classpath files as this is required for the automatic IDE setup
  • an entry in feature.xml, which will make sure that your binding is included in the distro.
<thing-type id="aqi">
<label>Air Quality</label>
<description>Provides various air quality data from the World Air Quality Project.
In order to receive the data, you must register an account on http://aqicn.org/data-platform/token/ and get

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 13, 2017

Member

please apply the auto-formatter - there is a tabs/spaces mix in here

<parameter name="location" type="text" required="false"
pattern="^[-+]?([1-8]?\d(\.\d+)?|90(\.0+)?)[,|;]\s*[-+]?(180(\.0+)?|((1[0-7]\d)|([1-9]?\d))(\.\d+)?)$">
<label>Location</label>
<description>Your geo coordinates separated with comma or semicolon (e.g. "37.8;-122.4").</description>

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 13, 2017

Member

I wouldn't allow two options, but only go for comma as a separator (similar as the astro binding).
I think we should also introduce a "context" for geo locations as config parameters, so that the same format is used by all (and UIs might even show a map for choosing a location).

<state readOnly="true" pattern="%s"></state>
</channel-type>
<channel-type id="locationUrl" advanced="true">

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 13, 2017

Member

Is this channel really useful? We do not support links in item states, so I do not know, how this should be used in any meaningful way?

| stationId | Unique ID of the measuring station. |
| refresh | Refresh interval in minutes. Optional, the default value is 60 minutes. |
For the location parameter, the following syntaxes are allowed (comma or semicolon):

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 13, 2017

Member

see comment above - imho comma is enough

</channel-type>
<channel-type id="stationLocation" advanced="true">
<item-type>String</item-type>

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 13, 2017

Member

This should clearly be a "Location" item, shouldn't it?

/**
* Get new data from aqicn.org service
*
* @return {boolean}

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 13, 2017

Member

boolean?

// Run the HTTP request and get the JSON response from aqicn.org
URL url = new URL(urlStr);
URLConnection connection = url.openConnection();
String repsonse = IOUtils.toString(connection.getInputStream());

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 13, 2017

Member

typo: response

This comment has been minimized.

@kaikreuzer
logger.warn("Error running aqicn.org (Air Quality) request: {}", errorMsg);
} catch (IOException | IllegalStateException e) {
errorMsg = e.getMessage();
logger.warn("Error running aqicn.org (Air Quality) request: {}", errorMsg);

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 13, 2017

Member

I would expect IOExceptions to happen if you do not have a network connection, right?
So according to the guidelines, you must not log a warning here, but set the Thing status accordingly only (what you do below).

*
* @author Kuba Wolanin - Initial contribution
*/
public class AirQualityConfiguration {

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 13, 2017

Member

No, the idea of the Configuration classes is actually to have public fields and no need for boilerplate methods. This is also done in other bindings that way.

for (int i = 0; i < geo.size(); i++) {
list.add(geo.get(i).toString());
}
return String.join(", ", list);

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 13, 2017

Member

join only exists in Java8, while your MANIFEST.MF states Java7 as "Bundle-RequiredExecutionEnvironment". You should adapt the MANIFEST.MF.

@kaikreuzer

This comment has been minimized.

Member

kaikreuzer commented Feb 13, 2017

@kaikreuzer the PR gained two approvals so pinging you for awareness ;-)

@rtvb & @ThomDietrich: Thanks for helping on the review! Let us try to align about the requirements that need to be met before approving a PR - the main checklist should be the guidelines. If you go through my comment, you will notice that a couple of things are still missing there; I'll try to check where to best put such infos (like e.g. about .project & feature.xml files).

@kubawolanin

This comment has been minimized.

Contributor

kubawolanin commented Feb 13, 2017

@kaikreuzer thank you for the review! I'll address the comments as soon as I find some time for it (hopefully this week).

AirQuality binding - initial commit
Signed-off-by: Kuba Wolanin <hi@kubawolanin.com>
Also-by: Łukasz Dywicki <luke@code-house.org>

Air Quality: code review follow-up
AirQuality bugfixes
AirQuality code review followup
AirQuality - sitemap in README touchup and units added
AirQuality enhancements
Signed-off-by: Kuba Wolanin <hi@kubawolanin.com>

AirQuality binding - code review follow-up

Signed-off-by: kubawolanin <hi@kubawolanin.com>

@kubawolanin kubawolanin force-pushed the kubawolanin:feature/airquality branch from 9fb895c to ce70d68 Feb 15, 2017

@kubawolanin

This comment has been minimized.

Contributor

kubawolanin commented Feb 16, 2017

@kaikreuzer I've applied all of the comments. Thanks again for your insight!

Regarding the binding itself, a few breaking changes:

  1. Location configuration no longer understands semicolon - you need to use comma instead (which is now compliant with other bindings)
  2. I've removed stationUrl channel - I couldn't find any reason to keep it. Let me know if you find any use case for it!
  3. Aqi_StationGeo item (and stationLocation channel) is now a Location element (instead of String) which allows you to display it on a map (closes kubawolanin#1)

I will update my first post (and the community thread) with latest compiled JAR in a few hours from now.

Thanks again! 😎

Bundle-Name: AirQuality Binding
Bundle-SymbolicName: org.openhab.binding.airquality;singleton:=true
Bundle-Vendor: openHAB
Bundle-Version: 2.0.0.qualifier

This comment has been minimized.

@kaikreuzer

kaikreuzer Feb 16, 2017

Member

One very last issue: This needs to be 2.1.0.qualifier!

This comment has been minimized.

@kubawolanin

kubawolanin Feb 16, 2017

Contributor

Forgot about this one. Thanks! :)

AirQuality - changed Bundle-Version
Changed Bundle-Version to 2.1.0.qualifier
Signed-off-by: Kuba Wolanin <hi@kubawolanin.com>
@kaikreuzer

This comment has been minimized.

Member

kaikreuzer commented Feb 16, 2017

perfect, you are quick! So no need to share another jar - I'll directly run a new official build, which will contain it 👍

@kaikreuzer kaikreuzer merged commit 00ca90a into openhab:master Feb 16, 2017

@kubawolanin kubawolanin deleted the kubawolanin:feature/airquality branch Feb 16, 2017

@metbril

This comment has been minimized.

Contributor

metbril commented Feb 16, 2017

Will the README be added to the docs site automatically or do we need to create an additional issue or pull request?

@kaikreuzer

This comment has been minimized.

Member

kaikreuzer commented Feb 16, 2017

fharni added a commit to fharni/openhab2-addons that referenced this pull request Feb 19, 2017

AirQuality binding (openhab#1738)
* AirQuality binding - initial commit

Also-by: Łukasz Dywicki <luke@code-house.org>
Signed-off-by: Kuba Wolanin <hi@kubawolanin.com>

jarlebh added a commit to jarlebh/openhab2-addons that referenced this pull request Mar 4, 2017

AirQuality binding (openhab#1738)
* AirQuality binding - initial commit

Also-by: Łukasz Dywicki <luke@code-house.org>
Signed-off-by: Kuba Wolanin <hi@kubawolanin.com>

cweitkamp added a commit to cweitkamp/openhab2-addons that referenced this pull request Mar 26, 2017

AirQuality binding (openhab#1738)
* AirQuality binding - initial commit

Also-by: Łukasz Dywicki <luke@code-house.org>
Signed-off-by: Kuba Wolanin <hi@kubawolanin.com>

tratho added a commit to tratho/openhab2-addons that referenced this pull request May 5, 2017

AirQuality binding (openhab#1738)
* AirQuality binding - initial commit

Also-by: Łukasz Dywicki <luke@code-house.org>
Signed-off-by: Kuba Wolanin <hi@kubawolanin.com>

@kaikreuzer kaikreuzer modified the milestone: 2.1 Jun 24, 2017

Markinus added a commit to Markinus/openhab2-addons that referenced this pull request Jul 2, 2017

AirQuality binding (openhab#1738)
* AirQuality binding - initial commit

Also-by: Łukasz Dywicki <luke@code-house.org>
Signed-off-by: Kuba Wolanin <hi@kubawolanin.com>

aogorek pushed a commit to aogorek/openhab2-addons that referenced this pull request Jul 5, 2017

AirQuality binding (openhab#1738)
* AirQuality binding - initial commit

Also-by: Łukasz Dywicki <luke@code-house.org>
Signed-off-by: Kuba Wolanin <hi@kubawolanin.com>

Markinus added a commit to Markinus/openhab2-addons that referenced this pull request Sep 8, 2017

AirQuality binding (openhab#1738)
* AirQuality binding - initial commit

Also-by: Łukasz Dywicki <luke@code-house.org>
Signed-off-by: Kuba Wolanin <hi@kubawolanin.com>
@emme-g

This comment has been minimized.

emme-g commented Oct 29, 2017

Hello,
some days ago I started to use this useful binding and I could set almost everything in an easy and quick way.
In the last days I was trying to understand in a better way how the AQI is calculated because some values I could read through the binding were a little unclear for me.
I understood that the total AQI is calculated as the maximum value among the individual AQIs related to the single pollutants (PM2,5, PM10, O3, NO2, CO), as it is better explained in Wikipedia
https://en.wikipedia.org/wiki/Air_quality_index.
I noticed that in the air quality binding the values of the items related to the single pollutants contain the μg/m³ dimension, but it seems to me that the individual AQIs related to the single pollutants are just indexes (without dimensions) and not values with μg/m³ dimensions, as we can read in this page
http://aqicn.org/faq/2013-09-09/revised-pm25-aqi-breakpoints/
where for example it is explained that a value 50 of the PM2,5 individual AQI corresponds to a PM2,5 concentration of 12 μg/m³.
So, I think that it should be better that the item values would appear in the binding without any dimension.
It would be very interesting to receive the values of the concentrations of the single pollutants too (adding some more items), but I think that this totally depends on the AQIcn.org service and eventually it should be asked to them.
Thank you for your work, I hope this opinion could be helpful.

@kevinshane

This comment has been minimized.

kevinshane commented Dec 1, 2017

Hi, I am using docker openhab 2.1.0 stable version with airquality binding installed, howerver I have some strange errors: when restarting the OS, the airquality binding go online at startup and I get all the weather info, but the binding immediately changed from online back to initializing and then go through the process again, finally go online once again, but this time I lose all the weather info, every item changed from current value to UNDEF.. and never go back.. what's the problem here?
here is the log:

2017-12-01 20:57:50.875 [ItemStateChangedEvent     ] - Aqi_Level changed from NULL to 68
2017-12-01 20:57:50.878 [ItemStateChangedEvent     ] - Aqi_Description changed from NULL to MODERATE
2017-12-01 20:57:50.882 [ItemStateChangedEvent     ] - Aqi_Pm25 changed from NULL to 68
2017-12-01 20:57:50.887 [ItemStateChangedEvent     ] - Aqi_Pm10 changed from NULL to 45
2017-12-01 20:57:50.889 [ItemStateChangedEvent     ] - Aqi_O3 changed from NULL to 53.2
2017-12-01 20:57:50.892 [ItemStateChangedEvent     ] - Aqi_No2 changed from NULL to 10.6
2017-12-01 20:57:50.895 [ItemStateChangedEvent     ] - Aqi_Co changed from NULL to 9.1
2017-12-01 20:57:50.899 [ItemStateChangedEvent     ] - Aqi_Temperature changed from NULL to 21.15
2017-12-01 20:57:50.909 [ItemStateChangedEvent     ] - Aqi_Pressure changed from NULL to 1019
2017-12-01 20:57:50.912 [ItemStateChangedEvent     ] - Aqi_Humidity changed from NULL to 68
2017-12-01 20:57:51.034 [hingStatusInfoChangedEvent] - 'airquality:aqi:home' changed from UNINITIALIZED to INITIALIZING
2017-12-01 20:57:51.087 [hingStatusInfoChangedEvent] - 'airquality:aqi:home' changed from INITIALIZING to ONLINE
2017-12-01 20:57:51.091 [ItemStateChangedEvent     ] - Aqi_Level changed from 68 to UNDEF
2017-12-01 20:57:51.095 [ItemStateChangedEvent     ] - Aqi_Description changed from MODERATE to UNDEF
2017-12-01 20:57:51.119 [ItemStateChangedEvent     ] - Aqi_Pm25 changed from 68 to UNDEF
2017-12-01 20:57:51.121 [ItemStateChangedEvent     ] - Aqi_Pm10 changed from 45 to UNDEF
2017-12-01 20:57:51.124 [ItemStateChangedEvent     ] - Aqi_No2 changed from 10.6 to UNDEF
2017-12-01 20:57:51.139 [ItemStateChangedEvent     ] - Aqi_O3 changed from 53.2 to UNDEF
2017-12-01 20:57:51.142 [ItemStateChangedEvent     ] - Aqi_Co changed from 9.1 to UNDEF
2017-12-01 20:57:51.144 [ItemStateChangedEvent     ] - Aqi_Temperature changed from 21.15 to UNDEF
2017-12-01 20:57:51.146 [ItemStateChangedEvent     ] - Aqi_Pressure changed from 1019 to UNDEF
2017-12-01 20:57:51.149 [ItemStateChangedEvent     ] - Aqi_Humidity changed from 68 to UNDEF
2017-12-01 20:57:52.129 [ERROR] [airquality.handler.AirQualityHandler] - Exception occurred during execution: null
java.lang.NullPointerException
	at org.openhab.binding.airquality.internal.json.AirQualityJsonData.getAttributions(AirQualityJsonData.java:86)[194:org.openhab.binding.airquality:2.1.0]
	at org.openhab.binding.airquality.handler.AirQualityHandler.getAirQualityData(AirQualityHandler.java:270)[194:org.openhab.binding.airquality:2.1.0]
	at org.openhab.binding.airquality.handler.AirQualityHandler.updateAirQualityData(AirQualityHandler.java:198)[194:org.openhab.binding.airquality:2.1.0]
	at org.openhab.binding.airquality.handler.AirQualityHandler.access$0(AirQualityHandler.java:196)[194:org.openhab.binding.airquality:2.1.0]
	at org.openhab.binding.airquality.handler.AirQualityHandler$1.run(AirQualityHandler.java:114)[194:org.openhab.binding.airquality:2.1.0]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)[:1.8.0_144]
	at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)[:1.8.0_144]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)[:1.8.0_144]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)[:1.8.0_144]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)[:1.8.0_144]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)[:1.8.0_144]
	at java.lang.Thread.run(Thread.java:748)[:1.8.0_144]

I restart the docker openhab 2.1.0 and all of the sudden all items changed from UNDEF back to read the value again, in this case the value will continue to read without any problem, but the Exception occurred during execution: null keep showing in the log for every 30 mins (sort of 30 min, I didn't count)
would u please take a look at what's happening to cause this problem? thanks

@kubawolanin

This comment has been minimized.

Contributor

kubawolanin commented Dec 1, 2017

@emme-g, @kevinshane - please submit separate issues for your requests. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment