-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[RFXCOM] Support all data from wind sensors #2329
Conversation
Previous code only supported wind direction and wind speed (actually gust speed). Added support for average wind speed, temperature and chill temperature where available. Closes #2267 Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
@mjagdis, thanks for your PR! By analyzing the history of the files in this pull request, we identified @paulianttila, @martinvw and @kaikreuzer to be potential reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, one small issue and please also update the readme or add it as a note to https://github.com/openhab/openhab2-addons/pull/2327, I'm busy at it anyway.
@@ -15,8 +15,19 @@ | |||
<description>A Wind device.</description> | |||
|
|||
<channels> | |||
<channel id="windSpeed" typeId="windspeed" /> | |||
<channel id="avgWindSpeed" typeId="windspeed" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an XML parsing error, here, it should not be self-closing I think
@@ -72,8 +73,7 @@ | |||
public static final String CHANNEL_RAIN_TOTAL = "rainTotal"; | |||
public static final String CHANNEL_WIND_DIRECTION = "windDirection"; | |||
public static final String CHANNEL_WIND_SPEED = "windSpeed"; | |||
public static final String CHANNEL_GUST = "gust"; | |||
public static final String CHANNEL_CHILL_FACTOR = "chillFactor"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these already unused and not defined somewhere else, where they also should be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were unused. Previously the gust value was what was being returned as windSpeed so to avoid breaking existing "working" setups I elected to keep that and add averageWindSpeed.
str += ", Battery level = " + batteryLevel; | ||
|
||
return str; | ||
return super.toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this style better :-)
Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
@@ -68,6 +68,9 @@ public static SubType fromByte(int input) throws RFXComUnsupportedValueException | |||
public int sensorId; | |||
public double windDirection; | |||
public double windSpeed; | |||
public double avgWindSpeed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite some changes, should you be added as an co-author?
Same question should we wait for the user to respond? Or do you think its safe to merge already? |
Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
Okay, the user confirmed, if we have the readme updated it can be merged :-) |
* master: [senseBox] Forgot to add binding to feature.xml (openhab#2371) [Kodi] Fix audio sink (openhab#2301) (openhab#2303) Initial contribution of a Niko Home Control binding. (openhab#1589) [keba] fixed typo (openhab#2370) Removed utilisation of org.apache.http.client (openhab#2369) [RFXCOM] Make LWRF mood buttons work (openhab#2366) [senseBox] Switch caching to ESH ExpiringCacheMap (openhab#2361) Fixed problems found by AuthorTagCheck in the cometvisu bundle (openhab#2364) Added author tag where missing. (openhab#2351) [RFXCOM] Fix chill temperature (openhab#2362) initial contribution Windcentrale binding (openhab#1535) Tesla : reset the Even Stream connection to the Tesla Back-end if the event time stamps differ too much from the current system time (openhab#2358) Homematic: Some updates (openhab#2346) [RFXCOM] Support all data from wind sensors (openhab#2329) Gardena: Optimized refresh after command (openhab#2353) Fix some javadocs. (openhab#2349) Some small non-standard copyright banners which we missed while reviewing (openhab#2347)
Previous code only supported wind direction and wind speed (actually gust speed). Added support for average wind speed, temperature and chill temperature where available. Closes openhab#2267 Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
Previous code only supported wind direction and wind speed (actually gust speed). Added support for average wind speed, temperature and chill temperature where available. Closes openhab#2267 Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
Previous code only supported wind direction and wind speed (actually gust speed). Added support for average wind speed, temperature and chill temperature where available. Closes openhab#2267 Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
Previous code only supported wind direction and wind speed (actually gust speed). Added support for average wind speed, temperature and chill temperature where available. Closes openhab#2267 Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
Previous code only supported wind direction and wind speed (actually gust speed). Added support for average wind speed, temperature and chill temperature where available. Closes openhab#2267 Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
Previous code only supported wind direction and wind speed (actually gust speed). Added support for average wind speed, temperature and chill temperature where available. Closes openhab#2267 Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
Previous code only supported wind direction and wind speed (actually gust speed). Added support for average wind speed, temperature and chill temperature where available. Closes openhab#2267 Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
Previous code only supported wind direction and wind speed (actually gust speed). Added support for average wind speed, temperature and chill temperature where available. Closes openhab#2267 Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
@@ -60,14 +61,17 @@ public static SubType fromByte(int input) throws RFXComUnsupportedValueException | |||
|
|||
private static final List<RFXComValueSelector> SUPPORTED_INPUT_VALUE_SELECTORS = Arrays.asList( | |||
RFXComValueSelector.SIGNAL_LEVEL, RFXComValueSelector.BATTERY_LEVEL, RFXComValueSelector.WIND_DIRECTION, | |||
RFXComValueSelector.WIND_SPEED); | |||
RFXComValueSelector.AVG_WIND_SPEED, RFXComValueSelector.WIND_SPEED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could both of us miss this 😉
The temperature fields were not added here.
See https://github.com/openhab/openhab2-addons/issues/2495#issuecomment-318463340
If you ( @mjagdis ) would have time to fix this that would be great, I have no opportunity because I m not near anything except for a tablet for at least one more week.
Previous code only supported wind direction and wind speed (actually gust speed). Added support for average wind speed, temperature and chill temperature where available. Closes openhab#2267 Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
Previous code only supported wind direction and wind speed (actually gust speed). Added support for average wind speed, temperature and chill temperature where available. Closes openhab#2267 Signed-off-by: Mike Jagdis <[email protected]> (github: mjagdis)
Signed-off-by: Wouter Born <[email protected]>
Previous code only supported wind direction and wind speed (actually
gust speed). Added support for average wind speed, temperature and chill
temperature where available.
Closes #2267
Signed-off-by: Mike Jagdis [email protected] (github: mjagdis)