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
Iota wallet #11398
Iota wallet #11398
Conversation
Hi @jinnerbichler, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
"""Fetch new attribures IRI node.""" | ||
node_info = self.api.get_node_info() | ||
self._state = node_info.get('appVersion') | ||
self._attr = {k: str(v) for k, v in node_info.items()} # convert values to raw string formats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (102 > 79 characters)
"""Initialize the sensor.""" | ||
|
||
super().__init__(name='Node Info', seed=None, | ||
iri=iota_config['iri'], is_testnet=iota_config['is_testnet']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (86 > 79 characters)
"""Initialize the sensor.""" | ||
|
||
super().__init__(name=wallet_config['name'], seed=wallet_config['seed'], | ||
iri=iota_config['iri'], is_testnet=iota_config['is_testnet']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (86 > 79 characters)
def __init__(self, wallet_config, iota_config): | ||
"""Initialize the sensor.""" | ||
|
||
super().__init__(name=wallet_config['name'], seed=wallet_config['seed'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (80 > 79 characters)
|
||
# Add sensors for wallet balance | ||
iota_config = hass.data[IOTA_DOMAIN] | ||
balance_sensors = [IotaBalanceSensor(wallet, iota_config) for wallet in iota_config['wallets']] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (99 > 79 characters)
@@ -0,0 +1,97 @@ | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is only a sensor. This doesn't require to include a component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, not yet. I am going to add more features to this component. Is it necessary to be a sensor only PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's the right way to create a component if there is coming a binary sensor or switch platform which can share the common stuff. Otherwise everything can be handled in the platform itself as no sharing is needed.
add_devices(balance_sensors) | ||
|
||
# Add sensor for node information | ||
add_devices([IotaNodeSensor(iota_config=iota_config)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add all devices in one go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
"""Initialize the sensor.""" | ||
super().__init__(name='Node Info', seed=None, iri=iota_config['iri'], | ||
is_testnet=iota_config['is_testnet']) | ||
self._state = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unknown state should be initialized with None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
super().__init__(name='Node Info', seed=None, iri=iota_config['iri'], | ||
is_testnet=iota_config['is_testnet']) | ||
self._state = "" | ||
self._attr = dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Init with {}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
homeassistant/components/iota.py
Outdated
@property | ||
def should_poll(self): | ||
"""Get polling requirement from IOTA device.""" | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx. removed it
homeassistant/components/iota.py
Outdated
|
||
CONFIG_SCHEMA = vol.Schema({ | ||
DOMAIN: vol.Schema({ | ||
vol.Required(CONF_IRI): vol.All(str), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use cv.string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove vol.All
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
homeassistant/components/iota.py
Outdated
} | ||
|
||
# Set states of IRI config | ||
hass.states.set('iota.iri', hass.data[DOMAIN]['iri']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want to set these states?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the URL of the IOTA node to connect to, which may change. It is a separate state in order to see current URL of the node in the UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Report it as a state attribute on the node sensor entity instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please report the two config settings as state attributes instead.
homeassistant/components/iota.py
Outdated
|
||
# Set states of IRI config | ||
hass.states.set('iota.iri', hass.data[DOMAIN]['iri']) | ||
hass.states.set('iota.is_testnet', hass.data[DOMAIN]['is_testnet']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment. Otherwise looks good.
homeassistant/components/iota.py
Outdated
"""Setup IOTA component.""" | ||
# Set domain sepecific data | ||
iota_config = config[DOMAIN] | ||
hass.data[DOMAIN] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configuration is supposed to be past in discovery info dict, ie fourth parameter of load_platform
. Only non serializable objects need to go in hass.data
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx. Changed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now you don't need to store the iota_config
in hass.data
. You also need to change setup_platform
to access the discovery_info
instead of hass.data
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
homeassistant/components/iota.py
Outdated
|
||
# Load platforms | ||
for platform in IOTA_PLATFORMS: | ||
load_platform(hass, platform, DOMAIN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass the iota_config
as the fourth argument and the original config as the fifth argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proper arguments should be passed now.
@property | ||
def unit_of_measurement(self): | ||
"""Return the unit of measurement.""" | ||
return '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a valid unit of measurement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it
|
||
# add values of iri config | ||
self._attr['url'] = self.iri | ||
self._attr['testnet'] = self.is_testnet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to update this as it will never change. Set it when you initialize the sensor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
seed=wallet_config['seed'], | ||
iri=iota_config['iri'], | ||
is_testnet=iota_config['is_testnet']) | ||
self._state = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set it to None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
self._state = self.api.get_inputs()['totalBalance'] | ||
|
||
|
||
class IotaNodeSensor(IotaDevice): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that this should be a binary sensor. A node can be offline or online.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, a node can have multiple attributes, which are changing over time (https://iota.readme.io/v1.2.0/reference#getnodeinfo)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attributes and state are not the same. A binary sensor can have attributes as well. If a node is always present then it's doesn't make sense.
There are a dozen values available from getNodeInfo. Those details should be exposed as attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Information about the node is part of the attributes of the node sensor and should be part of the wallet.
def update(self): | ||
"""Fetch new attribures IRI node.""" | ||
node_info = self.api.get_node_info() | ||
self._state = node_info.get('appVersion') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is it useful to know the appVersion
of a node as state? Will that change on a regular base? If not then it's just another attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of appVersion
tells if the version of the node is up-to-date. Yes, the version of the node changes almost every other week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would still require an additional part because it only shows the version and not if the node is up-to-date.
@property | ||
def device_state_attributes(self): | ||
"""Return the state attributes of the device.""" | ||
return self._attr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment this will stay empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed that line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* upstream/master: (465 commits) Update pyhomematic to 0.1.38 (home-assistant#11936) Implement Alexa temperature sensors (home-assistant#11930) Fixed rfxtrx binary_sensor KeyError on missing optional device_class (home-assistant#11925) Allow setting climate devices to AUTO mode via Google Assistant (home-assistant#11923) fixes home-assistant#11848 (home-assistant#11915) Add "write" service to system_log (home-assistant#11901) Update frontend to 20180126.0 Version 0.62 Allow separate command and state OIDs and payloads in SNMP switch (home-assistant#11075) Add ERC20 tokens to etherscan.io sensor (home-assistant#11916) Report scripts and groups as scenes to Alexa (home-assistant#11900) Minor fix to configuration validation and related log line. (home-assistant#11898) Multi-Room Support for Greenwave Reality (home-assistant#11364) Added Xeoma camera platform (home-assistant#11619) Improve foscam library exception support (home-assistant#11701) Iota wallet (home-assistant#11398) New venstar climate component (home-assistant#11639) Update python-wink version and multiple wink fixes/updates. (home-assistant#11833) Use API to discover Hue if no bridges specified (home-assistant#11909) Clarify emulated hue warning (home-assistant#11910) ...
Description:
Added a new platform for IOTA. A cryptocurrency specialized for IoT use cases (including smart home applications :) )
Pull request in home-assistant.github.io with documentation: home-assistant/home-assistant.io#4314
Example entry for
configuration.yaml
:Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass