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

[fpp] Initial contribution #16298

Merged
merged 21 commits into from
Sep 20, 2024
Merged

[fpp] Initial contribution #16298

merged 21 commits into from
Sep 20, 2024

Conversation

computergeek1507
Copy link
Contributor

@computergeek1507 computergeek1507 commented Jan 17, 2024

Description

Created a binding to Controls Falcon Player (FPP) Devices. Uses MQTT to update status.

https://community.openhab.org/t/fpp-binding/157478

@computergeek1507 computergeek1507 added the new binding If someone has started to work on a binding. For a new binding PR. label Jan 17, 2024
@computergeek1507 computergeek1507 requested a review from a team as a code owner January 17, 2024 17:58
@wborn wborn changed the title - [fpp][WIP] Initial contribution [fpp][WIP] Initial contribution Jan 17, 2024
@wborn wborn added the work in progress A PR that is not yet ready to be merged label Jan 17, 2024
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

This PR is a WIP for some months, that is perfectly fine. There are some build failures and other issues. I did a quickscan and the most important one te mention is that the channel and type id's are not according to the naming convention (lower-case-hyphen)
For reference: https://www.openhab.org/docs/developer/guidelines.html#naming-convention

Hope you can proceed with this PR, let me know if you need anything.

computergeek1507 and others added 8 commits June 12, 2024 13:11
Signed-off-by: Scott Hanson <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
@computergeek1507 computergeek1507 changed the title [fpp][WIP] Initial contribution [fpp] Initial contribution Jul 27, 2024
@lsiepel lsiepel removed the work in progress A PR that is not yet ready to be merged label Jul 30, 2024
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. Basics of this binding already seem good, there are some comments and questions. Besides this:

  • check compile warnings, there should be no warnings.
  • check SAT, only the missing bridge with id broker is ok, the rest should be fixed.

Let me know if you need anything to proceed, ping me when you are ready for another review round.

computergeek1507 and others added 6 commits July 31, 2024 23:18
Co-authored-by: lsiepel <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
Signed-off-by: Scott Hanson <[email protected]>
@lsiepel
Copy link
Contributor

lsiepel commented Sep 14, 2024

There is one comment left open, besides that this binding should also be added to the footer.xml here and add yourself as codeowner here
Otherwise we are ready to merge.

@lsiepel
Copy link
Contributor

lsiepel commented Sep 19, 2024

@computergeek1507 We can merge this and get it in the next milestone if you add the last two bits: codewoner and footer.xml

@lsiepel lsiepel added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 20, 2024
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

Now, you could add your binding's logo to the openHAB website. See https://next.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website-and-the-ui

@lsiepel lsiepel merged commit 163f517 into openhab:main Sep 20, 2024
3 of 5 checks passed
@lsiepel lsiepel added this to the 4.3 milestone Sep 20, 2024
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.

3 participants