[grundfosalpha] Initial contribution #15907

tisoft · 2023-11-18T13:19:00Z

This adds support for reading out the data of Grundfos Alpha Pumps with a Grundfos Alpha Reader (https://product-selection.grundfos.com/de/products/alpha-reader/mi401-alpha-reader-98916967?pumpsystemid=2213476387&tab=variant-specifications)

tisoft · 2023-11-18T13:23:42Z

I had to disable the default filtering of duplicate data in bluez, otherwise I could not get the data of the alpha reader. I only used bluez on my development laptop, in my production environment I use a bluegiga dongle. That worked out of the box, so the configuration for the bluez and bluegiga environment are different by default. The commit in this PR makes the bluez implementation behave the same as the bluegiga implementation regarding duplicate data.

jlaur

Thanks a lot for your contribution! It looks very good. I have added a few initial comments.

Do you know if the protocol for Grundfos Alpha Reader is similar to the one for Grundfos Alpha3, i.e. pumps with built-in Bluetooth support? I have an Alpha3 and would love to help testing and contributing if this could be supported as well. I already did a backport of your binding to 4.0.x to see if might communicate with my pump through BlueZ (Raspberry Pi 3), but unfortunately without success.

In order to be extensible in the future, I would already propose to rename to GrundfosAlpha, i.e. drop "Reader". If Alpha3 support could be added, this could then be a new thing type within the same binding.

I believe the binding id might need to be org.openhab.binding.bluetooth.grundfosalpha (i.e. with a bluetooth prefix), but I will need to get back to that to be sure.

Can you also add I18N properties? See https://www.openhab.org/docs/developer/utils/i18n.html#generating-i18n-properties-file.

There are a few compiler warnings and checkstyle warnings to address. You could take a look at target/code-analysis/report.html.

bom/openhab-addons/pom.xml

bundles/org.openhab.binding.grundfosalphareader/README.md

bundles/org.openhab.binding.grundfosalphareader/src/main/resources/OH-INF/binding/binding.xml

bundles/org.openhab.binding.grundfosalphareader/src/main/resources/OH-INF/thing/thing-types.xml

tisoft · 2023-11-18T19:26:46Z

Thanks for the review, I will work on your issues.

Regarding Grundfos Alpha 3:
Support should be possible, since it does use the same BLE implementation. It might use a slightly different protocol. I can't check that myself since I don't have one.
Have you tried the official app from Grundfos to check if that can read out the values?

When that works, you could try to dump the bluetooth messages with:

sudo hcitool lescan --duplicate > /dev/null &
sudo hcidump --raw hci

And send me some of the messages, so that I can compare them to mine.

jlaur · 2023-11-18T21:39:05Z

Support should be possible, since it does use the same BLE implementation.

That sounds promising! Just to be clear, support for Alpha3 can be added in another PR as it's clearly out of scope for this one. But I'm very much interested in finally getting my pump online. 😄

It might use a slightly different protocol. I can't check that myself since I don't have one. Have you tried the official app from Grundfos to check if that can read out the values?

I can read a lot of data with Grundfos Go Remote:
https://play.google.com/store/apps/details?id=com.grundfos.go

https://play.google.com/store/apps/details?id=com.grundfos.go.balance&hl=de&gl=US

I tried to install this one, but already at first step it seemed like it would start some major calibration, so didn't dare to go further.

When that works, you could try to dump the bluetooth messages with:

sudo hcitool lescan --duplicate > /dev/null &

sudo hcidump --raw hci

And send me some of the messages, so that I can compare them to mine.

Thanks. I've sent some messages by private mail.

Perhaps this might also be helpful, I had a brief look at that when previously collecting messages and trying to figure them out:
https://github.com/esphome/esphome/tree/dev/esphome/components/alpha3

tisoft · 2023-11-19T11:02:19Z

I have renamed/moved the add-on to org.openhab.binding.bluetooth.grundfosalpha.

Could you help me with the following warnings? What do I have to do to fix them?

[WARNING] OH-INF.thing.thing-types.xml:[0]
Missing the supported bridge with id roaming
[WARNING] OH-INF.thing.thing-types.xml:[0]
Missing the supported bridge with id bluegiga
[WARNING] OH-INF.thing.thing-types.xml:[0]
Missing the supported bridge with id bluez

I had a quick look on your Alpha3 logs and I think they can be supported.

jlaur

Not much found on second pass, only some minor comments.

bundles/org.openhab.binding.bluetooth.grundfosalpha/README.md

.../org/openhab/binding/bluetooth/grundfosalpha/internal/GrundfosAlphaReaderHandlerFactory.java

bundles/org.openhab.binding.bluetooth.grundfosalpha/src/main/resources/OH-INF/addon/addon.xml

bundles/pom.xml

.../org/openhab/binding/bluetooth/grundfosalpha/internal/GrundfosAlphaReaderHandlerFactory.java

...rg/openhab/binding/bluetooth/grundfosalpha/internal/GrundfosAlphaReaderBindingConstants.java

...penhab/binding/bluetooth/grundfosalpha/internal/GrundfosAlphaReaderDiscoveryParticipant.java

jlaur · 2023-11-19T19:25:14Z

I have renamed/moved the add-on to org.openhab.binding.bluetooth.grundfosalpha.

Great!

Could you help me with the following warnings? What do I have to do to fix them?

[WARNING] OH-INF.thing.thing-types.xml:[0] Missing the supported bridge with id roaming [WARNING] OH-INF.thing.thing-types.xml:[0] Missing the supported bridge with id bluegiga [WARNING] OH-INF.thing.thing-types.xml:[0] Missing the supported bridge with id bluez

Sure, I will have a look.

I had a quick look on your Alpha3 logs and I think they can be supported.

That would be awesome, let me know how I can help. 🙂

jlaur · 2023-11-19T19:27:55Z

[WARNING] OH-INF.thing.thing-types.xml:[0] Missing the supported bridge with id roaming [WARNING] OH-INF.thing.thing-types.xml:[0] Missing the supported bridge with id bluegiga [WARNING] OH-INF.thing.thing-types.xml:[0] Missing the supported bridge with id bluez

Sure, I will have a look.

I think you are missing these in the thing type definition:

		<supported-bridge-type-refs>
			<bridge-type-ref id="roaming"/>
			<bridge-type-ref id="bluegiga"/>
			<bridge-type-ref id="bluez"/>
		</supported-bridge-type-refs>

jlaur · 2023-11-30T13:29:05Z

@tisoft - just in case you missed them, there are still a few small comments to resolve. We can merge right after that. :)

tisoft · 2023-11-30T13:55:24Z

Yeah I know. Was a bit busy with work the last days. Will get back to this as soon as possible.

tisoft · 2023-12-03T19:55:42Z

[WARNING] OH-INF.thing.thing-types.xml:[0] Missing the supported bridge with id roaming [WARNING] OH-INF.thing.thing-types.xml:[0] Missing the supported bridge with id bluegiga [WARNING] OH-INF.thing.thing-types.xml:[0] Missing the supported bridge with id bluez

Sure, I will have a look.

I think you are missing these in the thing type definition:
		<supported-bridge-type-refs>
			<bridge-type-ref id="roaming"/>
			<bridge-type-ref id="bluegiga"/>
			<bridge-type-ref id="bluez"/>
		</supported-bridge-type-refs>

Those definitions are already in the file. I just tried to build the plugin org.openhab.binding.bluetooth.radoneye and I get the same warnings. So maybe this is currently expected?

.../org/openhab/binding/bluetooth/grundfosalpha/internal/GrundfosAlphaDiscoveryParticipant.java

...nhab.binding.bluetooth.grundfosalpha/src/main/resources/OH-INF/i18n/grundfosalpha.properties

tisoft · 2023-12-04T08:56:15Z

I think I should have addressed all review comments. I have tested the binding again and it still works. 😄

Signed-off-by: Markus Heberling <[email protected]>

Co-authored-by: Jacob Laursen <[email protected]> Signed-off-by: Markus Heberling <[email protected]>

jlaur

Minor details, will commit them before merging the PR.

bundles/org.openhab.binding.bluetooth.grundfosalpha/README.md

Signed-off-by: Jacob Laursen <[email protected]>

jlaur

LGTM - thank you!

jlaur · 2023-12-05T14:51:09Z

@kaikreuzer - can you add @tisoft as contributor so he will be automatically assigned as code reviewer for the new Grundfos Alpha binding? 🙂

kaikreuzer · 2023-12-05T15:05:33Z

Sure, done!

* [Bluez] Disable automatic filtering of duplicate data * Added Grundfos Alpha binding Signed-off-by: Markus Heberling <[email protected]> Signed-off-by: Jørgen Austvik <[email protected]>

tisoft requested review from blafois, cpmeister and a team as code owners November 18, 2023 13:19

jlaur changed the title ~~[grundfosaphareader] Initial contribution~~ [grundfosalphareader] Initial contribution Nov 18, 2023

jlaur added the new binding If someone has started to work on a binding. For a new binding PR. label Nov 18, 2023

jlaur requested changes Nov 18, 2023

View reviewed changes

tisoft force-pushed the grundfos_alpha_main branch 2 times, most recently from 89a7e31 to 002c541 Compare November 18, 2023 20:21

tisoft force-pushed the grundfos_alpha_main branch from 002c541 to 6bd4ce9 Compare November 19, 2023 08:32

tisoft changed the title ~~[grundfosalphareader] Initial contribution~~ [grundfosalpha] Initial contribution Nov 19, 2023

jlaur requested changes Nov 19, 2023

View reviewed changes

tisoft force-pushed the grundfos_alpha_main branch 2 times, most recently from 071a51d to f0f6231 Compare November 24, 2023 06:53

tisoft force-pushed the grundfos_alpha_main branch from f0f6231 to a57d910 Compare December 3, 2023 19:50

tisoft force-pushed the grundfos_alpha_main branch from 75185d0 to fc7788d Compare December 3, 2023 20:00

jlaur reviewed Dec 3, 2023

View reviewed changes

.../org/openhab/binding/bluetooth/grundfosalpha/internal/GrundfosAlphaDiscoveryParticipant.java Outdated Show resolved Hide resolved

jlaur reviewed Dec 3, 2023

View reviewed changes

...nhab.binding.bluetooth.grundfosalpha/src/main/resources/OH-INF/i18n/grundfosalpha.properties Outdated Show resolved Hide resolved

tisoft requested a review from jlaur December 4, 2023 08:55

tisoft and others added 2 commits December 4, 2023 10:02

[Bluez] Disable automatic filtering of duplicate data

5e293e7

Signed-off-by: Markus Heberling <[email protected]>

Added Grundfos Alpha binding

17339b2

Co-authored-by: Jacob Laursen <[email protected]> Signed-off-by: Markus Heberling <[email protected]>

tisoft force-pushed the grundfos_alpha_main branch from 3c2934c to 17339b2 Compare December 4, 2023 09:04

jlaur requested changes Dec 4, 2023

View reviewed changes

bundles/org.openhab.binding.bluetooth.grundfosalpha/README.md Outdated Show resolved Hide resolved

bundles/org.openhab.binding.bluetooth.grundfosalpha/README.md Outdated Show resolved Hide resolved

bundles/org.openhab.binding.bluetooth.grundfosalpha/README.md Outdated Show resolved Hide resolved

Fix minor details in README

d5be464

Signed-off-by: Jacob Laursen <[email protected]>

jlaur approved these changes Dec 4, 2023

View reviewed changes

jlaur merged commit f5065ad into openhab:main Dec 4, 2023
3 checks passed

jlaur added this to the 4.1 milestone Dec 4, 2023

tisoft deleted the grundfos_alpha_main branch December 4, 2023 10:13

jlaur mentioned this pull request Dec 26, 2023

[grundfosalpha] Add support for Grundfos Alpha3 #16116

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[grundfosalpha] Initial contribution #15907

[grundfosalpha] Initial contribution #15907

tisoft commented Nov 18, 2023

tisoft commented Nov 18, 2023 •

edited

Loading

jlaur left a comment

tisoft commented Nov 18, 2023

jlaur commented Nov 18, 2023

tisoft commented Nov 19, 2023

jlaur left a comment

jlaur commented Nov 19, 2023

jlaur commented Nov 19, 2023

jlaur commented Nov 30, 2023

tisoft commented Nov 30, 2023

tisoft commented Dec 3, 2023

tisoft commented Dec 4, 2023

jlaur left a comment

jlaur left a comment

jlaur commented Dec 5, 2023 •

edited

Loading

kaikreuzer commented Dec 5, 2023

[grundfosalpha] Initial contribution #15907

[grundfosalpha] Initial contribution #15907

Conversation

tisoft commented Nov 18, 2023

tisoft commented Nov 18, 2023 • edited Loading

jlaur left a comment

Choose a reason for hiding this comment

tisoft commented Nov 18, 2023

jlaur commented Nov 18, 2023

tisoft commented Nov 19, 2023

jlaur left a comment

Choose a reason for hiding this comment

jlaur commented Nov 19, 2023

jlaur commented Nov 19, 2023

jlaur commented Nov 30, 2023

tisoft commented Nov 30, 2023

tisoft commented Dec 3, 2023

tisoft commented Dec 4, 2023

jlaur left a comment

Choose a reason for hiding this comment

jlaur left a comment

Choose a reason for hiding this comment

jlaur commented Dec 5, 2023 • edited Loading

kaikreuzer commented Dec 5, 2023

tisoft commented Nov 18, 2023 •

edited

Loading

jlaur commented Dec 5, 2023 •

edited

Loading