Skip to content

Add additional needles to glances cpu_temp attribute#22311

Merged
fabaff merged 3 commits into
home-assistant:devfrom
shutupflanders:add-glances-cpu-temp-types
Sep 19, 2019
Merged

Add additional needles to glances cpu_temp attribute#22311
fabaff merged 3 commits into
home-assistant:devfrom
shutupflanders:add-glances-cpu-temp-types

Conversation

@shutupflanders
Copy link
Copy Markdown
Contributor

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.

@shutupflanders shutupflanders requested a review from fabaff as a code owner March 22, 2019 23:30
@ghost ghost added the in progress label Mar 22, 2019
@fabaff
Copy link
Copy Markdown
Member

fabaff 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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

fabaff 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
Copy link
Copy Markdown
Contributor Author

shutupflanders 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
Copy link
Copy Markdown
Contributor Author

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

@shutupflanders
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor Author

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?

@homeassistant
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Member

balloob 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
@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 21, 2019

@fabaff ?

@MartinHjelmare MartinHjelmare added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Sep 7, 2019
@MartinHjelmare
Copy link
Copy Markdown
Member

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

@balloob
Copy link
Copy Markdown
Member

balloob commented Sep 18, 2019

I pinged @fabaff on Discord

Copy link
Copy Markdown
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

Can be merged after the conflict is solved.

@fabaff fabaff merged commit 5e15675 into home-assistant:dev Sep 19, 2019
@MartinHjelmare MartinHjelmare removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Sep 19, 2019
@shutupflanders shutupflanders deleted the add-glances-cpu-temp-types branch September 19, 2019 08:43
@balloob balloob mentioned 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

None yet

Development

Successfully merging this pull request may close these issues.

6 participants