Skip to content
This repository has been archived by the owner on May 17, 2021. It is now read-only.

New binding: panStamp wireless Arduino modules #3187

Merged
merged 1 commit into from Dec 29, 2015
Merged

New binding: panStamp wireless Arduino modules #3187

merged 1 commit into from Dec 29, 2015

Conversation

GideonLeGrange
Copy link
Contributor

A binding to support panStamp wireless Arduino modules.

  • I have successfully tested the binding on openhab 1.7.x and in compatibility mode on openhab 2.0 alpha2.
  • I'm busy writing wiki documentation, but for the interim my testing documentation and configs are available here: https://github.com/AutoMates/openhab-panstamp-test

For more information about panStamp, please see: http://panstamp.com/
This library is based on panstamp-java, please see: https://github.com/GideonLeGrange/panstamp-java

@GideonLeGrange
Copy link
Contributor Author

}

@Override
protected void execute() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't use the polling of an AbstractActiveBinding, extend AbstractBinding instead.

@GideonLeGrange
Copy link
Contributor Author

@watou I have changed the inheritance as requested

@watou
Copy link
Contributor

watou commented Sep 23, 2015

Thank you. I hope to offer more comments. Is there a discussion thread?

@GideonLeGrange
Copy link
Contributor Author

<properties>
<bundle.symbolicName>org.openhab.binding.panstamp</bundle.symbolicName>
<bundle.namespace>org.openhab.binding.panstamp</bundle.namespace>
<deb.name>openhab-addon-binding-PanStamp</deb.name>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use lower case: openhab-addon-binding-panstamp

@watou
Copy link
Contributor

watou commented Oct 31, 2015

Hi @GideonLeGrange, I've made a few more minor review comments on the pull request. Also, could you use the openHAB code formatting (Preferences->Java->Code Style->Formatter) and reformat to that (max line length at 120, for example). There is new activity on the forum with a user hitting a problem with serial port access; perhaps you could comment there? Thanks very much for your excellent work! Regards, John

@GideonLeGrange
Copy link
Contributor Author

Thanks for your code review. I've fixed all the errors you pointed out. I am busy testing and will update the pull request once done.

@watou
Copy link
Contributor

watou commented Nov 2, 2015

Thank you @GideonLeGrange for your excellent work. Could you also add this binding to distribution/pom.xml please?

@GideonLeGrange
Copy link
Contributor Author

@watou I'm having trouble getting Eclipse to find the openHAB code formatter. I followed your instructions but there is no openHAB formatter. I downloaded and ran the yoxos installer as described in the wiki and this installed different version of Eclipse, once again with no formatter.

Any advice is appreciated. I would hate for code formatting to cause my binding to be excluded 😕

Bundle-DocURL: http://www.openhab.org
Bundle-RequiredExecutionEnvironment: J2SE-1.5
Service-Component: OSGI-INF/binding.xml, OSGI-INF/genericbindingprovider.xml
Bundle-ClassPath: .,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While not strictly required for OH1, could you add an about.html similiar to https://github.com/openhab/openhab2/blob/master/addons/binding/org.openhab.binding.avmfritz/about.html showing the additional libraries you are using and the licenses they are distributed with?

@hakan42
Copy link
Contributor

hakan42 commented Dec 8, 2015

@GideonLeGrange , are you still active on the PR?

@watou
Copy link
Contributor

watou commented Dec 8, 2015

I would like to finish my review and merge your binding, @GideonLeGrange, so please let us know and thank you!

@watou watou mentioned this pull request Dec 9, 2015
@hakan42 hakan42 mentioned this pull request Dec 14, 2015
@watou
Copy link
Contributor

watou commented Dec 27, 2015

Hello @GideonLeGrange, we are nearly out of time to consider including your binding in openHAB 1.8. Would you be able to make final changes, tests, squash the commit into one, and verify the accuracy/completeness of the companion wiki page, in the coming days? Many thanks for past and future efforts!

@watou
Copy link
Contributor

watou commented Dec 27, 2015

...also, please add the binding to binding/pom.xml and distribution/pom.xml in this PR. Thank you!

@GideonLeGrange
Copy link
Contributor Author

I've got a bit of time this week and will fix the above. I'll be finished before the 1st

@watou
Copy link
Contributor

watou commented Dec 29, 2015

Thanks very much, @GideonLeGrange !

@GideonLeGrange
Copy link
Contributor Author

Recap of this PR:

  • Tested binding on openhab 1.7
  • Tested binding on openhab 2.0 alpha2
  • Created wiki page https://github.com/openhab/openhab/wiki/panStamp-Binding
  • Fixed a long list of concerns raised by @watou
  • Formatted code using openHAB code formatter
  • Added to distribution/pom.xml
  • Reviewed the wiki page to make sure it is current
  • Added about.html as requested by @hakan42
  • Did top-level mvn clean install successfully
  • Did squash on pull request

@GideonLeGrange
Copy link
Contributor Author

@watou I'm done, except that I'm having issues with the squash. I tried the squash as per this document https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request but run into two problems:

  • There seems to be an issue with distribution/pom.xml where I keep having to fix merge conflicts when rebasing.
  • I still see multiple commits at the top of the PR. It isn't clear to me if this is correct or indicative of a non-squashed commit.

@watou
Copy link
Contributor

watou commented Dec 29, 2015

Here is how I would go about rebasing and squashing. In your local fork:

git checkout master
git fetch upstream
git merge upstream/master
# your local master is now same as openhab repo
git push origin master
# up-to-dateness is now reflected in your fork at github
git checkout panstamp-pr
git merge-base panstamp-pr master
# copy the return string to clipboard
git rebase -i <the string returned above>
# fix any merge conflicts  between >>>> and <<<< markers
# change 2nd and subsequent pick to squash
# force push your branch to github:
git push -f origin panstamp-pr
# go to github and see if the PR looks correct

@GideonLeGrange
Copy link
Contributor Author

@watou Squash completed, I think this should be it.

@hakan42
Copy link
Contributor

hakan42 commented Dec 29, 2015

@GideonLeGrange sorry to come up with a last comment but I just noticed that you reference (and bring the binary for it) slf4j-api-1.7.7 in your pull request. Is that necessary? Is it possible to remove that dependency and use whatever the Openhab core provides to you?

@watou
Copy link
Contributor

watou commented Dec 29, 2015

Good catch, @hakan42. @GideonLeGrange, if you could remove that lib, make sure mvn clean install still works in your project, make sure the resulting (and now smaller) binding JAR still works correctly, commit the change, squash the commits, and git push -f origin <your branch>, we should be good to merge.

Also, please promote your work, if you don't mind, at https://community.openhab.org, also describing the PanStamp system and its features and benefits. The binding will be available after the next nightly build after the merge, and also in openHAB 1.8.

Thanks for your perseverance!

<strong>slf4j 1.7.7</strong> <br/><br/>
<a href="https://github.com/GideonLeGrange/panstamp-java">The Simple Logging Facade for Java</a> obtained from Maven central repository under
<a href="http://www.slf4j.org/license.html">a copy of the MIT License</a>.
</em></p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove above section

Changed binding to extend AbstractBinding

Moved to panstamp-java 2.1

Changes as requested by @watou. Still needs formatting fixed.

Updated to latest panstamp-java library build

Formatted with openHAB formatter

Revert "Formatted with openHAB formatter"

This reverts commit ee8f6df. Reformatting of files not in my work sneaked in.

Reformatted using openHAB code formatter (this time excluding other directories)

Fixed mistake I made when fixing merge conflicts

Removed slf4j from includes

Removed spurious reference to io.caldav?

Removed include of nrjavaserial

Trying to sort that weird caldav include
@GideonLeGrange
Copy link
Contributor Author

@hakan42 @watou I've removed all external jars except for the panstamp-java library. I found gnu.io (provided by nrjavaserial) being exported by one of the transport modules, so dependencies are stripped to the absolute minimum.

@watou I've removed that spurious reference to io.caldav too - it seems to have been bad dependency ordering in the pom on my part, probably happened when resolving merge conflicts.

Commits squashed.

@watou watou added this to the 1.8.0 milestone Dec 29, 2015
watou added a commit that referenced this pull request Dec 29, 2015
New binding: panStamp wireless Arduino modules
@watou watou merged commit 6d7130c into openhab:master Dec 29, 2015
@watou
Copy link
Contributor

watou commented Dec 29, 2015

Nice wiki page. I've made a small typo correction to it. Many thanks.

@hakan42
Copy link
Contributor

hakan42 commented Dec 29, 2015

Very nice, thank you for your efforts.

@GideonLeGrange GideonLeGrange deleted the panstamp-pr branch December 29, 2015 20:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants