feat(inputs.huebridge): Make inputs.huebridge an internal plugin #16352
Conversation
telegraf-tiger
bot
added
feat
… and test logging
There was a problem hiding this comment.
Thanks @hdecarne for your contribution and sorry for this taking so long! I do have a very first comments in the code.
Additionally, I suggest splitting the code into some more files to make the plugin code more readable. How about defining a bridge type for handling all the discovery stuff, resource gathering and actual data gathering and keep the plugin code to manage the calls to the different bridges? The new type could then more to an own file for readability...
| This input plugin gathers status from [Hue Bridge][1] devices. | ||
| It uses the device's [CLIP API][2] interface to retrieve the status. | ||
|
|
||
| [1]: https://www.philips-hue.com/ | ||
| [2]: https://developers.meethue.com/develop/hue-api-v2/ |
There was a problem hiding this comment.
| This input plugin gathers status from [Hue Bridge][1] devices. | |
| It uses the device's [CLIP API][2] interface to retrieve the status. | |
| [1]: https://www.philips-hue.com/ | |
| [2]: https://developers.meethue.com/develop/hue-api-v2/ | |
| This input plugin gathers status from [Hue Bridge][hue] devices | |
| using the [CLIP API][hue_api] interface of the devices. | |
| ⭐ Telegraf v1.34.0 | |
| 🏷️ iot | |
| 💻 all | |
| [hue]: https://www.philips-hue.com/ | |
| [hue_api]: https://developers.meethue.com/develop/hue-api-v2/ |
| Retrieved status are: | ||
|
|
||
| - Light status (on|off) | ||
| - Temperatures | ||
| - Light levels | ||
| - Motion sensors | ||
| - Device power status (battery level and state) | ||
|
|
There was a problem hiding this comment.
Not sure if we keep this as an excessive list here as this might easily get outdated if we gather more information later...
| ## The Hue bridges to query, each identified via an URL of the following form: | ||
| ## <locator scheme>://<bridge id>:<user name>@<locator dependent address>/ | ||
| ## where: | ||
| ## <locator scheme> is one of | ||
| ## - address: To identify the bridge via the DNS name or ip address within the | ||
| ## URLs address part (see example below). | ||
| ## - cloud: To identify the bridge via its cloud registration. The address | ||
| ## part defines the discovery endpoint. If empty the standard endpoint | ||
| ## https://discovery.meethue.com/ is used. | ||
| ## - mdns: To identify the bridge via mDNS. The URL's address part is always | ||
| # empty in this case. | ||
| ## - remote: To identify the bridge via the Cloud Remote API. The address part | ||
| ## defines the cloud API endpoint. If empty the standard endpoint | ||
| ## https://api.meethue.com/ is used. | ||
| ## <bridge id> is the unique bridge id as returned in | ||
| ## curl -k https://<bridge address>/api/config/0 | ||
| ## <user name> is the secret user name returned during application authentication. | ||
| ## To create a new user name issue the following command after pressing the | ||
| ## bridge's link button: | ||
| ## curl -k -X POST http://<bridge address>/api \ | ||
| ## -H 'Content-Type: application/json' \ | ||
| ## -d '{"devicetype":"huebridge-telegraf-plugin"}' | ||
| ## Examples: | ||
| ## - "address://0123456789ABCDEF:sFlEGnMAFXO6RtZV17aViNUB95G2uXWw64texDzD@mybridge/" | ||
| ## - "cloud://0123456789ABCDEF:sFlEGnMAFXO6RtZV17aViNUB95G2uXWw64texDzD@/" | ||
| ## - "mdns://0123456789ABCDEF:sFlEGnMAFXO6RtZV17aViNUB95G2uXWw64texDzD@/" | ||
| ## - "remote://0123456789ABCDEF:sFlEGnMAFXO6RtZV17aViNUB95G2uXWw64texDzD@/" |
There was a problem hiding this comment.
Can we shorten this to a minimum? The reason is that the example config file is already quite long and having brief description eases navigation. You can add the more detailed description the the README and reference it in the sample-configuration. What do you think?
| bridges = [ | ||
| ] |
There was a problem hiding this comment.
How about putting one example in here?
| bridges = [ | |
| ] | |
| bridges = [ "address://<bridge id>:<username>@mybridge/" ] |
| ## The remote parameters to use to access a bridge remotely. | ||
| ## To access a bridge remotely a Hue Developer Account is required, a Remote | ||
| ## App must be registered and the corresponding Authorization flow must be | ||
| ## completed. See https://developers.meethue.com/develop/hue-api-v2/cloud2cloud-getting-started/ | ||
| ## for further details. | ||
| ## The Remote App's client id, client secret and callback url must be entered | ||
| ## here exactly as used within the App registration. | ||
| ## The remote_token_dir points to the directory receiving the token data. |
There was a problem hiding this comment.
Please shorten the comment sections in the sample-configuration and move it to the README if necessary. Same for all other comment sections...
| return sampleConfig | ||
| } | ||
|
|
||
| func (plugin *HueBridge) Init() error { |
There was a problem hiding this comment.
You could shorten the code by using
| func (plugin *HueBridge) Init() error { | |
| func (h *HueBridge) Init() error { |
| } | ||
|
|
||
| func (plugin *HueBridge) Init() error { | ||
| apilog.RedirectRootLogger(&wrappedLog{log: plugin.Log}, false) |
There was a problem hiding this comment.
Do we really need to redirect the logging? I would suggest to just drop the API's internal logs...
| type wrappedLog struct { | ||
| log telegraf.Logger | ||
| } | ||
|
|
||
| func (wrapped *wrappedLog) Write(p []byte) (n int, err error) { | ||
| return wrapped.WriteLevel(zerolog.DebugLevel, p) | ||
| } | ||
|
|
||
| func (wrapped *wrappedLog) WriteLevel(level zerolog.Level, p []byte) (n int, err error) { | ||
| switch level { | ||
| case zerolog.PanicLevel: | ||
| wrapped.log.Error(string(p)) | ||
| case zerolog.ErrorLevel: | ||
| wrapped.log.Error(string(p)) | ||
| case zerolog.WarnLevel: | ||
| wrapped.log.Warn(string(p)) | ||
| case zerolog.InfoLevel: | ||
| wrapped.log.Info(string(p)) | ||
| case zerolog.DebugLevel: | ||
| wrapped.log.Debug(string(p)) | ||
| default: | ||
| wrapped.log.Trace(string(p)) | ||
| } | ||
| return len(p), nil | ||
| } |
There was a problem hiding this comment.
How about moving this to an own file if we really need it?
| for _, bridge := range plugin.Bridges { | ||
| bridgeUrl, err := url.Parse(bridge) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Please use unique error prefixes so we can easily find the code spot in case users report issues...
| return err | |
| return fmt.Errorf("parsing bridge URL failed: %w", err) |
Same for all other spots.
| func (plugin *HueBridge) initBridge(bridgeUrl *url.URL) error { | ||
| bridgeClient, err := plugin.resolveBridge(bridgeUrl) | ||
| err, unrecoverable := unwrapUnrecoverableError(err) | ||
| if unrecoverable { | ||
| return err | ||
| } else if err != nil { | ||
| plugin.Log.Warnf("Unable to resolve bridge URL %q (reason: %s)", bridgeUrl, err) | ||
| } | ||
| plugin.resolvedBridges[bridgeUrl] = bridgeClient | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Please move the code to the caller location. It's okay to have a bit longer Init() function but at least you can follow of what is done easily.
Summary
This PR provides the inputs.huebridge plugin for collecting Smart Home statistics.
It gathers status from Hue Bridge devices using the device's CLIP API.
I previously contributed this plugin as an external one. As part of the latest reworks I also converted it into an internal one.
Checklist
Related issues
resolves #16351