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.

Sign up for GitHub

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

Jump to bottom

[vwweconnect] [WIP] Initial contribution #8314

Closed

jannegpriv wants to merge 4 commits into openhab:2.5.x from jannegpriv:8108-vwweconnect

Contributor

jannegpriv commented Aug 19, 2020

[vwweconnect] [WIP] Initial contribution.

My goal is to add support for the VW We Connect portal to openHAB.

Link to community thread with URL to latest built jar-file:
https://community.openhab.org/t/new-binding-volkswagen-we-connect/92057

jannegpriv requested a review from a team as a code owner

August 19, 2020 14:52

fwolter added the new binding label

Hilbrand added the work in progress label

fwolter requested changes

View reviewed changes

Member

fwolter left a comment

Can you add yourself to the CODEOWNERS file?

You need to add your binding to bundles/pom.xml and bom/openhab-addons/pom.xml.

fwolter requested changes

View reviewed changes

Member

fwolter left a comment

I peeked into the first files of your code. Here is my feedback.

bundles/org.openhab.binding.vwweconnect/README.md

		@@ -0,0 +1,367 @@
		# VW We Connect Binding

		This is an OpenHAB binding for VW We Connect Portal.

Member

fwolter Aug 21, 2020

Suggested change

      
            This is an OpenHAB binding for VW We Connect Portal.
          
            This is an openHAB binding for VW We Connect Portal.

bundles/org.openhab.binding.vwweconnect/README.md

Comment on lines +19 to +22

+              You will have to configure the bridge with username and password, these must be the same credentials as used when logging into:
+              https://www.portal.volkswagen-we.com.
+              You must also configure your secure pin to be able to lock/unlock and start/stop the heater/ventilation.

Member

fwolter Aug 21, 2020

As the binding doesn't have any general configuration, this chapter could be removed.

bundles/org.openhab.binding.vwweconnect/README.md

Comment on lines +38 to +40

		## Disclaimer

		Any side effects using this binding is the responsibility of the person installing and using the binding and not the binding developer.

Member

fwolter Aug 21, 2020

Bindings don't need a disclaimer as this is covered by the framework's license.

bundles/org.openhab.binding.vwweconnect/README.md


		#### Channels

		([vwweconnectapi]) supports the following channel:

Member

fwolter Aug 21, 2020

Do you intent to get a special formatting with the brackets? The GitHub preview doesn't render them in a special way.

bundles/org.openhab.binding.vwweconnect/README.md

Comment on lines +79 to +90

+              | details#name                         | String                 | Vehicle name                            |
+              | details#model                        | String                 | Vehicle model                           |
+              | details#modelCode                    | String                 | Vehicle model code                      |
+              | details#modelYear                    | String                 | Vehicle model year                      |
+              | details#enrollmentDate               | DateTime               | Vehicle enrollment date                 |
+              | details#dashboardURL                 | String                 | User's home URL                         |
+              | details#imageURL                     | Image                  | Vehicle picture                         |
+              | details#engineTypeCombustian         | Switch                 | Is engine type combustian               |
+              | details#engineTypeElectric           | Switch                 | Is engine type electric                 |
+              | details#engineTypeHybridOCU1         | Switch                 | Is engine type hybrid OCU1              |
+              | details#engineTypeHybridOCU2         | Switch                 | Is engine type hybrid OCU2              |
+              | details#engineTypeCNG                | Switch                 | Is engine type compressed natural gas   |

Member

fwolter Aug 21, 2020

I think these are rather properties than Channels.

...g.vwweconnect/src/main/java/org/openhab/binding/vwweconnect/internal/VWWeConnectSession.java

Comment on lines +778 to +780

+                      for (int i = 0; i < vinList.size(); i++) {
+                          String vin = (String) vinList.get(i);
+                          String dashboardUrl = (String) dashboardUrlList.get(i);

Member

fwolter Aug 22, 2020

Better check if both lists have the same length.

...g.vwweconnect/src/main/java/org/openhab/binding/vwweconnect/internal/VWWeConnectSession.java

Comment on lines +911 to +914

+                      for (int i = 0; i < vinList.size(); i++) {
+                          String vin = (String) vinList.get(i);
+                          logger.debug("Trying to fetch status for vehicle with VIN {}", vin);
+                          String dashboardUrl = (String) dashboardUrlList.get(i);

Member

fwolter Aug 22, 2020

See above

...g.vwweconnect/src/main/java/org/openhab/binding/vwweconnect/internal/VWWeConnectSession.java

+                                  while (requestStatus != null && requestStatus.equals(REQUEST_IN_PROGRESS)) {
+                                      try {
+                                          long currentTime = System.currentTimeMillis();
+                                          logger.debug("Time: {}", new Timestamp(currentTime));

Member

fwolter Aug 22, 2020

Do you use the SQL class by intention?

...onnect/src/main/java/org/openhab/binding/vwweconnect/internal/action/VWWeConnectActions.java

+                  private @Nullable VehicleHandler handler;
+                  public VWWeConnectActions() {
+                      logger.info("VW We Connect actions service instanciated");

Member

fwolter Aug 22, 2020

Can this message be replaced by using the debugger or by increasing the framework's log level? See point 4 https://www.openhab.org/docs/developer/guidelines.html#f-logging

...onnect/src/main/java/org/openhab/binding/vwweconnect/internal/action/VWWeConnectActions.java

+                  public static void heaterStartCommand(@Nullable ThingActions actions) {
+                      if (actions instanceof VWWeConnectActions) {
+                          ((VWWeConnectActions) actions).heaterStartCommand();

Member

fwolter Aug 22, 2020

Are you aware of this bug openhab/openhab-core#1265?
You could add the workaround like here:
https://github.com/openhab/openhab-addons/blob/2.5.x/bundles/org.openhab.binding.ecobee/src/main/java/org/openhab/binding/ecobee/action/EcobeeActions.java#L89

TravisBuddy commented Sep 5, 2020 •

edited

Loading

Travis tests were successful

Hey @jannegpriv,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

jannegpriv added 3 commits

September 5, 2020 16:47


          First version

d62ea3a

Signed-off-by: Jan Gustafsson <[email protected]>


          Updated POM.

dec91c3

Signed-off-by: Jan Gustafsson <[email protected]>


          Cosmetic changes.

1b89e77

Signed-off-by: Jan Gustafsson <[email protected]>

jannegpriv force-pushed the 8108-vwweconnect branch from a0ffa81 to 1b89e77 Compare

September 19, 2020 10:12

kaikreuzer added migrate to OH3 and removed migrate to OH3 labels

Collaborator

openhab-bot commented Sep 20, 2020

Automatic code migration to openHAB 3 succeeded.

The resulting code can be found at https://ci.openhab.org/job/openHAB-Addons-Migration/30/artifact/bundles/.

You can download the migrated code from there and create a new PR against the master branch of the openhab-addons repo to contribute it for being included in openHAB 3.x.

Please see this issue about the details on how to proceed with your existing PR.


          Updated README.

99198a4

Signed-off-by: Jan Gustafsson <[email protected]>

Contributor Author

jannegpriv commented Oct 25, 2020

@Hilbrand I've changed my upstream to point to openhab-addons after the name change but I fail to merge into my 2,5,x branch:

$ git remote add upstream https://github.com/openhab/openhab-addons.git
jannegpriv in /Users/jannegpriv/git/openhab-master/git/openhab-addons (2.5.x) (22 entries, 6 hidden)
$ git remote -v
origin	https://github.com/jannegpriv/openhab2-addons.git (fetch)
origin	https://github.com/jannegpriv/openhab2-addons.git (push)
upstream	https://github.com/openhab/openhab-addons.git (fetch)
upstream	https://github.com/openhab/openhab-addons.git (push)
$ git fetch upstream
remote: Enumerating objects: 1539, done.
remote: Counting objects: 100% (1539/1539), done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 19016 (delta 1533), reused 1533 (delta 1533), pack-reused 17477
Receiving objects: 100% (19016/19016), 8.55 MiB | 2.77 MiB/s, done.
Resolving deltas: 100% (6773/6773), completed with 601 local objects.
From https://github.com/openhab/openhab-addons
 * [new branch]            2.5.x      -> upstream/2.5.x
 * [new branch]            dependabot/maven/bundles/org.openhab.persistence.dynamodb/com.fasterxml.jackson.core-jackson-databind-2.9.10.5 -> upstream/dependabot/maven/bundles/org.openhab.persistence.dynamodb/com.fasterxml.jackson.core-jackson-databind-2.9.10.5
 * [new branch]            dependabot/maven/bundles/org.openhab.persistence.jdbc/mysql-mysql-connector-java-8.0.16 -> upstream/dependabot/maven/bundles/org.openhab.persistence.jdbc/mysql-mysql-connector-java-8.0.16
 * [new branch]            main       -> upstream/main
 * [new tag]               2.5.0      -> 2.5.0
 * [new tag]               V0.6.0     -> V0.6.0
 * [new tag]               v0.4.0     -> v0.4.0
 * [new tag]               v0.5.0     -> v0.5.0
jannegpriv in /Users/jannegpriv/git/openhab-master/git/openhab-addons (2.5.x) (22 entries, 6 hidden)
$ git merge upstream/2.5.x
fatal: refusing to merge unrelated histories

Do I have to fork and clone the openhab-addons from start?

Member

fwolter commented Oct 26, 2020

It might be easier to start over. Nevertheless, you need to migrate your code manually to OH3 or use the migrated code by the openhab-bot.

Contributor Author

jannegpriv commented Oct 26, 2020

It might be easier to start over. Nevertheless, you need to migrate your code manually to OH3 or use the migrated code by the openhab-bot.

By start-over you mean to close this PR, do a new fork/clone and adapt my binding to 3.0 and then open a new PR towards 3.0?
Where can I get more info about this openhab-bot?

Member

fwolter commented Oct 26, 2020

Correct. The procedure is described here #8512. The bot posted a link to the migrated code above: #8314 (comment)

Contributor Author

jannegpriv commented Oct 26, 2020

Correct. The procedure is described here #8512. The bot posted a link to the migrated code above: #8314 (comment)

OK, thanks for the tips! :-)

Contributor Author

jannegpriv commented Oct 30, 2020

Correct. The procedure is described here #8512. The bot posted a link to the migrated code above: #8314 (comment)

@fwolter @Hilbrand
I managed to get an openHAB 3.0 eclipse environment working and the VWWeconnect binding builds fine.
However when I want to debug the binding it has some depenedencies listed in its pom.xml:

<dependency>
      <groupId>org.jsoup</groupId>
      <artifactId>jsoup</artifactId>
      <version>1.8.3</version>
      <scope>compile</scope>
    </dependency>
    <dependency>
      <groupId>com.jayway.jsonpath</groupId>
      <artifactId>json-path</artifactId>
      <version>2.4.0</version>
      <scope>compile</scope>
    </dependency>
    <dependency>
      <groupId>org.ow2.asm</groupId>
      <artifactId>asm</artifactId>
      <version>5.0.4</version>
      <scope>compile</scope>
    </dependency>
    <dependency>
      <groupId>net.minidev</groupId>
      <artifactId>accessors-smart</artifactId>
      <version>1.2</version>
      <scope>compile</scope>
    </dependency>
    <dependency>
      <groupId>net.minidev</groupId>
      <artifactId>json-smart</artifactId>
      <version>2.3</version>
      <scope>compile</scope>
    </dependency>

I've added the binding to the Run Requirements of app.bndrun and when pressing Resolve button I get the following unresolved requirements:

Resolution failed. Capabilities satisfying the following requirements could not be found:
    [<<INITIAL>>]
      ⇒ osgi.identity: (osgi.identity=org.openhab.binding.vwweconnect)
          ⇒ [org.openhab.binding.vwweconnect version=3.0.0.202010300859]
              ⇒ osgi.wiring.package: (&(osgi.wiring.package=com.jayway.jsonpath)(version>=2.4.0)(!(version>=3.0.0)))
                  ⇒ [com.jayway.jsonpath.json-path version=2.4.0]
                      ⇒ osgi.wiring.package: (&(osgi.wiring.package=net.minidev.json.writer)(version>=2.3.0)(!(version>=3.0.0)))
                          ⇒ [net.minidev.json-smart version=2.3.0]
                              ⇒ osgi.wiring.package: (&(osgi.wiring.package=net.minidev.asm)(version>=1.2.0)(!(version>=2.0.0)))
                                  ⇒ [net.minidev.accessors-smart version=1.2.0]
                                      ⇒ osgi.wiring.package: (&(osgi.wiring.package=org.objectweb.asm)(version>=5.0.0)(!(version>=6.0.0)))

I'm not sure this is a new problem or if I had it on my old build environment for 2.5.x and resolved it there in a way I cannot remember now, do you have any tips how to fix this?

Member

fwolter commented Nov 1, 2020

The groupId org.objectweb of asm doesn't seem to match your maven dependency.

Contributor Author

jannegpriv commented Nov 1, 2020

The groupId org.objectweb of asm doesn't seem to match your maven dependency.

After doing some mvn clean install on both binding and openhab-distro and refreshing Eclipse the Resolve finally succeeded.

When debugging and starting a scan from the new GUI I got the following WARN:

Representation property 'WVABGDJFGJKSHDUGHJ' of discovery result for thing 'vwweconnect:vehicle:3f65494f1c:WVABGDJFGJKSHDUGHJ' is missing in properties map. It has to be fixed by the bindings developer.

That is a new warning, how do I fix that?

I got the following definitions in the vehicle.xml:

               ...
                <properties>
			<property name="vendor">Volkswagen</property>
		</properties>

		<representation-property>vin</representation-property>

		<config-description>
			<parameter name="vin" type="text" required="true">
				<label>Vehicle Identification Number</label>
				<description>VIN of the vehicle associated with this Thing</description>
			</parameter>
		</config-description>
	</thing-type>

What is the properties map ?

fwolter requested changes

View reviewed changes

...ain/java/org/openhab/binding/vwweconnect/internal/discovery/VWWeConnectDiscoveryService.java

Comment on lines +83 to +84

		DiscoveryResult discoveryResult = DiscoveryResultBuilder.create(thingUID).withLabel(label)
		.withBridge(bridgeUID).withProperty(VIN, vin).withRepresentationProperty(vin).build();

Member

fwolter Nov 1, 2020

The representation property must be the name of the property, not the value.

Contributor Author

jannegpriv Nov 1, 2020

aha :-)

Thanks for the info!

I've now created a new PR #8934 for openHAB3.0, this PR can be closed.

jannegpriv closed this

jannegpriv deleted the 8108-vwweconnect branch

November 1, 2020 19:33

jannegpriv mentioned this pull request

[vwweconnect] [WIP] Initial contribution #8934

Closed

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

Labels

new binding work in progress