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 additional needles to glances cpu_temp attribute #22311

Merged
merged 3 commits into from Sep 19, 2019

Conversation

@shutupflanders
Copy link
Contributor

commented Mar 22, 2019

Description:

Added additional keys in the section that checks for the "cpu_temp" attribute from the Glances API with a fallback to "Core 0" to at least provide some temperature information.

Checklist:

  • [ Y] The code change is tested and works locally.
  • [ Y] Local tests pass with tox. Your PR cannot be merged unless tests pass
  • [ Y] There is no commented out code in this PR.
@fabaff

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

On which hardware/architecture does this occur? I though that by now we have covered all available CPU architectures...

@fabaff fabaff self-assigned this Mar 23, 2019
@shutupflanders

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

I have a gigabyte brix and a Lenovo q180, couldn't tell you the architecture off the top of my head, but in testing it pulled through both, whereas I was getting a blank value on the main release

@fabaff

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

This should be ordinary hardware (x86_64). Both should expose the temperature through Package id 0.

What do you get if you run?

$ curl http://IP_ADDRESS:61208/api/3/sensors
@shutupflanders

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

I get Core 0, Core 1, and one of the other strings I've added depending on which box I run it against, I only get Package 0 on my NUC

@shutupflanders

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

I'll drop the full responses in later when I'm at my desk

@shutupflanders

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

Here's the response from the sensors endpoint on 3 of my machines that were reporting nothing prior to my changes:

Q180

[
  {
    "key": "label",
    "type": "temperature_core",
    "unit": "C",
    "value": 74,
    "label": "Core 0"
  },
  {
    "key": "label",
    "type": "temperature_core",
    "unit": "C",
    "value": 73,
    "label": "Core 1"
  },
  {
    "key": "label",
    "type": "temperature_core",
    "unit": "C",
    "value": 68,
    "label": "radeon 1"
  }
]

Proxmox server (Ryzen 7)

[
  {
    "key": "label",
    "type": "temperature_core",
    "unit": "C",
    "value": 23,
    "label": "amdgpu 1"
  }
]

Brix (Core 0 fallback needed for this)

[
  {
    "key": "label",
    "type": "temperature_core",
    "unit": "C",
    "value": 26,
    "label": "acpitz 1"
  },
  {
    "key": "label",
    "type": "temperature_core",
    "unit": "C",
    "value": 39,
    "label": "Core 0"
  },
  {
    "key": "label",
    "type": "temperature_core",
    "unit": "C",
    "value": 39,
    "label": "Core 1"
  },
  {
    "key": "label",
    "type": "temperature_core",
    "unit": "C",
    "value": 41,
    "label": "Core 2"
  },
  {
    "key": "label",
    "type": "temperature_core",
    "unit": "C",
    "value": 41,
    "label": "Core 3"
  }
]

If you need any more info, please let me know.

@shutupflanders

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

Also, I added both my email accounts to my github, but the cla error hasn't gone away, not sure if that's set to check again or if it's a manual thing I'm supposed to trigger?

@robbiet480 robbiet480 added cla-recheck and removed cla-error labels Mar 26, 2019
@homeassistant

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Hi @shutupflanders,

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!

@balloob

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

@fabaff can you make a decision what to do with this PR, can we merge, do we require changes or do we not want this change?

@MartinHjelmare MartinHjelmare changed the title Added additional needles to the cpu_temp attribute Add additional needles to glances cpu_temp attribute Jun 19, 2019
@MartinHjelmare MartinHjelmare added this to In progress in Dev Jul 23, 2019
@balloob

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

@project-bot project-bot bot moved this from Needs review to Second opinion wanted in Dev Sep 7, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

What should we do here? This is six months old.

@balloob

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

I pinged @fabaff on Discord

@fabaff
fabaff approved these changes Sep 19, 2019
Copy link
Member

left a comment

Can be merged after the conflict is solved.

Dev automation moved this from Second opinion wanted to Reviewer approved Sep 19, 2019
fabaff added 2 commits Sep 19, 2019
@fabaff fabaff merged commit 5e15675 into home-assistant:dev Sep 19, 2019
11 checks passed
11 checks passed
CI Build #20190919.5 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 873d331...44a6873
Details
codecov/project 94% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 19, 2019
@shutupflanders shutupflanders deleted the shutupflanders:add-glances-cpu-temp-types branch Sep 19, 2019
ochlocracy added a commit to ochlocracy/home-assistant that referenced this pull request Sep 30, 2019
…22311)

* Added additional needles to the cpu_temp attribute

* Fix conflict
ochlocracy added a commit to ochlocracy/home-assistant that referenced this pull request Oct 1, 2019
…22311)

* Added additional needles to the cpu_temp attribute

* Fix conflict
@balloob balloob referenced this pull request Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Done
6 participants
You can’t perform that action at this time.