Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[mysensors] MySensors Binding initial contribution #2066

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
@tobof
Copy link

commented Mar 22, 2017

This binding adds support for MySensors to OpenHAB2.

The binding supports the serial and ethernet gateway.

MySensors: https://www.mysensors.org

Thread related to the binding: https://forum.mysensors.org/topic/1598/openhab-2-0-binding/412

static-code-analysis tool was executed and changes to conform OH2 coding guidelines are done.

Due to changes in the Repo the original PR (#1243) was closed.

Also-by: Andrea Cioni supercionci@hotmail.it (github: andreacioni)
Signed-off-by: Tim Oberföll oberfoell@web.de (github: tobof)

@mention-bot

This comment has been minimized.

Copy link

commented Mar 22, 2017

@tobof, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kaikreuzer, @xylo and @tmrobert8 to be potential reviewers.

@hakan42
Copy link
Contributor

left a comment

A few comments without through review yet.

<plugins>
<plugin>
<groupId>${tycho-groupid}</groupId>
<artifactId>tycho-compiler-plugin</artifactId>

This comment has been minimized.

Copy link
@hakan42

hakan42 Mar 22, 2017

Contributor

Is the specific configuration of tycho-compiler still necessary? We are on JDK 1.8 😄

This comment has been minimized.

Copy link
@andreacioni

andreacioni Mar 23, 2017

The tycho-compiler has been added because, in the past, we encountered errors on "maven package" target run. Do you think is unnecessary?

Bundle-SymbolicName: org.openhab.binding.mysensors;singleton:=true
Bundle-Vendor: openHAB
Bundle-Version: 2.1.0.qualifier
Bundle-RequiredExecutionEnvironment: JavaSE-1.7

This comment has been minimized.

Copy link
@hakan42

hakan42 Mar 22, 2017

Contributor

Everywhere else we are referencing 1.8. Should the RequiredExecutionEnvironment be JavaSE-1.8 too?

This comment has been minimized.

Copy link
@andreacioni

andreacioni Mar 23, 2017

Yes, we use Java 8 feature, this should be changed.

@martinvw
Copy link
Member

left a comment

Thanks for your efforts, I can see you invested a lot of time in this binding 👍

I might have been reviewing a little to enthusiastic, make it like just another code review I also do when working.

There are a lot of small remarks, like unclear naming, overly defensive programming or unneeded initialisations, I'll mark them italic you can ignore them if you want but they are there to improve your code :-)

I did some initial review, I got till 'internal/MySensorsUtility.java' I hope to continue later on.


For more information on the features take a look at the summary at https://www.mysensors.org/controller

Installation, configuration and examples are described in the wiki https://github.com/tobof/openhab2-addons/wiki

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 23, 2017

Member

Is this the proper place to document it? I think not.

public Integer baudRate; // baud rate used to connect the serial port
public Boolean imperial; // should nodes send imperial or metric values?
public Boolean skipStartupCheck; // should the startup check of the bridge at boot skipped?
public Boolean enableNetworkSanCheck; // network sanity check enabled?

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 23, 2017

Member

Maybe it's wiser to improve some of the names and leave out the extra comments

Why not just networkSanityCheckEnabled instead of having that line of comment?


@Override
public String toString() {
return "MySensorsBridgeConfiguration [serialPort=" + serialPort + ", ipAddress=" + ipAddress + ", tcpPort="

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 23, 2017

Member

You could consider using a string formatter, StringBuilder or some new lines to improve readability here

public class MySensorsOnOffTypeConverter implements MySensorsTypeConverter {

@Override
public State fromString(String s) {

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 23, 2017

Member

Maybe s is not the best variable name, although the scope is limited

} else if (value == OnOffType.ON) {
return "1";
} else {
throw new IllegalArgumentException("Passed command is not On/Off");

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 23, 2017

Member

This (an other value for OnOffType then On or OFF) is (currently) not possible and is over defensive.

If you want to be so defensive you can use:

if (value instanceof OnOffType) {
    if (value == OnOffType.OFF) {
        return "0";
    } else if (value == OnOffType.ON) {
        return "1";
    }
}
throw new IllegalArgumentException("Passed command: " + value + " is not a valid OnOff command");
* @return BridgeHandler of the bridge/gateway to the MySensors network
*/
private synchronized MySensorsBridgeHandler getBridgeHandler() {
MySensorsBridgeHandler myBridgeHandler = null;

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 23, 2017

Member

this initialization is not needed


private String getChannelNameFromVar(MySensorsVariable var) {
// Cover thing has a specific behavior
if (getThing().getThingTypeUID().equals(MySensorsBindingConstants.THING_TYPE_COVER)) {

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 23, 2017

Member

Is it equal to or the same instance? => ==

MySensorsNode ret = null;
Integer nodeId = -1, childId = -1, pres = -1;
try {
nodeId = Integer.parseInt(t.getConfiguration().as(MySensorsSensorConfiguration.class).nodeId);

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 23, 2017

Member

we have a configuration as parameter, we have a configuration in this and we get the configuration from t what is going on here?

try {
nodeId = Integer.parseInt(t.getConfiguration().as(MySensorsSensorConfiguration.class).nodeId);
childId = Integer.parseInt(t.getConfiguration().as(MySensorsSensorConfiguration.class).childId);
pres = INVERSE_THING_UID_MAP.get(t.getThingTypeUID());

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 23, 2017

Member

what is a pres?

import org.openhab.binding.mysensors.internal.exception.MergeException;

/**
* Indicates that a class could be merged to another one of the same type

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 23, 2017

Member

The goal of the interface is still really vague to me.

@martinvw

This comment has been minimized.

Copy link
Member

commented Mar 24, 2017

I see a lot of files from other people please mention them using the also-by, see: http://docs.openhab.org/developers/contributing/contributing.html#sign-your-work

@martinvw

This comment has been minimized.

Copy link
Member

commented Mar 24, 2017

I think its common (or maybe even required) to name the contributions done by the authors in the files, this not done at this moment maybe another reviewer can say something about the rules / guidelines

@martinvw
Copy link
Member

left a comment

Hello, what a great effort it must have been to write this. Thanks for the though work.

I added a lot of remarks most of them are to improve the code readability, the maintainability and limit the chances of bugs appear.

* @return the new inverted map
* @throws NullPointerException if map is null
*/
public static <V, K> Map<V, K> invertMap(Map<K, V> map, boolean hasDuplicate) throws NullPointerException {

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 24, 2017

Member

NullPointerException is a RuntimeException and should not be mentioned like this IMHO

* @param map1
* @param map2
* @return the new map that contains the entry of map1 and map2
* @throws NullPointerException

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 24, 2017

Member

See other comment about NPE

* @throws MergeException if allowOverwrite is true and a duplicate is found for a key
*/
public static <K, V> void mergeMap(Map<K, V> map1, Map<K, V> map2, boolean allowOverwrite)
throws MergeException, NullPointerException {

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 24, 2017

Member

See remark about NPE and method throws an IllegalArgumentException instead of a MergeException.

IMHO the MergeException sounds more appropriate


}

public static <K, V> boolean containsSameKey(Map<K, V> map1, Map<K, V> map2) throws NullPointerException {

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 24, 2017

Member

Maybe rename it containsSameKeys I expect something else in the implementation

*/
public class EventRegister<T extends EventListener> implements Register<T> {

private Logger logger = LoggerFactory.getLogger(getClass());

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 24, 2017

Member

if this class will be extended it logs the child class instead making it harder to trace back where the actual logging came from, imho this not handy

MySensorsVariable v = new MySensorsVariableVCustom();

v.setValue("1");
String s = v.getValue();

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 24, 2017

Member

I assume that s = "1", but this complete test will still pass if the implementation of v.getValue is return "0"

So its not really strong


serialPort=$1

echo "0;0;3;0;2;0" > $serialPort

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 24, 2017

Member

Unexpected test for me... should this be checked in, if so what would the proper directory be.


serialPort=$1

# socat -d -d pty,raw,echo=0 pty,raw,echo=0

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 24, 2017

Member

a lot of commented code, not sure if this should be checked in.

*/
package org.openhab.binding.mysensors.handler;

import static org.openhab.binding.mysensors.MySensorsBindingConstants.*;

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 24, 2017

Member

I remember that these (star-imports) were not supposed to be used, but I am not sure.

This comment has been minimized.

Copy link
@martinvw

martinvw Apr 23, 2017

Member

@kaikreuzer should we or should we not use * imports?

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Apr 24, 2017

Member

For static imports I do not care too much. But afaik, the Eclipse code formatter anyhow replaces them by individual imports...

Map<Integer, MySensorsNode> nodes = new HashMap<Integer, MySensorsNode>();

List<Integer> givenIds = cacheFactory.readCache(MySensorsCacheFactory.GIVEN_IDS_CACHE_FILE,
new ArrayList<Integer>(), new TypeToken<ArrayList<Integer>>() {

This comment has been minimized.

Copy link
@martinvw

martinvw Mar 24, 2017

Member

Found the meaning of this construct online, I would never use it like this. But okay.

@martinvw

This comment has been minimized.

Copy link
Member

commented May 24, 2017

@tobof I think that you made a great contribution with this binding. It would also be nice to integrate it into the master. Is there anything we can do to help you address the review comments so we in the end merge your contribution?

If you request it maybe others want to pick up and make a pull-request on your repo with the changes?

@tobof

This comment has been minimized.

Copy link
Author

commented May 24, 2017

@martinvw Thank you for the offer and especially thank you very much for your amazing feedback!! I'm working on it and I'm half way done. I've pushed the changes to a separate branch, so this may be the reason you don't see any progress here. :-)

@tobof tobof force-pushed the tobof:MySensors_Binding branch 2 times, most recently from 1791a10 to 27f6f42 Jul 12, 2017

@tobof

This comment has been minimized.

Copy link
Author

commented Jul 14, 2017

Okay, I'm done.

@martinvw Please take a look at the exceptions. I'm not that sure about checked and unchecked exceptions, I may need some additional explanation.

@kaikreuzer Could you please take a look at the thread management? That point is still unchecked after @martinvw review.

@tobof tobof force-pushed the tobof:MySensors_Binding branch from 27f6f42 to 5a28ab5 Jul 14, 2017

@tobof tobof force-pushed the tobof:MySensors_Binding branch from 5a28ab5 to 5fd8fec Jul 18, 2017

@martinvw
Copy link
Member

left a comment

I added some more small issues, I will also queue you up for a review by @kaikreuzer, the owner of this project.

<name>MySensors.test Binding</name>
<packaging>eclipse-plugin</packaging>

<profiles>

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 18, 2017

Member

Is this really needed here?

This comment has been minimized.

Copy link
@tobof

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 24, 2017

Member

It is also included in the parent Pom.xml

import org.junit.Test;
import org.openhab.binding.mysensors.internal.protocol.message.MySensorsMessageSubType;

public class DiscoveryServiceTest {

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 18, 2017

Member

Every class should have a description and author. Check all.

@@ -0,0 +1 @@
/*.cached

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 18, 2017

Member

Please remove this local .gitignore file, the cache should not be stored here anymore

<parameter name="ipAddress" type="text" required="true">
<label>IP Address</label>
<description>The IP Address the MySensors Gateway uses
</description>

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 18, 2017

Member

Please add:

<context>network-address</context>
sleep 3

# I_HEARTBEAT_RESPONSE
echo "104;255;3;0;22;4999" > $serialPort

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 18, 2017

Member

Missing newline

/**
* Mapping MySensors message type/subtypes to channels.
*/
public static final Map<MySensorsMessageSubType, String> CHANNEL_MAP = new HashMap<MySensorsMessageSubType, String>() {

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 18, 2017

Member

Diamond operator, ie leave out the second pair of generics and replace it by <>

This comment has been minimized.

Copy link
@tobof

tobof Jul 24, 2017

Author

If I remove them, eclipse complains about:
"'<>' cannot be used with anonymous classes"

What am I missing here?

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 24, 2017

Member

Nothing, I do not use anonymous inner classes for such initialisations so I was not yet aware that you could not use with the diamond operator if eclipse or the Jdk says so, I believe him.

*/
public static final Map<String, MySensorsTypeConverter> TYPE_MAP = new HashMap<String, MySensorsTypeConverter>() {

/**

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 18, 2017

Member

Leave out this empty docblock, check all in this file

/**
* Mappings between ChannelUID and class that represents the type of the channel
*/
public static final Map<String, MySensorsTypeConverter> TYPE_MAP = new HashMap<String, MySensorsTypeConverter>() {

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 18, 2017

Member

Diamond operator, check all in this file

private final Gson gson = new Gson();

public MySensorsCacheFactory(String userDataFolder) {
CACHE_BASE_PATH = userDataFolder + "/mysensors/cache";

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 18, 2017

Member

This is a field now, please follow coding conventions and make the name small and camelcase

@@ -52,6 +52,8 @@
<module>org.openhab.binding.mihome</module>
<module>org.openhab.binding.milight</module>
<module>org.openhab.binding.minecraft</module>
<module>org.openhab.binding.mysensors</module>
<module>org.openhab.binding.mysensors.test</module>

This comment has been minimized.

Copy link
@martinvw

martinvw Jul 18, 2017

Member

To have the binding being picked up by the distro, you furthermore need to add it to the feature.xml, again at the alphabetically correct position. If you have a dependency on some transport bundle (e.g. upnp, mdns or serial), make sure to add a line for this dependency as well (see the other bindings as an example).

This comment has been minimized.

Copy link
@martinvw

martinvw Apr 4, 2018

Member

@tobof this point seems still to be open

This comment has been minimized.

Copy link
@wmarkow

This comment has been minimized.

Copy link
@martinvw

martinvw Jun 22, 2018

Member

Correct

@martinvw martinvw requested a review from kaikreuzer Jul 18, 2017

@tobof tobof force-pushed the tobof:MySensors_Binding branch 2 times, most recently from bdfc47f to 43aacb6 Jul 20, 2017

@openhab-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2017

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/mysensors-binding-strange-behaviour/35472/5

@martinvw

This comment has been minimized.

Copy link
Member

commented May 29, 2018

@tobof what is the status of this PR, I hope that we can include it in the next release. But there are some changes required, for that, at this moment it does not build anymore because it still refers to 2.2 while we just switched to 2.4. I also don't know whether your copyrights are still up-to-date but it would be great if you could also handle that. If the build is green I suppose @kaikreuzer would be able to review it for final review in the upcoming month(s).

If you have any questions please let us know.

@openhab-bot

This comment has been minimized.

Copy link
Collaborator

commented Jun 16, 2018

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/mysensors-binding-disappeared-after-upgrade-to-2-3/46497/2

@martinvw

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

@tobof what is the status of this PR it seems to be in a limbo state, it is still missing from the feature.xml and the build is broken. If you can fix this I hope that @kaikreuzer will find time to review this soon. @kaikreuzer its not as bad as it looks you don't have to review all those 15000 LoC... ;-)

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

Yes, I have it on my list - getting the build green first would be appreciated, though :-)

@FredericMa

This comment has been minimized.

Copy link

commented Sep 25, 2018

@andreacioni do you maybe have an idea how we can go on with the MySensors binding?

@andreacioni

This comment has been minimized.

Copy link

commented Sep 25, 2018

Hi @FredericMa, I'm wondering the same thing. I don't know if Tim is working on new features and want to finish them before making another PR. Personally I can try to find some time to spend on making a new PR from a fork of Tim repo, but I'd like to discuss it with him.
@tobof what do you think?

@tobof

This comment has been minimized.

Copy link
Author

commented Sep 28, 2018

There are still some things to do for me, before the PR is ready for a review @kaikreuzer .
The hardest part for me is cleaning up the git history. I'll give it another try.

@andreacioni If you could find the time to look at one or two open issues in the list, this would really help me out.

Refactoring of the MySensors binding (#74)
* Initial commit of the refactored binding
* Changes to apply OpenHAB coding guidelines
* Lock Status: Fixed wrong item-type.
* Fixes AutoDiscovery bug aufter thing removal (Fixes tobof#39)
* RGB & RGBW support
* Serial fix to support more than one binding with usb access

Signed-off-by: Tim Oberföll <oberfoell@web.de>

@tobof tobof force-pushed the tobof:MySensors_Binding branch from ea89434 to 8d9f28a Dec 13, 2018

andreacioni and others added some commits Oct 12, 2017

NPE fix inside NetworkSanityChecker (#99)
* fixing heartbeat checking in NetworkSanityChecker

* nodeId parameter missing in logger

Signed-off-by: Andrea Cioni <supercionci@hotmail.it>
Changed the way we condiser a message as a request for ID since child…
… ID is now random

fixed tobof/openhab2-addons/#94

Signed-off-by: Net Lemarinel <l.lemarinel@gmail.com>
Redesign of the ESH-INF Folder & Added var1 to var5; bugfixes (#101)
* redesign of the ESH-INF Folder
* separate file (channel-types.xml) for all channels
* separate file (gateway-types.xml) for the gateways
* separate file for every child (example: SBinary.xml, STemp.xml ..)
* added var1 to var5 to every sensor
* addition of some sensors (gps, sceneController, moisture, sprinkler,
vibration)
* bug fixing (Fixed tobof#65 & Fixed tobof#95)
* Adds support for MySensors MQTT gateway (Fixes tobof#98)
* Serial refactoring (#105) (Fixes tobof#103 & Fixes tobof#86)
* Bugs in Expert Mode (MySensorsMessage) removed
* Added V_PERCENTAGE (Dimmer) to S_RGB & S_RGBW (#106) (Fixes tobof#87)
* Fixed reverse (wrong) description for option networkSanCheckEnabled (Fixes tobof#90)
* Bug: (RGB/W) ON / OFF sent instead of 0 & 1 (Fixes tobof#108)
* Remove warning: "Overwrite variable: V_VAR1"

Signed-off-by: Tim Oberföll <oberfoell@web.de>
RGBW pure white added
added new setting that makes the rgbw type converter convert a pure
white color to a hex value without rgb part

Signed-off-by: Oliver Hilsky <hilsky@campus.tu-berlin.de> (github: OliverHi)

@tobof tobof force-pushed the tobof:MySensors_Binding branch from 8d9f28a to f71625c Dec 13, 2018

MQTT API changes and bug fixes
* Bugs in Expert Mode (MySensorsMessage) removed
* License header changed
* Added org.eclipse.jdt.annotation;resolution:=optional
* Changes to comply with the changes in the API in MQTT (ESH) (API changes: https://github.com/eclipse/smarthome/pull/4173/files)
* Move to 2.4
* Adopted MQTT API changes (API changes: eclipse/smarthome#5535)
* Fixes smartsleep function in binding
* Sending of smartsleep messages is now activated through I_PRE_SLEEP_NOTIFICATION (Fixes tobof#121)
* I_TIME response now depends on the default / local timezone instead of UTC (Fixes tobof#125)

Signed-off-by: Tim Oberföll <oberfoell@web.de>

@tobof tobof force-pushed the tobof:MySensors_Binding branch from f71625c to a2329c6 Dec 13, 2018

@openhab-bot

This comment has been minimized.

Copy link
Collaborator

commented Dec 14, 2018

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/oh2-4-0-m-milestone-testing-summary/59035/27

@jgantenberg

This comment has been minimized.

Copy link

commented Dec 14, 2018

I have a severe problem with actual version of MySensors binding in OH 2.4 M8 environment. Everything can be installed and for a short while everything is fine. Sensors are online and deliver values and also actions can be fired. After a while which is from minutes to an hour whole OP is stalled. No more event handling. The web interface is still alive but delivers old values.
When I leave MySensors binding out the problem seems to be gone.
Difficult to analyze as there are no hints in the logs. Really nothing. Maybe an lock situation.

@wborn wborn changed the title MySensors binding [mysensors] MySensors Binding initial contribution Dec 18, 2018

@tobof

This comment has been minimized.

Copy link
Author

commented Dec 19, 2018

@jgantenberg I've not done a long term test yet. Thats remains open. Is the MySensors binding the only affected binding or do other bindings also stop event handling?

@jgantenberg

This comment has been minimized.

Copy link

commented Dec 19, 2018

@tobof during upgrade from 2.3 to 2.4 I had to leave out enigma2 binding as I don't find it at the moment. Besides from that I think it is only MySensors binding causing stall in the event engine. I left out MySensors binding but switch all things to plain mqtt. Now everything works with no problem.

@tobof tobof requested a review from openhab/2-x-add-ons-maintainers as a code owner Feb 5, 2019

Switch to SNAPSHOT 2.5 & license header adjustment
Signed-off-by: Tim <oberfoell@web.de>

@tobof tobof force-pushed the tobof:MySensors_Binding branch from aaf8184 to 64b43e7 Feb 5, 2019

@davidgraeff davidgraeff added the stale label Feb 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.