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

[modbus.lambda] Lambda Heat Pump #17595

Closed
wants to merge 0 commits into from
Closed

[modbus.lambda] Lambda Heat Pump #17595

wants to merge 0 commits into from

Conversation

chilobo
Copy link

@chilobo chilobo commented Oct 20, 2024

This is my first contribution on github!

Based on the Stiebel Eltron bundle by Paul Frank I contribute a bundle to manage Lambda Heat Pumps as a OSG addon to the Modbus Binding.

I did some testing by adding my jar to the local addon directory.
See the README.md for restrictions on usage.

The binding is based on the Modbus description of the manufacturer:
(https://lambda-wp.at/wp-content/uploads/2022/01/Modbus-Protokoll-und-Beschreibung.pdf)

Compressed directory:
org.openhab.binding.modbus.lambda.zip

@chilobo chilobo requested a review from a team as a code owner October 20, 2024 15:47
@jlaur jlaur added the new binding If someone has started to work on a binding. For a new binding PR. label Oct 20, 2024
@jlaur jlaur changed the title Lambda Heat Pump bundle for Openhab [modbus.lambda] Lambda Heat Pump Oct 20, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Oct 20, 2024

Thank you for this contribution. It would be nice if you could perform a self review see the checklist here: https://github.com/openhab/openhab-addons/wiki/Review-Checklist
Also make sure that this binding is also added to files outside of the binding folder:
bom/openhab-addons/pom.xml
bundles/pom.xml
CODEOWNERS

Once these are done, me or another maintainer will review this. Just ping if you need anything.

@chilobo
Copy link
Author

chilobo commented Oct 21, 2024

Once these are done, me or another maintainer will review this. Just ping if you need anything.

I added the changes to
bom/openhab-addons/pom.xml
bundles/pom.xml
CODEOWNERS
and put some // in front of my trace loggings.
I read the Review-Checklist.
I guess some of the channel names are too long, but

  • I tried to stick with the names the manufacturer provided.
  • Some descriptions of the modbus registers are quite similar in different groups, so shortening them could produce legibility problems.

After some fiddling with git to signoff my commits and annoying problems with VS formatting pom.xml etc. the project was compiled without errors in github.
Please review!

@chilobo chilobo force-pushed the main branch 2 times, most recently from b1c9241 to 528d406 Compare October 21, 2024 09:05
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/add-heatpump-binding-to-modbus-binding-help-needed/157725/10

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.

Thank you for contributing to openHAB, greatly appreciated. This is a partly review, I looked at 33/38 files. I left some comments, the most important one is about getting the openHAB side of the moddeling right. From there on i can look at the rest of the files. Let me know if you need anything

@chilobo
Copy link
Author

chilobo commented Oct 24, 2024

I am lost.
@Java17: The details button does not show what the problem is.
The problem with the DCO Action cannot be resolved by me because I am not a codeowner of openhab?

@lsiepel
Copy link
Contributor

lsiepel commented Oct 25, 2024

I am lost. @Java17: The details button does not show what the problem is. The problem with the DCO Action cannot be resolved by me because I am not a codeowner of openhab?

If you click details next to the DCO issue, there are 4 commits that lack the signoff. That page also lists some actions to take to fix it. (you have to rewrite the commit message).

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.

Thank you for fixing most comments quicly. When looking at the other files i got some new ones. Only the handler is left to review. But that is only usefull after we finished the modelling of things and channels.

@chilobo
Copy link
Author

chilobo commented Nov 26, 2024

I think I am finished with the necessary changes.
@ISiepel: Is there anything else missing?

@lsiepel
Copy link
Contributor

lsiepel commented Nov 26, 2024

Some commits got accidently added to this PR. Please rebase / undo excess commits

@chilobo
Copy link
Author

chilobo commented Dec 2, 2024

@lsiepel
I did a rebase removed some tracelogs and would ask you to have a look at the project again.

@lsiepel
Copy link
Contributor

lsiepel commented Dec 2, 2024

@lsiepel I did a rebase removed some tracelogs and would ask you to have a look at the project again.

Could you fix the excess commits and conflicts? I guess something went wrong with rebasing.

@chilobo
Copy link
Author

chilobo commented Dec 3, 2024

@lsiepel I did a rebase removed some tracelogs and would ask you to have a look at the project again.

Could you fix the excess commits and conflicts? I guess something went wrong with rebasing.

I tried again, but I guess I failed again. In vscode the merging now looks like:
image
... and I have not yet checked if my code is up to date, but I have working copies.
Does this look OK to you?

@lsiepel
Copy link
Contributor

lsiepel commented Dec 3, 2024

You have to merge openHAB main into your GitHub main. And from there in your feature branch. It looks like you updated your feature branch straight from openHAB main

@chilobo
Copy link
Author

chilobo commented Dec 3, 2024

You have to merge openHAB main into your GitHub main. And from there in your feature branch. It looks like you updated your feature branch straight from openHAB main

I am lost...
Should I try to do something like:
https://community.openhab.org/t/rebase-your-code-or-how-to-fix-your-git-history-before-requesting-a-pull/129358

@lsiepel
Copy link
Contributor

lsiepel commented Dec 3, 2024

You have to merge openHAB main into your GitHub main. And from there in your feature branch. It looks like you updated your feature branch straight from openHAB main

I am lost... Should I try to do something like: https://community.openhab.org/t/rebase-your-code-or-how-to-fix-your-git-history-before-requesting-a-pull/129358

As from there you pull in the changes from upstream straight into your local branch this can be expected.

  1. You could merge openhab main into your github fork main See here: https://github.com/chilobo/openhab-addons/tree/main is 400+ commits behind.
  2. Backup the binding folder. Create a new fork / branch etc and in the end a new PR if you can't fix it.

@chilobo
Copy link
Author

chilobo commented Dec 5, 2024

You have to merge openHAB main into your GitHub main. And from there in your feature branch. It looks like you updated your feature branch straight from openHAB main

I am lost... Should I try to do something like: https://community.openhab.org/t/rebase-your-code-or-how-to-fix-your-git-history-before-requesting-a-pull/129358

As from there you pull in the changes from upstream straight into your local branch this can be expected.

1. You could merge openhab main into your github fork main See here: https://github.com/chilobo/openhab-addons/tree/main is 400+ commits behind.

2. Backup the binding folder. Create a new fork / branch etc and in the end a new PR if you can't fix it.

I went back to a really old version, did a git pull and used my local backup to change the project.
The old pull request was closed - that is OK with me.
Should I delete this fork now?

Then I pushed my local fork to github and opened up a new pull request:
[modbus.lambda] Lambda Heat Pump #17846

But ...
I changed CODEOWNERS, but forgot to change pom.xml in the bundles directory, so the compilation failed.
Now I added
org.openhab.binding.modbus.lambda
in the bundles\pom.xml and pushed it to github.
Will the compilation start automagically or do I have to do something else?

Could you have a look at the new pull request?

@chilobo
Copy link
Author

chilobo commented Dec 5, 2024

I fixed the DCO failures and the compilations worked out.
So: When you find time, please look at the new pull request.
Should I delete the old fork?

@lsiepel
Copy link
Contributor

lsiepel commented Dec 5, 2024

I'll look into the new PR, let's continue there.

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.

4 participants