Skip to content

Add IBM Watson IoT Platform component#13664

Merged
balloob merged 1 commit intohome-assistant:devfrom
mtreinish:watson-iot
Jun 7, 2018
Merged

Add IBM Watson IoT Platform component#13664
balloob merged 1 commit intohome-assistant:devfrom
mtreinish:watson-iot

Conversation

@mtreinish
Copy link
Copy Markdown
Contributor

@mtreinish mtreinish commented Apr 3, 2018

Description:

This commit adds a new history component for the IBM Watson IoT
Platform. The IBM Watson IoT Platform allows for tracking of devices
and analytics on top of the device data. This new component allows
users to have home assistant automatically populate a watson iot
platform board with device data from devices managed by home assistant.

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5100

Example entry for configuration.yaml (if applicable):

watson_iot:
  organization: 'organization_id'
  type: 'device_type'
  id: 'device_id'
  token: 'auth_token'

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

Comment thread homeassistant/components/watson_iot.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

undefined name 'event'

Copy link
Copy Markdown

@stevemar stevemar left a comment

Choose a reason for hiding this comment

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

i came here out of interest to check out @mtreinish's PR, stumbled upon a typo or two. Neat stuff.

Comment thread homeassistant/components/watson_iot.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

typos: IoT* Platform*

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@ezar
Copy link
Copy Markdown

ezar commented Apr 4, 2018

Like it!

Comment thread .coveragerc Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are you touching xiaomi_miio.py ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was a rebase mistake, fixed in the next rev.

Comment thread homeassistant/components/watson_iot.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You already defined defaults, no need to use get with default just use []

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread homeassistant/components/watson_iot.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CONF_ORG is required, use conf[CONF_ORG]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread homeassistant/components/watson_iot.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

False ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You know I'm looking at this again, and I'm not sure what my intent was when I wrote this a month ago. False wouldn't make much sense because if we get a ValueError there _include_state will already be false. I can only assume I meant for it to be true, but I can't remember why. I'm just going to leave it as is, and if I remember what my intent was I'll add a comment here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well I think that using exception handling to implement logic is not the best approach... maybe you can rework this part of the code into something more readable and less prone to confusion.

Comment thread homeassistant/components/watson_iot.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Make 3 a constant declared in the beginning of the file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@balloob
Copy link
Copy Markdown
Member

balloob commented May 17, 2018

May I ask why you closed this? This was so close to being done 🏁

@mtreinish
Copy link
Copy Markdown
Contributor Author

The latest revision reworks the logic around setting the device state in the output json to be a bit more clear to address @dgomes comments.

Comment thread homeassistant/components/watson_iot.py Outdated

include = conf[CONF_INCLUDE]
exclude = conf[CONF_EXCLUDE]
whitelist_e = set(include.get(CONF_ENTITIES, []))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The empty list default value is already set in the config schema. Don't use dict.get if there are defaults in place. Just use dict[key].

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread homeassistant/components/watson_iot.py Outdated

client_args = {
'org': conf[CONF_ORG],
'type': conf.get(CONF_TYPE),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't use dict.get for required keys.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread homeassistant/components/watson_iot.py Outdated
self.gateway = gateway
self.gateway.connect()
self.event_to_json = event_to_json
self.max_tries = MAX_TRIES
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can use the constant directly. You don't need to store it in an instance attribute.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

This commit adds a new history component for the IBM Watson IoT
Platform. The IBM Watson IoT Platform allows for tracking of devices
and analytics on top of the device data. This new component allows
users to have home assistant automatically populate a watson iot
platform board with device data from devices managed by home assistant.
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

@dgomes
Copy link
Copy Markdown
Contributor

dgomes commented Jun 2, 2018

Thanks for coming back and finishing :)

@balloob balloob merged commit d14d2fe into home-assistant:dev Jun 7, 2018
@ghost ghost removed the Ready for review label Jun 7, 2018
@mtreinish mtreinish deleted the watson-iot branch June 7, 2018 17:57
balloob pushed a commit to home-assistant/home-assistant.io that referenced this pull request Jun 21, 2018
* Add documentation for the new watson_iot component

This commit adds documention for the new watson_iot component which was
added in home-assistant/core#13664
It links to the official documentation for setting up the cloud service
and generating the required auth information.

* ✏️ Missing comma

* Use configuration tags

This commit updates the formatting of the configuration section of the
documentation. Previously it manually formatted a list instead of
leveraging the templating format. This corrects that oversight and uses
the standard format.

* 🎨 Adds logo

* ⬆️ ha_release -> 0.68

* ✏️ Processed RFC's
balloob pushed a commit to home-assistant/home-assistant.io that referenced this pull request Jun 21, 2018
* Add documentation for the new watson_iot component

This commit adds documention for the new watson_iot component which was
added in home-assistant/core#13664
It links to the official documentation for setting up the cloud service
and generating the required auth information.

* ✏️ Missing comma

* Use configuration tags

This commit updates the formatting of the configuration section of the
documentation. Previously it manually formatted a list instead of
leveraging the templating format. This corrects that oversight and uses
the standard format.

* 🎨 Adds logo

* ⬆️ ha_release -> 0.68

* ✏️ Processed RFC's
@balloob balloob mentioned this pull request Jun 22, 2018
@gnkarn
Copy link
Copy Markdown

gnkarn commented Jun 28, 2018

it would be good to add more detail on a config example , as following the docs , the config fails in loading the component :
in my case the config is simple but couldnt make it to work

watson_iot:
  organization: 'xxxxx'
  type: 'gateway'
  id: 'watson-hass-xxxx'
  token: 'xxxxxxxx'
  include:
    entities:
      - sensor.home_energy_sensor_current

--
tks

@mtreinish
Copy link
Copy Markdown
Contributor Author

@gnkarn hmm, that config looks valid can you please open an issue with the details including the error in the logs when the component fails to load. (there likely is a stack trace in the logs if it's erroring on load) I'll take a look at it as soon as I can once you open it, there might be a bug in the include/exclude configuration I didn't test that bit as thoroughly as the rest of the component. But from a quick glance nothing looks wrong with that.

@robmarkcole
Copy link
Copy Markdown
Contributor

@mtreinish have you published anywhere the analysis you have performed on the Watson platform?

@gnkarn
Copy link
Copy Markdown

gnkarn commented Jun 28, 2018

@mtreinish there is nothing of help on the logs , as the configuration of the component : could not be loaded .
i have not deleting the include as i only want to publish a few sensors , starting with one .
tks

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Jun 28, 2018
@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 28, 2018

No support in pull requests. Open an issue as requested.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants