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

[bluetooth.govee] Govee Bluetooth Binding initial contribution #8610

Merged
merged 15 commits into from
Jan 25, 2021

Conversation

cpmeister
Copy link
Contributor

I got some of these devices in order to try to test the new bluetooth dynamic channel generation feature I've been working on. Sadly they didn't really help, but since I already had the devices on my hands I made a quick binding for them anyway.

Signed-off-by: Connor Petty [email protected]

@cpmeister cpmeister requested a review from a team as a code owner September 30, 2020 00:05
@TravisBuddy
Copy link

TravisBuddy commented Sep 30, 2020

Travis tests were successful

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

@cpmeister cpmeister added the new binding If someone has started to work on a binding. For a new binding PR. label Sep 30, 2020
@cpmeister
Copy link
Contributor Author

If you consider this a test of the new binding generation script, it seems to have really botched the bundles/pom.xml file by adding blank lines everywhere.

@cpmeister cpmeister added the work in progress A PR that is not yet ready to be merged label Nov 3, 2020
Connor Petty added 2 commits November 27, 2020 13:17
Signed-off-by: Connor Petty <[email protected]>
Signed-off-by: Connor Petty <[email protected]>
@cpmeister cpmeister removed the work in progress A PR that is not yet ready to be merged label Nov 28, 2020
Connor Petty added 2 commits November 27, 2020 21:00
Signed-off-by: Connor Petty <[email protected]>
Signed-off-by: Connor Petty <[email protected]>
@cpmeister
Copy link
Contributor Author

So my initial implementation of this binding didn't actually work when I tested it, so I've had to rewrite it from scratch.
Been working and testing this for the past month and I can finally say this is ready for review.

Signed-off-by: Connor Petty <[email protected]>
@cpmeister cpmeister requested a review from fwolter December 4, 2020 18:27
bundles/org.openhab.binding.bluetooth.govee/README.md Outdated Show resolved Hide resolved
<features name="org.openhab.binding.bluetooth.govee-${project.version}" xmlns="http://karaf.apache.org/xmlns/features/v1.4.0">
<repository>mvn:org.openhab.core.features.karaf/org.openhab.core.features.karaf.openhab-core/${ohc.version}/xml/features</repository>

<feature name="openhab-binding-bluetooth-govee" description="Bluetooth Binding Govee" version="${project.version}">
Copy link
Member

Choose a reason for hiding this comment

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

You use "extension", "adapter" and "binding" for this code in different places.

Copy link
Contributor Author

@cpmeister cpmeister Dec 4, 2020

Choose a reason for hiding this comment

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

That is kinda the case for all of the bluetooth bindings...
"extension" refers to the fact that the binding adds functionality to the base bluetooth bindings.
"adapter" is used the pom.xml as part of the project name. But strictly speaking I don't really think this is appropriate and the word should probably be changed to extension instead. But this standard was in place for all of the other bluetooth bindings so if I'm going to change it it should be a separate PR.

I do agree with you that these naming conventions are a bit confusing and I would love to change them at some point.

* @author Connor Petty - Initial contribution
*
*/
public class ThingTypeTableGenerator {
Copy link
Member

Choose a reason for hiding this comment

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

I like this. Love to see this at some more prominent location.

Signed-off-by: Connor Petty <[email protected]>
Signed-off-by: Connor Petty <[email protected]>
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

@fwolter fwolter added the cre Coordinated Review Effort label Dec 6, 2020
Connor Petty added 6 commits January 15, 2021 11:46
Signed-off-by: Connor Petty <[email protected]>
Signed-off-by: Connor Petty <[email protected]>
Signed-off-by: Connor Petty <[email protected]>
Signed-off-by: Connor Petty <[email protected]>
Signed-off-by: Connor Petty <[email protected]>
@cpmeister
Copy link
Contributor Author

@openhab/add-ons-maintainers This needs a second reviewer if someone can take a look. Thanks.

@wborn wborn added the rebuild Triggers Jenkins PR build label Jan 22, 2021
@wborn wborn removed the rebuild Triggers Jenkins PR build label Jan 22, 2021
Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

Just some small stuff.

Signed-off-by: Connor Petty <[email protected]>
@cpmeister cpmeister requested a review from Hilbrand January 24, 2021 07:38
Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

LGTM

@Hilbrand Hilbrand merged commit 239e33a into openhab:main Jan 25, 2021
@Hilbrand Hilbrand removed the cre Coordinated Review Effort label Jan 25, 2021
@Hilbrand Hilbrand added this to the 3.1 milestone Jan 25, 2021
lsiepel added a commit to lsiepel/openhab-addons that referenced this pull request Feb 9, 2021
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants