Input-plugin for KNX home-automation bus #7048

DocLambda · 2020-02-19T20:22:21Z

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

This pull request adds an input-plugin for the KNX standard used in home and building automation. Using a KNX-IP interface it is possible to wait for messages on that bus and handle them with the full power of telegraf.

As I'm pretty new to golang I like to get comments on the code especially:

What needs to be done to get this merged?
What can be simplified and/or written more go-ish?
Is the config acceptable or do you have any better idea about the layout?

I'm aware that some things (especially in knx_helpers.go) belong to the underlying knx-go project. Some things are already on their way but might need time, so bear with me on this end...

Looking forward to suggestions, critics, comments...

ssoroka

I think it looks pretty good. I've reviewed it mostly from the perspective of whether or not it looks like typical Go code, and left some comments related to that. They're mostly low priority, but would be nice to have.
I'll get another team member to look it over for any other concerns.
Thanks!

plugins/inputs/knx_listener/knx_helpers.go

plugins/inputs/knx_listener/knx_listener.go

plugins/inputs/knx_listener/knx_listener_test.go

LukasGrebe · 2020-11-18T08:26:32Z

Hi, what is the status of this Plugin?

I'd like to push IoT Data from a KNX Bus to Influx cloud.

it looks like there is an outstanding review, and also a failing integration test but that hasn't changed since February.

LukasGrebe

Looked though the readme and found a typo

plugins/inputs/knx_listener/README.md

DocLambda · 2020-11-22T17:13:13Z

@LukasGrebe I'll try to vamp up the code ad now there is potentially another user. :-) I have one request to you as I'm not sure about the config layout. Is this ok for you or can you think of anything better? Especially the grouping in measurements is unusual I guess... :-)

LukasGrebe · 2020-11-22T21:04:03Z

@LukasGrebe … about the config layout. Is this ok for you or can you think of anything better? Especially the grouping in measurements is unusual I guess... :-)

seems fine to me. maybe two thoughts:

It'd be nice to add arbitrary tags to a specific measurement or KNX groups - eg a room name/building section or other metadata
Wildcards in GAs would be useful as the Groups usually follow some sort of schema. eg "5/5/" or "5//2" instead of listing a huge array of GAs

combining 1 and 2 it would be great to have the KNX ids (GA, source) as individual tags in influxDB. eg. a Telegram from Source "1.2.3" to GA "4/5/6" would have 6 tags in Influx:
3 tags from the Physical Adress

area=1
line=2
bus device=3

and 3 tags from the group address

main group=4
middle group=5
sub group=6

(leaving other group address structures open for future improvements maybe)

this would allow arbitrary groupings for measurements in influxDB Queries as any desired data grouping could be achieved though these metadata labels. - but i'm new to influx. maybe this could be achieved with placeholders/partial matching of tag values in influxdb queries?

LukasGrebe · 2020-11-22T21:31:21Z

i'm missing dpt 7.X - 2-Octet Unsigned Value
specifically: Time Period - DPT_TimePeriodHrs 7.007 unit: h

If i understand vapourismo/knx-go#7 correctly, the data[] of https://godoc.org/github.com/vapourismo/knx-go/knx#GroupEvent could be used to work with 7.007h, albeit missing some convenience functions. - i'm not sure how to proceed here

LukasGrebe · 2020-11-28T07:44:10Z

@LukasGrebe I'll try to vamp up the code ad now there is potentially another user. :-)

id love to help with the unsuccessful CI checks.

continuous-integration/appveyor/pr — AppVeyor was unable to build non-mergeable pull request

but i wouldn't know where to start. I'm currently running telegraf on a dev machine, but id love to move it to a docker container on a server machine. Unfortunately i'm not allowed to run custom builds of Telegraf on that mashine, so i'd really appreciate this being part of the official repo.

DocLambda · 2020-12-07T20:52:52Z

@LukasGrebe did my best to make it in time. :-) Will try to get things in time but no promises. If you want to help, I found that some DPTs are missing in the registry of the upstream library, especially in the 14.xxx range...

zak-pawel

You also need to add proper line to LICENSE_OF_DEPENDENCIES.md (as make check-deps suggests)

plugins/inputs/knx_listener/README.md

plugins/inputs/knx_listener/knx_listener.go

plugins/inputs/knx_listener/knx_listener_test.go

plugins/inputs/knx_listener/README.md

zak-pawel

LGTM!

DocLambda · 2021-01-28T10:06:42Z

For the sake of completeness: The CI errors are (old) surricata and grok ones not originating from this plugin.

ivorybilled · 2021-03-03T14:45:28Z

plugins/inputs/knx_listener/knx_listener.go

+	go kl.listen()
+
+	return nil
+}
+
+func (kl *KNXListener) Stop() {
+	if kl.client != nil {
+		kl.client.Close()
+	}
+}


should there perhaps be a waitGroup used here and wait() called in the Stop() method so that listen() can be allowed to finish what it's doing if it's in the middle of something?

ivorybilled

overall lgtm

srebhan · 2021-03-04T08:12:02Z

!retry-failed

Co-authored-by: Paweł Żak <pawel.zak.pawel@gmail.com>

…uoted string format for GA.

srebhan · 2021-03-17T17:38:19Z

!retry-failed

srebhan · 2021-03-18T12:58:03Z

!retry-failed

ssoroka self-requested a review February 19, 2020 20:24

danielnelson added the new plugin label Feb 19, 2020

ssoroka reviewed Feb 20, 2020

View reviewed changes

ssoroka requested a review from danielnelson February 20, 2020 19:51

LukasGrebe suggested changes Nov 18, 2020

View reviewed changes

plugins/inputs/knx_listener/README.md Outdated Show resolved Hide resolved

LukasGrebe reviewed Nov 18, 2020

View reviewed changes

plugins/inputs/knx_listener/README.md Show resolved Hide resolved

DocLambda force-pushed the input/knx_listener branch from cebba3d to 27ee6d2 Compare December 7, 2020 20:38

DocLambda changed the title ~~[RFC] Input-plugin for KNX home-automation bus~~ Input-plugin for KNX home-automation bus Dec 7, 2020

DocLambda force-pushed the input/knx_listener branch from 27ee6d2 to 5314351 Compare December 7, 2020 20:50

zak-pawel suggested changes Dec 7, 2020

View reviewed changes

zak-pawel reviewed Dec 7, 2020

View reviewed changes

plugins/inputs/knx_listener/knx_listener_test.go Outdated Show resolved Hide resolved

zak-pawel reviewed Jan 21, 2021

View reviewed changes

plugins/inputs/knx_listener/README.md Outdated Show resolved Hide resolved

zak-pawel approved these changes Jan 21, 2021

View reviewed changes

zak-pawel added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jan 21, 2021

sjwang90 added the plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins label Jan 22, 2021

DocLambda requested review from ssoroka and removed request for danielnelson January 28, 2021 10:07

ivorybilled reviewed Mar 3, 2021

View reviewed changes

DocLambda force-pushed the input/knx_listener branch from 3930267 to 2724d5b Compare March 4, 2021 07:35

srebhan requested a review from ivorybilled March 4, 2021 08:12

ivorybilled approved these changes Mar 4, 2021

View reviewed changes

DocLambda and others added 23 commits March 17, 2021 08:35

Check for duplicate GA specification.

6cc5a52

Make GetKNXInterface() private.

5ca98e1

Move datapoint value construction to the only user and remove it.

4f2e363

Dissolve getKNXInterface() to the respective place.

887ab1f

Move KNXInterface declaration to main block.

50566f6

Remove superfluous sendRegularly() function.

7a7cbed

Rework test-cases, simplify and extend them.

3c36e96

Remove superfluous helpers.

dcb39bf

Fix formatting. No functional changes.

02e113d

Update plugins/inputs/knx_listener/README.md

115bd28

Co-authored-by: Paweł Żak <pawel.zak.pawel@gmail.com>

Replace unused accumulator parameter

7024f8c

Co-authored-by: Paweł Żak <pawel.zak.pawel@gmail.com>

Fix typo

efcc1f6

Co-authored-by: Paweł Żak <pawel.zak.pawel@gmail.com>

Use double-# for comments and make 'tunnel' the default type.

7573345

Use telegraf logger and improve quoted-string constructs.

d8c4583

Fix parameter order in type-conversion error-message and always use q…

0ea64cb

…uoted string format for GA.

Save an indentation-level.

64205bf

Avoid spamming the log with unknown group-addresses.

3cb2508

Fix tests for Windows where the time might be equal.

52317c3

Fix typo.

d712bc7

Add dependency to list.

5541540

Add knx to the list of input plugins.

80d28ed

Fix dependency lost during rebase.

ada8c37

Wait for the listener to finish when stopping the plugin.

ea5b502

DocLambda force-pushed the input/knx_listener branch from 2724d5b to ea5b502 Compare March 17, 2021 07:38

Fix linter error by explicitly specifying the return values.

5b1998a

reimda approved these changes Mar 18, 2021

View reviewed changes

reimda merged commit 1eb47e2 into influxdata:master Mar 18, 2021

jblesener pushed a commit to jblesener/telegraf that referenced this pull request Apr 18, 2021

Add input plugin for KNX home automation bus (influxdata#7048)

c41a71d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Input-plugin for KNX home-automation bus #7048

Input-plugin for KNX home-automation bus #7048

DocLambda commented Feb 19, 2020

ssoroka left a comment

LukasGrebe commented Nov 18, 2020 •

edited

Loading

LukasGrebe left a comment

DocLambda commented Nov 22, 2020

LukasGrebe commented Nov 22, 2020 •

edited

Loading

LukasGrebe commented Nov 22, 2020

LukasGrebe commented Nov 28, 2020

DocLambda commented Dec 7, 2020

zak-pawel left a comment

zak-pawel left a comment

DocLambda commented Jan 28, 2021

ivorybilled Mar 3, 2021

ivorybilled left a comment

srebhan commented Mar 4, 2021

srebhan commented Mar 17, 2021

srebhan commented Mar 18, 2021

Input-plugin for KNX home-automation bus #7048

Input-plugin for KNX home-automation bus #7048

Conversation

DocLambda commented Feb 19, 2020

Required for all PRs:

ssoroka left a comment

Choose a reason for hiding this comment

LukasGrebe commented Nov 18, 2020 • edited Loading

LukasGrebe left a comment

Choose a reason for hiding this comment

DocLambda commented Nov 22, 2020

LukasGrebe commented Nov 22, 2020 • edited Loading

LukasGrebe commented Nov 22, 2020

LukasGrebe commented Nov 28, 2020

DocLambda commented Dec 7, 2020

zak-pawel left a comment

Choose a reason for hiding this comment

zak-pawel left a comment

Choose a reason for hiding this comment

DocLambda commented Jan 28, 2021

ivorybilled Mar 3, 2021

Choose a reason for hiding this comment

ivorybilled left a comment

Choose a reason for hiding this comment

srebhan commented Mar 4, 2021

srebhan commented Mar 17, 2021

srebhan commented Mar 18, 2021

LukasGrebe commented Nov 18, 2020 •

edited

Loading

LukasGrebe commented Nov 22, 2020 •

edited

Loading