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

[linktap] Initial contribution #17235

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Conversation

dag81
Copy link
Contributor

@dag81 dag81 commented Aug 11, 2024

[linkTap] Initial code commit.

@lsiepel lsiepel added the new binding If someone has started to work on a binding. For a new binding PR. label Aug 11, 2024
@dag81 dag81 force-pushed the bindinngs/linkTap branch 2 times, most recently from 24e1a13 to e8799ee Compare August 11, 2024 22:06
@lsiepel
Copy link
Contributor

lsiepel commented Aug 11, 2024

Thank you for creating this PR to contribute this binding. As this is still a draft it won’t be reviewed yet, but I had a quick peek and would like to comment upfront.
To manage expectations; review capacity is somewhat limited so don’t expect an immediate full review. To speed up the process you can already perform a self-review by checking this list: https://github.com/openhab/openhab-addons/wiki/Review-Checklist
For example one of the improvements that should be made is to adhere to the naming convention lower-case-hyphen for channel id’s etc. For reference: https://www.openhab.org/docs/developer/guidelines.html#naming-convention

@dag81
Copy link
Contributor Author

dag81 commented Aug 11, 2024

Thank you for creating this PR to contribute this binding. As this is still a draft it won’t be reviewed yet, but I had a quick peek and would like to comment upfront. To manage expectations; review capacity is somewhat limited so don’t expect an immediate full review. To speed up the process you can already perform a self-review by checking this list: https://github.com/openhab/openhab-addons/wiki/Review-Checklist For example one of the improvements that should be made is to adhere to the naming convention lower-case-hyphen for channel id’s etc. For reference: https://www.openhab.org/docs/developer/guidelines.html#naming-convention

Thank you I'll work through it before changing to a non-draft PR to save any time wastage.

@dag81 dag81 force-pushed the bindinngs/linkTap branch 2 times, most recently from a3e33b3 to 8c3cc6c Compare August 11, 2024 23:03
[linkTap] Initial code commit.

Signed-off-by: dag81 <[email protected]>
[linkTap] Initial code commit.

Signed-off-by: dag81 <[email protected]>
[linkTap] Initial code commit.

Signed-off-by: dag81 <[email protected]>
[linkTap] Initial code commit.

Signed-off-by: dag81 <[email protected]>
@dag81 dag81 force-pushed the bindinngs/linkTap branch 5 times, most recently from b97f678 to 3079be4 Compare August 19, 2024 13:43
@lsiepel lsiepel changed the title [linkTap] Initial Code Commit [linktap] Initial contribution Aug 19, 2024
@dag81 dag81 force-pushed the bindinngs/linkTap branch 8 times, most recently from 3709a3f to b7c26bc Compare August 20, 2024 20:34
@lsiepel
Copy link
Contributor

lsiepel commented Sep 9, 2024

Sure - I did that before on a different PR and they preferred resolving them :) More than happy to thumb's up - I just need to track which ones I've been through - will switch to that now.

Oh, another request ;-) could you try not to use force-push as by then it is harder to track progress. If you just commit and push it should be fine.

@dag81
Copy link
Contributor Author

dag81 commented Sep 9, 2024

Sure - I did that before on a different PR and they preferred resolving them :) More than happy to thumb's up - I just need to track which ones I've been through - will switch to that now.

Oh, another request ;-) could you try not to use force-push as by then it is harder to track progress. If you just commit and push it should be fine.

Now I know its being looked at I'll push the changes in different commits - you were very quick at getting on it - thank you. I presume when merged the whole lot can be squashed at that point.

[linkTap] PR Feedback 1

Signed-off-by: dag81 <[email protected]>
@dag81
Copy link
Contributor Author

dag81 commented Sep 9, 2024

Hi @lsiepel, I believe there's approx 3 bits left which I'll look at tomorrow evening if I have time. I've just pushed everything from tonight, thank you for all the work reviewing. Anything with a thumb's up I've hopefully / potentially covered in the feedback 1 push.

I don't know if you want to wait for all to be addressed or am happy to start resolving any your happy with now, but figured I'll let you know either way.

