Skip to content
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

Add System monitoring component #10130

Closed
wants to merge 7 commits into from
Closed

Add System monitoring component #10130

wants to merge 7 commits into from

Conversation

fabaff
Copy link
Member

@fabaff fabaff commented Oct 25, 2017

Description:

This is a limited prototype/proposal for a system_monitoring component. As mentioned in #10118 should this help us to avoid the issue with the naming as the names are handled by the component through a dict lookup. Also, units of measurement and icons would be identical across platforms.

Depends on: #10555

Related issue (if applicable): fixes #10118

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

system_monitoring:
  - platform: cpuspeed
  - platform: demo

Checklist:

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • 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.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

raise NotImplementedError()

@property
def state(self):
Copy link
Member

Choose a reason for hiding this comment

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

I'd leave this not overwritten and also remove the value property, and let platforms overwrite state.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that my intention was not clear here and the current state of this PR is not reflecting this. Platforms should not longer need to take care about converting or rounding the data. The component will process or preparing the data which then are used for the state.

'memory_used': ['Memory used', 'MiB', 'mdi:memory'],
}

from pint import UnitRegistry

Choose a reason for hiding this comment

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

module level import not at top of file

@balloob
Copy link
Member

balloob commented Nov 24, 2017

I'm not a fan of this approach. I don't see any benefit compared to the sensor component. If we want names and units to be consistent, can't we just have system monitoring platforms do that? (also, how many system platform monitoring components do we need?)

@balloob balloob closed this Dec 8, 2017
@fabaff
Copy link
Member Author

fabaff commented Dec 12, 2017

We have system monitoring sensor platforms for that and we use them. With the introduction of this component we would have been capable to offload the whole conversion and round process from the platforms to the component. Also, it would have been possible for users set the unit of measurement according their needs.

also, how many system platform monitoring components do we need?

The more interesting questions is not how many platforms we need but how many we already have and can profit. The cpuspeed platform is boring as there is only the frequency of the chip which comes in Hz. The netdata platform is quite limited because of it's hard-coded entities and the systemmonitor has to perform calculations on some values.

I will not forget this and re-open it in 2018 when I spend more time thinking about additional benefits.

@balloob balloob deleted the system-monitoring branch February 18, 2018 23:04
@fabaff fabaff mentioned this pull request Mar 12, 2018
2 tasks
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
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.

System monitoring platforms
6 participants