MqttService: Remove all deprecated methods, rename close to stop. #4173

davidgraeff · 2017-09-01T09:06:42Z

This PR prepares the io.transport.mqtt bundle for the discussed MQTT architecture.

Remove name from MqttBrokerConnection. This is only necessary for the MqttService.
Consumers of MqttBrokerConnection should not be bothered with this detail.
The MqttService requires a brokerID for addConnection() and removeConnection() and the observer interface adds the brokerID next to the connection as well, to compensate for the removed name.
Remove textualConfigured flag. Again this is not interesting for anything outside MqttService.
Don't parse configuration parameters ourselves, but use a configuration values class instead.
Use Nullable where it makes sense.
The code was modernized
ESH compatible implementation of the service factory pattern including a config description for GUIs
Add MqttService.setLastWill method. Fixes Unable to use MQTT Last Will and Testament with dynamic data #5162

API break:

All deprecated methods are removed.
MqttBrokerConnection.close -> stop
MqttBrokerConnection.getURL() -> getHost(), getPort(), isSecure().
MqttBrokerConnection constructor changed

Config interface break:

There is no "url" anymore, it is now "host"+"port"+"secure"
"retain" is now "retainMessages"
"user" is now "username"
"pass" is now "password"
"lw" is now "lwtTopic"+"lwtMessage"+"lwtQos"+"lwtRetain"
"clientid" is now "clientID"
Uses OSGI service factories.

Documentation

Add more public class documentation especially for MqttService and its purpose.

Fixes #4170

Signed-off-by: David Gräff david.graeff@web.de

kaikreuzer · 2017-09-01T12:30:26Z

Oops, merge conflicts...

maggu2810

Why is it necessary that the host, port, secure flag for a broker connection to be changed after construction?
Wouldn't it be simpler to assign a host and port on construction and create another broker connection object if the host or port should be changed?
Immutable members are thread safe.
What is the behavior if host is changed while connected, what is a reconnect is done etc.
Isn't it common practice that such stuff is only set on construction (e.g. similar to the Java socket)?
Same for the other stuff, too.
If connection parameters that are used on connection establishment only could be changed all the time... it doesn't make the implementation and knowledge what is applied at which done easier.

davidgraeff · 2017-09-04T02:02:24Z

I agree. The changes would be:

Make host, port, secure, clientID final
The port parameter could also be of type "@nullable Integer" to distinguish between a not set port and a given port number.
The clientID must be set in the constructor. Null is allowed, an ID will be generated in that case.

Everything else (qos, reconnect time etc) aren't connection parameters but take effect immediately for all following published messages.

davidgraeff · 2017-09-06T04:59:32Z

I changed the PR accordingly. I'm just not sure about username and password. Those are technically speaking not connection parameters and can change between stop/start cycles and are optional.

maggu2810 · 2017-09-06T08:35:19Z

...ome.io.transport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttService.java

-            throw new ConfigurationException("MQTT Broker property 'name' is not provided");
-        }
-        brokerID = brokerID.toLowerCase();
+    public synchronized MqttBrokerConnection addBrokerConnection(@NonNull String brokerID,


@kaikreuzer Do I remember correctly that you allowed @Nullable on return types to enable checks for the consumer code?
If it is allowed I would prefer to add it here.

Hm, Nullable is the java standard. I agree that it sometimes makes sense to add it, as a way to document parameters, for instance Integer: A consumer might feel like he always needs to provide a value, even if null would be absolutely acceptable.

But for return types I would say Nullable is not reasonable.

Please read #4080

Nah a twenty page long discussion doesn't help :) A link or quote of the developer guidelines would convince me of course :D

There isn't a developer guidelines about the usage of nullness annotation. At least we didn't fully agree.

At least everything that has been agreed on, should be put in the developer guidelines. Should I open an issue?

I don't think so. I have added a summary here:
https://github.com/eclipse/smarthome/wiki/Type-annotations

maggu2810 · 2017-09-06T08:35:51Z

...ome.io.transport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttService.java

-        synchronized (brokerConnections) {
-            return brokerConnections.get(brokerName.toLowerCase());
-        }
+    public synchronized MqttBrokerConnection getBrokerConnection(String brokerName) {


See other comment about Nullable on return type.

kaikreuzer · 2017-09-21T17:30:05Z

@maggu2810 Was this PR now ok for you?
If so, what might be left is to adapt the use of null annotations to our new guideline (i.e. using @NonNullByDefault on the class level.

maggu2810 · 2017-09-21T19:51:32Z

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

-        return url;
+    @Nullable
+    public Integer getPort() {
+        return port;


On the constructor you state.

* @param port A port or null to select the default port for a secure or insecure connection

So I would assume that a port is choosen now and we could return the explicit set one, the insecure or secure default.
I am pretty sure that a connection uses a port ;)

maggu2810 · 2017-09-21T19:55:41Z

@maggu2810 Was this PR now ok for you?

If the MQTT transport and reconnect is still working (using the non-deprecated methods), all is okay for me. 😉
Didn't test it myself.

maggu2810 · 2017-09-22T06:19:29Z

Parts of the nullness fixes could be taken from here: ae1a8ac

davidgraeff · 2017-09-23T06:09:55Z

I adapted the PR including the Nullness fixes.
I removed the custom nested configuration parser, as we agreed in Support hierarchical configuration & exclusive cfg files #4178, that a central place to parse nested configuration is a good idea, and I expect the "Configuration" class to handle the details for me.

At the moment that means, this service will not be configurable without changes to the "Configuration" class.

davidgraeff · 2018-03-15T13:34:05Z

Because my other Mqtt PRs are based on the API changes, introduced here, I will resurrect this PR. I will also adapt it to the service multi-config concept.

htreu · 2018-03-16T10:14:45Z

@davidgraeff ping me when ready for review.

davidgraeff · 2018-03-16T10:24:55Z

@htreu Code-wise it is complete. But travis failed because of another issue, therefore I do not know if if will pass the static code check yet.

Could you have a look at the MqttBrokerConnectionServiceInstance file, if I'm using the service-factory in a right way?
https://github.com/eclipse/smarthome/pull/4173/files#diff-9365221333b769c30777cfc2914e2001R39

htreu · 2018-03-16T12:05:19Z

Travis seems to be unrelated.

For the MqttBrokerConnectionServiceInstance please have a look at the MagicMultiInstanceService and the MagicMultiInstanceServiceMarker classes.

MagicMultiInstanceService is the implementation of the actual service which can be configured multiple times.
MagicMultiInstanceServiceMarker is a marker class which mainly carries the relevant information about the service to Paper UI.

The ConfigurableService.SERVICE_PROPERTY_DESCRIPTION_URI + "=test:multipleMagic" refers to the ESH-INF/config/multipleConfig.xml file where the configuration for the service is described.

Once you have everything setup correctly Paper UI should allow you to add/edit/remove instances of your service.

davidgraeff · 2018-03-17T20:53:28Z

@htreu If have adjusted the PR. Could you have a look?

htreu

Hi @davidgraeff good to see this progressing. Please find my comments inline.

htreu · 2018-03-19T09:46:38Z

...t.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnectionConfig.java

+    public @Nullable String password;
+    public @Nullable String clientID;
+    // MQTT parameters
+    public BigDecimal qos = BigDecimal.valueOf(MqttBrokerConnection.DEFAULT_QOS);


When configuring a new service instance from Paper UI this fails with:

10:36:02.939 WARN o.e.s.c.core.Configuration[:108] - Could not set field value for field 'qos': Can not set java.math.BigDecimal field org.eclipse.smarthome.io.transport.mqtt.MqttBrokerConnectionConfig.qos to java.lang.String java.lang.IllegalArgumentException: Can not set java.math.BigDecimal field org.eclipse.smarthome.io.transport.mqtt.MqttBrokerConnectionConfig.qos to java.lang.String at sun.reflect.UnsafeFieldAccessorImpl.throwSetIllegalArgumentException(UnsafeFieldAccessorImpl.java:167) at sun.reflect.UnsafeFieldAccessorImpl.throwSetIllegalArgumentException(UnsafeFieldAccessorImpl.java:171) at sun.reflect.UnsafeObjectFieldAccessorImpl.set(UnsafeObjectFieldAccessorImpl.java:81) at java.lang.reflect.Field.set(Field.java:764) at org.apache.commons.lang.reflect.FieldUtils.writeField(FieldUtils.java:523) at org.apache.commons.lang.reflect.FieldUtils.writeField(FieldUtils.java:500) at org.apache.commons.lang.reflect.FieldUtils.writeField(FieldUtils.java:560) at org.eclipse.smarthome.config.core.Configuration.as(Configuration.java:105) at org.eclipse.smarthome.io.transport.mqtt.internal.MqttBrokerConnectionServiceInstance.modified(MqttBrokerConnectionServiceInstance.java:76) at org.eclipse.smarthome.io.transport.mqtt.internal.MqttBrokerConnectionServiceInstance.activate(MqttBrokerConnectionServiceInstance.java:107) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

I'm not aware of the configuration conversion logic, but maybe Integer will do?

htreu · 2018-03-19T09:46:55Z

...t.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnectionConfig.java

+    public Boolean retainMessages = false;
+    public @Nullable String lastWill;
+    /** Keepalive in seconds */
+    public @Nullable BigDecimal keepAlive;


same here as for qos.

htreu · 2018-03-19T09:48:21Z

...ome.io.transport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttService.java

- * http://www.eclipse.org/legal/epl-2.0
- *
- * SPDX-License-Identifier: EPL-2.0
+ * Copyright (c) 2014-2017 by the respective copyright holders.


just run mvn license:format on your sources again.

Unfortunately I do have to correct license headers myself. mvn license:format only corrects xml headers for me after 10min of downloading various files (it is time for gradle!). Therefore, If you see another spot, please let the know.

htreu · 2018-03-19T09:57:11Z

...va/org/eclipse/smarthome/io/transport/mqtt/internal/MqttBrokerConnectionServiceInstance.java

+                logger.warn("Ignore invalid broker connection configuration: {}", config);
+                return;
+            }
+            assert brokerID != null;


This should already be handled by StringUtils.isBlank(brokerID)

Correct, but at least my eclipse will issue a warning. I'll investigate.

The static analyser, used by eclipse, is not recognizing the result of StringUtils.isBlank. The assert is unfortunately necessary here.

Please remove it and ignore the warning. Or add an extra null check to the StringUtils check.

htreu · 2018-03-19T09:58:14Z

...va/org/eclipse/smarthome/io/transport/mqtt/internal/MqttBrokerConnectionServiceInstance.java

+            assert brokerID != null;
+
+            // Add connection and make sure it succeeded
+            assert mqttService != null;


This is already checked by line 71 (null check against config and mqttservice).

True, but eclipse is issuing a warning. I guess that's because mqttService is a field variable and the static analyser doesn't guarantee that no concurrent thread or side effect of a method call changes the field. I would leave it for now.

Please remove and ignore/refactor

htreu · 2018-03-19T09:58:51Z

.../eclipse/smarthome/io/transport/mqtt/internal/MqttBrokerConnectionServiceInstanceMarker.java

+ * @author David Graeff - Initial contribution
+ */
+@Component(immediate = true, service = MqttBrokerConnectionServiceInstanceMarker.class, property = {
+        Constants.SERVICE_PID + "=org.eclipse.smarthome.magicMultiInstance",


copy&paste error?

htreu · 2018-03-19T10:01:28Z

...ome.io.transport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttService.java

-        public final @Nullable String lwtTopic;
-        public final byte @Nullable [] lwtMessage;
-        public final @Nullable Integer lwtQos;
-        public final @Nullable Boolean lwtRetain;


Please review the latest changes @maggu2810 did on the LWT configuration. It is now possible to either pass the encoded String with topic:message:qos:retained or provide all values in separate fields (see #5231).

htreu · 2018-03-19T10:02:57Z

bundles/io/org.eclipse.smarthome.io.transport.mqtt/ESH-INF/config/brokerConnectionInstance.xml

+            <description>Hostname or IP of the broker</description>
+            <default></default>
+        </parameter>
+        <parameter name="host" type="integer" required="false">


name should be host

htreu · 2018-03-19T10:04:28Z

...t.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnectionConfig.java

+
+    /**
+     * Return the brokerID of this connection. This is either the name or host:port(:s).
+     * This method might return null, if none of the parameters is set.


The method does actually not return null but the empty String is nothing is configured.

davidgraeff · 2018-03-19T10:59:07Z

@htreu Issues addressed

htreu

Thanks!
Some more comment inline.

Also: Again I get the following exception:

java.lang.IllegalArgumentException: Can not set java.lang.Integer field org.eclipse.smarthome.io.transport.mqtt.MqttBrokerConnectionConfig.lwtQos to java.lang.String

htreu · 2018-03-19T15:34:34Z

bundles/io/org.eclipse.smarthome.io.transport.mqtt/ESH-INF/config/brokerConnectionInstance.xml

+            <advanced>true</advanced>
+        </parameter-group>
+
+		<parameter name="name" type="text" required="false" groupName="group_connection">


please run auto formatter once.

htreu · 2018-03-19T15:35:19Z

bundles/io/org.eclipse.smarthome.io.transport.mqtt/ESH-INF/config/brokerConnectionInstance.xml

+            <default>false</default>
+        </parameter>
+
+


htreu · 2018-03-19T15:51:36Z

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

-                        e.getMessage(), e.getReasonCode(),
-                        (e.getCause() == null ? "Unknown" : e.getCause().getMessage()));
+        public synchronized void connectionLost(@Nullable Throwable exception) {
+            assert exception != null;


exception being null seems to be a legit case. Its way too hard to throw errors here.
We do not use assert in ESH at all: Assertions need to be switched on during compilation what we don't do. Thus they will be removed from the resulting class files. And in this particular case it is hiding a NPE when exception actually is null.
Please review other occurrences and replace them by throwing IllegalArgumentException or IllegalStateException.

I checked with the documentation. All parameters on this interface are meant to be non null. The annotations are just missing and assert is basically used to compensate. I have added some comments. I do know that assert's are not compiled into bytecode in our case, it is more of a compiler and IDE hint.

Assertions need to be switched on during compilation what we don't do.

That's not correct.
assertions could be enabled at runtime to use additional checks (e.g. in the development phase) that are not enabled default

Ah I see (http://etutorials.org/Programming/Java+performance+tuning/Chapter+6.+Exceptions+Assertions+Casts+and+Variables/6.2+Assertions/). So it actually imposes a runtime overhead in contrast to C/C++. I'll use local variables then to get rid of the warnings.

htreu · 2018-03-19T15:52:21Z

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

+        this.secure = secure;
+        if (clientId == null) {
+            clientId = MqttClient.generateClientId();
+            assert clientId != null;


please remove or refactor

Some asserts can not be removed in my eclipse setup, which was done by oomph and the official installer. I get errors like "Null type mismatch (type annotations): required '@nonnull String' but this expression has type '@nullable String'. The problem severity defaults to "error" for those null checks.

htreu · 2018-03-19T15:55:22Z

...va/org/eclipse/smarthome/io/transport/mqtt/internal/MqttBrokerConnectionServiceInstance.java

+                logger.warn("Ignore invalid broker connection configuration: {}", config);
+                return;
+            }
+            assert brokerID != null;


Please remove it and ignore the warning. Or add an extra null check to the StringUtils check.

htreu · 2018-03-19T15:56:06Z

...va/org/eclipse/smarthome/io/transport/mqtt/internal/MqttBrokerConnectionServiceInstance.java

+            assert brokerID != null;
+
+            // Add connection and make sure it succeeded
+            assert mqttService != null;


Please remove and ignore/refactor

htreu · 2018-03-19T15:56:16Z

...va/org/eclipse/smarthome/io/transport/mqtt/internal/MqttBrokerConnectionServiceInstance.java

+                return;
+            }
+            // Start connection
+            assert connection != null;


Please remove and ignore/refactor

davidgraeff · 2018-03-19T16:30:56Z

I have no idea, why you get "java.lang.IllegalArgumentException: Can not set java.lang.Integer field org.eclipse.smarthome.io.transport.mqtt.MqttBrokerConnectionConfig.lwtQos to java.lang.String" to be honest. The conf desc xml says <parameter name="lwtQos" type="integer" ...> and it is defined as public Integer lwtQos in MqttBrokerConnectionConfig.

htreu · 2018-03-20T07:20:55Z

I don't understand the exception either. Does it work for you?
Also: This time Travis really hangs at MQTT transport with

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:1.1.0:compile (default-compile) on project org.eclipse.smarthome.io.transport.mqtt: Compilation failure: Compilation failure: 
[ERROR] /home/travis/build/eclipse/smarthome/bundles/io/org.eclipse.smarthome.io.transport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttService.java:[47] 
[ERROR] 	@Component(immediate = true, service = { MqttService.class }, configurationPid = {
[ERROR]         "org.eclipse.smarthome.mqtt" }, property = "service.pid=org.eclipse.smarthome.mqtt")
[ERROR] 	                                                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[ERROR] Type mismatch: cannot convert from String[] to String

maggu2810 · 2018-03-20T07:58:51Z

See: #4625

davidgraeff · 2018-03-20T08:45:54Z

The reason for the IllegalArgumentException is this:
Some time ago numbers in service configuration maps were dispatched as BigDecimal objects. Now they are Strings instead. We use config/core/Configuration for parsing the map and it fails on line 89ff, because it still expects BigDecimals:

if (value != null && value instanceof BigDecimal && !typeName.equals("BigDecimal")) {
       BigDecimal bdValue = (BigDecimal) value;
     if (typeName.equalsIgnoreCase("Float")) {
         value = bdValue.floatValue();
     } else if (typeName.equalsIgnoreCase("Double")) {
         value = bdValue.doubleValue();
     } else if (typeName.equalsIgnoreCase("Long")) {
         value = bdValue.longValue();
     } else if (typeName.equalsIgnoreCase("Integer") || typeName.equalsIgnoreCase("int")) {
         value = bdValue.intValue();
     }
}

Configuration need to be changed to also accept Strings

davidgraeff · 2018-03-20T09:02:05Z

I have addressed the Configuration class shortcoming here: #5262

htreu

There are some cosmetics left. And the question, if lwt should also be configurable by a single line as in the past (see inline comment).

htreu · 2018-03-27T09:12:57Z

bundles/io/org.eclipse.smarthome.io.transport.mqtt/ESH-INF/config/brokerConnectionInstance.xml

+			<description>Broker username</description>
+			<default />
+		</parameter>
+		<parameter name="password" type="text" required="false" groupName="group_connection">


Please give this parameter the context password.

htreu · 2018-03-27T09:14:45Z

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

        }

        @Override
-        public void deliveryComplete(IMqttDeliveryToken token) {
+        public void deliveryComplete(@Nullable IMqttDeliveryToken token_) {
+            IMqttDeliveryToken token = (@NonNull IMqttDeliveryToken) token_;


this looks like an accident waiting to happen :-) Please simply null-check here.

htreu · 2018-03-27T09:19:57Z

...ansport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnection.java

-                final List<MqttMessageSubscriber> consumerList = entry.getValue();
-
+        public void messageArrived(@Nullable String topic_, @Nullable MqttMessage message_) {
+            byte[] payload = ((@NonNull MqttMessage) message_).getPayload();


Okay, I get what you are doing here. Instead of defining the methods args @Nullable and do the cast here you may add @NonNullByDefault({}) at ClientCallbacks class level. This will "switch off" null annotation checks in this class. It hurts the eyes a little less then casting @Nullabel to @NonNull.

htreu · 2018-03-27T09:26:41Z

...t.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttBrokerConnectionConfig.java

+    /** Keepalive in seconds */
+    public @Nullable Integer keepAlive;
+    // Last will parameters
+    public @Nullable String lwtTopic;


Now the "old" configuration of lwt is not supported anymore. With #5231 @maggu2810 made sure both options could be used.
I thinks its fine that the UI (config description) only provides the single fields. But text based configuration may depend on the single line config. wdyt?

This PR is a API brake, code-wise as well as configuration-wise. Remember that we used the dot syntax before to express multiple mqtt instances and we use osgi service factories with a config file each now. Any user of this service (which I doubt there are any, because we provide no means to use Mqtt really) need to migrate anyway ^^.

@maggu2810 fine for you?

It is okay for me to drop the old way to configure the LWT.

I am pretty sure there are guys the use this service. There has been one that inspired me to implement that change (to support a more generic topic and payload) and I am using that service, too. Already two of them. 😉 But as @davidgraeff also pointed out, changes in its usage needs to be done regardless if we keep this field or not, so I would prefer to remove old stuff and stop maintaining it on this change and all the other ones that will come some time.

davidgraeff · 2018-03-27T14:07:16Z

@htreu Done

htreu · 2018-03-28T09:04:51Z

Strange: Travis complains about

[ERROR] /home/travis/build/eclipse/smarthome/bundles/io/org.eclipse.smarthome.io.transport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttService.java:[48] 
[ERROR] 	@Component(immediate = true, service = { MqttService.class }, configurationPid = {
[ERROR]         "org.eclipse.smarthome.mqtt" }, property = { Constants.SERVICE_PID + "=org.eclipse.smarthome.mqtt" })
[ERROR] 	                                                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[ERROR] Type mismatch: cannot convert from String[] to String

~~Locally it does compile fine.~~ Update: No, it does not :-)

davidgraeff · 2018-03-28T09:54:01Z

Hm, but why. A fast lookup on google let me find this example: @Component(property={EventConstants.EVENT_TOPIC+"=osgi/enroute/*"}). This is what we do here.

And another hm. It does of course compile... in eclipse. That's the main reason btw why google migrated for Android from eclipe to IDEA+gradle, to always use one build system.

htreu · 2018-03-28T11:05:33Z

My guess is, that configurationPid is the parameter in question (although it is in fact defined as String[] in @Component). I have no clue why we see this error.
Please give it a try.

maggu2810 · 2018-03-28T11:40:43Z

That artifact is used for the annotations on a Maven build (see pom.xml):

    <dependency>
      <groupId>org.apache.felix</groupId>
      <artifactId>org.apache.felix.scr.ds-annotations</artifactId>
      <version>1.2.10</version>
    </dependency>

This artifact itself uses the org.osgi.service.component.annotations package of

    <dependency>
      <groupId>org.osgi</groupId>
      <artifactId>org.osgi.compendium</artifactId>
      <version>4.3.0</version>
    </dependency>

Have a look at Release 4 Version 4.3 - Annotation Type Component: configurationPid needs to be a String, not an array.
That differs from Release 6 - Annotation Type Component: configurationPid is a String[] (and settings one value for an array with one element is supported in annotations AFAIK)

htreu · 2018-03-28T12:17:00Z

Thanks @maggu2810. I always searched our org.eclipse.osgi.services-3.6.0 dependency where String[] is defined. Where do we define the OSGi DS Annotation version to be used? Or: who is right? the maven dependency or the IDE dependency?

and settings one value for an array with one element is supported in annotations AFAIK

Yes, I just observed that in the annotation processor sources.

maggu2810 · 2018-03-28T12:24:37Z

The development guidelines states: "The minimum OSGi framework version supported is OSGi R4.2, no newer features must be used."

The OSGi stuff that is available in the Eclipse IDE provides much more recent versions, so in this case the Maven one is "more correct" as it uses an older version of the annotations.

Another example: In the Eclipse IDE you can also annotate a field in a component using @Reference but this is supported with starting with OSGi R6. Also in this case you will succeed in the IDE but fail with Maven.

htreu · 2018-03-28T12:50:07Z

Thanks. And to complete the picture (since you referenced R4V4.3) we can use these because no runtime support is needed (I see no *.annotation package in the 4.2 API).
Would it be an option to exchange the org.eclipse annotations by the org.osgi annotations in the IDE (sorry if this had been discussed before...)?

maggu2810 · 2018-03-28T13:12:23Z

exchange the org.eclipse annotations by the org.osgi annotations in the IDE

Which one ("org.eclipse annotations") do you mean?
Isn't the problem that the IDE is using the org.osgi annotations in a more recent version that the Maven build which is caused by the stuff that is present in the Target Platform?

davidgraeff · 2018-03-28T13:19:00Z

Guys :) I changed the configurationpid to a simple string and would like to go on with this PR.

* eclipse-archived#4173 changes some APIs, adapt to those. * Remove the broker connection thing, we are going to use the one defined in eclipse-archived#3839 * Remove all discovery stuff Signed-off-by: David Gräff <david.graeff@web.de>

htreu · 2018-03-28T17:03:55Z

@davidgraeff agreed :-)
Once again Travis is taking the lead:

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:1.1.0:compile (default-compile) on project org.eclipse.smarthome.io.transport.mqtt: Compilation failure: Compilation failure: 
[ERROR] /home/travis/build/eclipse/smarthome/bundles/io/org.eclipse.smarthome.io.transport.mqtt/src/main/java/org/eclipse/smarthome/io/transport/mqtt/MqttService.java:[135] 
[ERROR] 	connection = new MqttBrokerConnection(config.host, config.port, config.secure, config.clientID);
[ERROR] 	                                      ^^^^^^^^^^^
[ERROR] Null type mismatch (type annotations): required '@NonNull String' but this expression has type '@Nullable String'

Again strange that the IDE misses to show the error.

davidgraeff · 2018-03-28T18:26:21Z

Because the IDE has more background knowledge. Look at the code in question, especially
config.host != null:

if (StringUtils.isNotBlank(config.host) && config.host != null) {
        connection = new MqttBrokerConnection(config.host, config.port, config.secure, config.clientID);
        brokerConnections.put(brokerID, connection);
}

htreu · 2018-03-29T05:02:16Z

come on, don't let travis win!
Now tests fail due to unresolved dependencies. Following imports are missing:

org.mockito.invocation,
org.mockito.stubbing

After correcting this there is also one test left failing:

java.lang.AssertionError: Name getter expected:<1883> but was:<null>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:118)
	at org.eclipse.smarthome.io.transport.mqtt.MqttBrokerConnectionTest.setterGetterTests(MqttBrokerConnectionTest.java:169)

You now always initialise the port. Now the test assumption is false.

…more. * Remove name from MqttBrokerConnection. This is only necessary for the MqttService. Consumers of MqttBrokerConnection should not be bothered with that detail. * Remove textConfigured flag. Again this is not interesting for anything outside MqttService. * Don't parse configuration parameters ourself, but use a configuration values class instead. * Adapt to Nullable usage. * Mqtt: Introduce service factory service MqttBrokerConnectionServiceInstance * ESH compatible implementation of the service factory pattern including a config description for GUIs * Add MqttService.setLastWill method. Fixes eclipse-archived#5162 Signed-off-by: David Graeff <david.graeff@web.de>

davidgraeff · 2018-03-29T13:15:09Z

@htreu Tests corrected. And squashed.

htreu

Travis is happy and so am I :-)
Thanks @davidgraeff.

davidgraeff force-pushed the mqttservice branch 2 times, most recently from 370b6cf to 64d48dd Compare September 1, 2017 19:20

maggu2810 reviewed Sep 3, 2017

View reviewed changes

davidgraeff force-pushed the mqttservice branch from 64d48dd to e15627e Compare September 6, 2017 04:57

maggu2810 reviewed Sep 6, 2017

View reviewed changes

davidgraeff force-pushed the mqttservice branch 4 times, most recently from 5ca6321 to a8d6746 Compare September 8, 2017 09:40

maggu2810 reviewed Sep 21, 2017

View reviewed changes

maggu2810 mentioned this pull request Sep 22, 2017

Nullness fixes #4290

Merged

davidgraeff force-pushed the mqttservice branch from a8d6746 to f818ccd Compare September 23, 2017 06:03

davidgraeff mentioned this pull request Sep 26, 2017

Support hierarchical configuration & exclusive cfg files #4178

Closed

7 tasks

sjsf mentioned this pull request Feb 2, 2018

Adapt MQTT service to the multi-config concept #5015

Closed

davidgraeff force-pushed the mqttservice branch from f818ccd to 9c672af Compare March 15, 2018 14:39

davidgraeff mentioned this pull request Mar 17, 2018

MqttEmbeddedBroker IO extension #3910

Merged

htreu reviewed Mar 19, 2018

View reviewed changes

htreu suggested changes Mar 19, 2018

View reviewed changes

davidgraeff force-pushed the mqttservice branch from 2b32193 to b09c65c Compare March 19, 2018 16:34

davidgraeff mentioned this pull request Mar 22, 2018

OSGi services: Generate config xmls through annotations #5300

Open

htreu suggested changes Mar 27, 2018

View reviewed changes

davidgraeff mentioned this pull request Mar 28, 2018

Adapt to new mqttservice API. Remove broker connection thing. sjsf/smarthome#4

Merged

davidgraeff force-pushed the mqttservice branch from 4713b81 to d4ad8c1 Compare March 29, 2018 13:13

htreu reviewed Mar 29, 2018

View reviewed changes

htreu merged commit 325efe3 into eclipse-archived:master Mar 29, 2018

htreu added this to the 0.10.0 milestone Oct 30, 2018

MqttService: Remove all deprecated methods, rename close to stop. #4173

MqttService: Remove all deprecated methods, rename close to stop. #4173

Conversation

davidgraeff commented Sep 1, 2017 • edited

kaikreuzer commented Sep 1, 2017

maggu2810 left a comment • edited

Choose a reason for hiding this comment

davidgraeff commented Sep 4, 2017 • edited

davidgraeff commented Sep 6, 2017

Choose a reason for hiding this comment

davidgraeff Sep 6, 2017 • edited

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

kaikreuzer commented Sep 21, 2017

Choose a reason for hiding this comment

maggu2810 commented Sep 21, 2017

maggu2810 commented Sep 22, 2017 • edited

davidgraeff commented Sep 23, 2017

davidgraeff commented Mar 15, 2018 • edited

htreu commented Mar 16, 2018

davidgraeff commented Mar 16, 2018

htreu commented Mar 16, 2018

davidgraeff commented Mar 17, 2018

htreu 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

davidgraeff Mar 19, 2018 • edited

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

davidgraeff commented Mar 19, 2018

htreu 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

davidgraeff Mar 19, 2018 • edited

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

davidgraeff Mar 19, 2018 • edited

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

davidgraeff commented Mar 19, 2018

htreu commented Mar 20, 2018

maggu2810 commented Mar 20, 2018

davidgraeff commented Mar 20, 2018 • edited

davidgraeff commented Mar 20, 2018

htreu left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htreu Mar 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff Mar 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff commented Mar 27, 2018

htreu commented Mar 28, 2018 • edited

davidgraeff commented Sep 1, 2017 •

edited

maggu2810 left a comment •

edited

davidgraeff commented Sep 4, 2017 •

edited

davidgraeff Sep 6, 2017 •

edited

maggu2810 commented Sep 22, 2017 •

edited

davidgraeff commented Mar 15, 2018 •

edited

davidgraeff Mar 19, 2018 •

edited

davidgraeff Mar 19, 2018 •

edited

davidgraeff Mar 19, 2018 •

edited

davidgraeff commented Mar 20, 2018 •

edited

htreu left a comment •

edited

htreu Mar 27, 2018 •

edited

davidgraeff Mar 27, 2018 •

edited

htreu commented Mar 28, 2018 •

edited

davidgraeff commented Mar 28, 2018 •

edited

maggu2810 commented Mar 28, 2018 •

edited

htreu commented Mar 28, 2018 •

edited