P.S On the servlet question, the gateway basically pushes event's via http. The servlet is to receive these pushes from the gateway itself. Obviously if it can't for whatever reason the system will fallback to a polling less responsive design after it hasn't receive certain data for so long, but hopefully for the majority of users, this will cover the majority of openHAB's user base.

[linkTap] PR Feedback 2

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Feedback 3

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Feedback 3

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Feedback 4

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Feedback 5

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Feedback 6

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Feedback 7

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Feedback 8

Signed-off-by: dag81 <[email protected]>
@dag81
Copy link
Contributor Author

dag81 commented Sep 10, 2024

Hi @lsiepel, thanks for all the time again tonight reviewing. I think I've caught up now and all the comments are addressed. Since the push ending with the text Feedback 8.

[linkTap] FW Ver req updated

Signed-off-by: dag81 <[email protected]>
@dag81 dag81 requested a review from lsiepel September 17, 2024 07:37
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.

Finished the review and covered the full binding. Nice to see there are no compile and SAT warnings. After these comments are addressed i expect this PR to be in a mergable state, so we are getting close now.

private static final String DEFAULT_INST_WATERING_VOL_LIMIT = "0";
private static final String DEFAULT_INST_WATERING_TIME_LIMIT = "15";

private static final double GW_LITRES_M3_CONVERSION_RATIO = 3.785;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static final double GW_LITRES_M3_CONVERSION_RATIO = 3.785;

No need for this as the framework can do it for you. See next few comments as they belong together

if (speed != null) {
updateState(DEVICE_CHANNEL_FLOW_RATE, new QuantityType<>(
"L".equals(volumeUnit) ? speed : (speed * GW_LITRES_M3_CONVERSION_RATIO), Units.LITRE_PER_MINUTE));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
updateState(DEVICE_CHANNEL_FLOW_RATE, new QuantityType<>(speed,
"L".equals(volumeUnit) ? Units.LITRE_PER_MINUTE : ImperialUnits.GALLON_PER_MINUTE));

if (volume != null) {
updateState(DEVICE_CHANNEL_CURRENT_VOLUME, new QuantityType<>(
"L".equals(volumeUnit) ? volume : (volume * GW_LITRES_M3_CONVERSION_RATIO), Units.LITRE));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
updateState(DEVICE_CHANNEL_CURRENT_VOLUME,
new QuantityType<>(volume, "L".equals(volumeUnit) ? Units.LITRE : ImperialUnits.GALLON_LIQUID_US));

if (volumeLimit != null) {
updateState(DEVICE_CHANNEL_FAILSAFE_VOLUME, new QuantityType<>(
"L".equals(volumeUnit) ? volumeLimit : (volumeLimit * GW_LITRES_M3_CONVERSION_RATIO), Units.LITRE));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
updateState(DEVICE_CHANNEL_FAILSAFE_VOLUME, new QuantityType<>(volumeLimit,
"L".equals(volumeUnit) ? Units.LITRE_PER_MINUTE : ImperialUnits.GALLON_PER_MINUTE));

Comment on lines +100 to +101

private static final double GW_LITRES_M3_CONVERSION_RATIO = 3.785;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to do manual conversion, leave it to the framework.

Suggested change
private static final double GW_LITRES_M3_CONVERSION_RATIO = 3.785;

bundles/org.openhab.binding.linktap/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.linktap/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.linktap/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.linktap/README.md Show resolved Hide resolved
bundles/org.openhab.binding.linktap/README.md Outdated Show resolved Hide resolved
[linkTap] Corrections for equals invokes

Signed-off-by: dag81 <[email protected]>
[linkTap] Names and casting

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Adjustments 1 pass 2

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Adjustments 2 pass 2

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Adjustments 3 pass 2

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Adjustments 4 pass 2

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Adjustments 4 pass 2

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Adjustments 4 pass 2

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Adjustments 5 pass 2

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Adjustments 6 pass 2

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Adjustments 7 pass 2

Signed-off-by: dag81 <[email protected]>
[linkTap] PR Adjustments 8 pass 2

Signed-off-by: dag81 <[email protected]>
[linkTap] i18 http warning addition

Signed-off-by: dag81 <[email protected]>
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.

2 participants