CPU Temperature (and similar values) support for Windows #1238

pgrawehr · 2020-10-30T17:27:19Z

This adds basic CPU temperature support for windows (unfortunately, it is not very generic, so it might work on your hardware or not).

A separate binding to get values from OpenHardwareMonitor (https://openhardwaremonitor.org/) is provided which gets a full set of whatever values you might want to know about your PC. This leverages the requirement to do anything hardware-dependent on our side. It is also a good example on how to work with UnitsNet.

Ellerbach

Thanks for this addition, really like it all up. Great usage of the UnitsNet especially to add calculated additional sensors. That makes life so easy.

src/Iot.Device.Bindings/Iot.Device.Bindings.csproj

src/devices/OpenHardwareMonitor/OpenHardwareMonitor.cs

Ellerbach · 2020-10-31T07:51:00Z

src/devices/OpenHardwareMonitor/OpenHardwareMonitor.cs

+        /// <exception cref="PlatformNotSupportedException">The operating system is not Windows.</exception>
+        public OpenHardwareMonitor()
+        {
+            if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))


As I read the Open Hardware Monitor dcumentation, I understand it's also working on Linux. Now, on Linux there is no WMI prodider, so clearly the logic would be different. So is the CpuTemperature for Linux largely enough or is there something that can be done for Linux as well. I doesn't mean it had to be done for this PR but it's somethings that can be done later.

I will check what it takes to use it on linux (and at least document that).

There seems to be some sort of support on linux, using mono. I haven't tried that, though. There's an alternate connection api that creates a small HTTP server that could be used, but this would need to be investigated separately.

src/devices/OpenHardwareMonitor/OpenHardwareMonitor.cs

Ellerbach · 2020-10-31T08:10:08Z

src/devices/OpenHardwareMonitor/OpenHardwareMonitor.cs

+                    return false;
+                }
+
+                float newValue = Convert.ToSingle(_instance["Value"]);


With this pattern, how do we ensure that the unit is created with the right default unit? Are _instance["Value"] always returning the default international unit? I may have miss something here, I would just like to understand

Yes, the values returned are always SI units. Which unit belongs to which sensor type is defined in the global _typeMap array.

src/devices/OpenHardwareMonitor/OpenHardwareMonitor.csproj

Ellerbach · 2020-10-31T08:13:20Z

src/devices/OpenHardwareMonitor/README.md

+- From the CPU Package power sensor, the CPU heat flux is calculated (estimating the size of the die).
+- If both a power and a voltage sensor are available for CPU Package, the current is calculated.
+
+## References


seems like you are missing the references

src/devices/OpenHardwareMonitor/SensorType.cs

krwq · 2020-11-02T08:45:35Z

src/devices/CpuTemperature/CpuTemperature.cs

@@ -3,29 +3,64 @@
 // See the LICENSE file in the project root for more information.

 using System;
+using System.Collections.Generic;


I think in this case because implementation is completely different I'd recommend to create platform specific files: CpuTemperature.Linux.cs, CpuTemperature.Windows.cs

Also you should not have public API differences between Windows/Linux, extra stuff can be available through Windows-only library though

The Iot.Devices library has no platform dependency (and shouldn't have one). Therefore the platform test is done at runtime, so that two files don't really add value, I think.

The API is exactly the same for all cases.

krwq · 2020-11-02T08:46:30Z

src/devices/CpuTemperature/CpuTemperature.csproj

@@ -10,4 +10,8 @@
    <None Include="README.md" />
  </ItemGroup>

+  <ItemGroup>
+    <PackageReference Include="System.Management" Version="5.0.0-alpha.1.19618.1" />


I'd recommend to make it Windows-only dependency

See above. There's no platform dependency on this dll.

Also, any platform dependency introduced at this level would apply to the source computer, not the one running it.

src/devices/CpuTemperature/README.md

src/devices/CpuTemperature/samples/CpuTemperature.Sample.cs

krwq · 2020-11-02T08:49:58Z

src/devices/OpenHardwareMonitor/OpenHardwareMonitor.cs

@@ -0,0 +1,740 @@
+using System;
+using System.Collections;
+using System.Collections.Generic;


should Windows implementation of CpuTemperature use this class instead of doing queries directly?

I considered this as well, but since this class requires an external application to be running, I tought that it might be good to not add that dependency to the basic CpuTemperature class. But since the current implementation of CpuTemperature often doesn't work (depending on hardware), it may be better to change it anyway.

So we have a solution (current implementation of CpuTemperature) that often doesn't work and needs elevation vs a solution that requires external components. Which do you prefer?

I think whichever one is more useful. We should start with making the code work with good APIs, next steps are perf and size

I'll try to use the OpenHardwareMonitor implementation on Windows now, and use the direct approach only as fallback.

src/devices/CpuTemperature/CpuTemperature.csproj

src/devices/HardwareMonitor/HardwareMonitor.csproj

src/Iot.Device.Bindings/Iot.Device.Bindings.csproj

joperezr · 2020-11-04T19:46:38Z

Sorry @pgrawehr I didn't have time yesterday to push the change but I just did, so that should now use the right 5.0 versions of the packages that will ship in 5.0

pgrawehr · 2020-11-04T20:49:46Z

@joperezr No problem, I've got other loose ends to work on ;-)

Unfortunately, it looks as if you've broken the build now (something about package downgrading). Did you miss an occurence?

joperezr · 2020-11-04T21:00:40Z

Oops, my bad, looking into it now.

joperezr · 2020-11-04T21:49:12Z

I'll rebase to fix the merge conflict

Downside: Requires elevated permissions

Properties should not have side effects

On Windows, the example requires elevated permissions. It may still not find a suitable sensor, dependent on the hardware.

Also changed namespace, to prevent namespace to class name collisions

And add missing Dispose

Latest Version not available on official sources yet

…ew package subscription

joperezr · 2020-11-05T21:40:55Z

@richlander given you are the nullable expert in the repo, let me know if you have any feedback on the last commit changes: 855a8e6

richlander · 2020-11-05T21:50:33Z

Would be nice to see this marked in some way to remove the ! when we stop targeting netcoreapp2.1. We want as much compiler checking as we can get.

855a8e6#diff-962c5fd4c57ced02999e704a7a51c628ec345eacb38dcd0bc3fb83a2d2056eafR461

An #if and #elif would certainly resolve that.

joperezr

PR looks good to me, thanks for the contribution @pgrawehr, I'll go ahead and merge once this is green in order to also get the update on the package dependency versions in.

* Add basic CpuTemperature implementation for Windows Downside: Requires elevated permissions * Clean init sequence Properties should not have side effects * Add application manifest for Windows On Windows, the example requires elevated permissions. It may still not find a suitable sensor, dependent on the hardware. * Update readme * Add new binding for OpenHardwareMonitor connection * Clean up * Add documentation * Better documentation for TimeSpan constants * Allow arbitrary thread cycle times * Value must not be negative * More generic binding name Also changed namespace, to prevent namespace to class name collisions * Use HardwareMonitor on Windows, if available And add missing Dispose * Use more recent version of System.Management Latest Version not available on official sources yet * Minor documentation improvements * Updating dependency package versions from dotnet/runtime and adding new package subscription * Fixing build break due to dependencies * Removing restore sources so that NuGet.Config is used * Rebasing and addressing nullability changes * Addressing feedback Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com>

ghost added the area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio label Oct 30, 2020

pgrawehr force-pushed the CpuTemperature branch from c2a1d44 to adf5ecf Compare October 30, 2020 20:16

Ellerbach reviewed Oct 31, 2020

View reviewed changes

pgrawehr mentioned this pull request Oct 31, 2020

Arduino/Firmata device support #1039

Merged

krwq reviewed Nov 2, 2020

View reviewed changes

src/devices/CpuTemperature/README.md Outdated Show resolved Hide resolved

krwq reviewed Nov 2, 2020

View reviewed changes

src/devices/CpuTemperature/samples/CpuTemperature.Sample.cs Outdated Show resolved Hide resolved

krwq reviewed Nov 2, 2020

View reviewed changes

joperezr reviewed Nov 3, 2020

View reviewed changes

src/devices/CpuTemperature/CpuTemperature.csproj Outdated Show resolved Hide resolved

joperezr reviewed Nov 3, 2020

View reviewed changes

src/devices/HardwareMonitor/HardwareMonitor.csproj Outdated Show resolved Hide resolved

joperezr reviewed Nov 3, 2020

View reviewed changes

src/Iot.Device.Bindings/Iot.Device.Bindings.csproj Outdated Show resolved Hide resolved

joperezr force-pushed the CpuTemperature branch from 767e7bd to 11de1a6 Compare November 4, 2020 21:54

joperezr mentioned this pull request Nov 5, 2020

[master] Update dependencies from dotnet/runtime #1255

Closed

pgrawehr and others added 12 commits November 5, 2020 11:48

Add basic CpuTemperature implementation for Windows

4c51ea0

Downside: Requires elevated permissions

Clean init sequence

eaa5f69

Properties should not have side effects

Add application manifest for Windows

de74a2b

On Windows, the example requires elevated permissions. It may still not find a suitable sensor, dependent on the hardware.

Update readme

37adeb9

Add new binding for OpenHardwareMonitor connection

77a7063

Clean up

77c1bb1

Add documentation

ff95d7a

Better documentation for TimeSpan constants

a0a7eb3

Allow arbitrary thread cycle times

53bdb56

Value must not be negative

ae6d7c9

More generic binding name

05882ee

Also changed namespace, to prevent namespace to class name collisions

Use HardwareMonitor on Windows, if available

658f0cf

And add missing Dispose

pgrawehr and others added 6 commits November 5, 2020 11:52

Use more recent version of System.Management

e7bfa5f

Latest Version not available on official sources yet

Minor documentation improvements

4a1b322

Updating dependency package versions from dotnet/runtime and adding n…

6ef71ab

…ew package subscription

Fixing build break due to dependencies

a84660e

Removing restore sources so that NuGet.Config is used

94d727e

Rebasing and addressing nullability changes

855a8e6

joperezr force-pushed the CpuTemperature branch from b2508ea to 855a8e6 Compare November 5, 2020 21:40

Addressing feedback

2aa6eac

joperezr approved these changes Nov 9, 2020

View reviewed changes

joperezr merged commit 17d497e into dotnet:master Nov 9, 2020

github-actions bot locked and limited conversation to collaborators Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPU Temperature (and similar values) support for Windows #1238

CPU Temperature (and similar values) support for Windows #1238

pgrawehr commented Oct 30, 2020

Ellerbach left a comment

Ellerbach Oct 31, 2020

pgrawehr Oct 31, 2020

pgrawehr Nov 3, 2020

Ellerbach Oct 31, 2020

pgrawehr Nov 3, 2020

Ellerbach Oct 31, 2020

krwq Nov 2, 2020

pgrawehr Nov 2, 2020

krwq Nov 2, 2020

pgrawehr Nov 2, 2020

pgrawehr Nov 2, 2020

krwq Nov 2, 2020

pgrawehr Nov 2, 2020

krwq Nov 2, 2020

pgrawehr Nov 3, 2020

joperezr commented Nov 4, 2020

pgrawehr commented Nov 4, 2020

joperezr commented Nov 4, 2020

joperezr commented Nov 4, 2020

joperezr commented Nov 5, 2020

richlander commented Nov 5, 2020

joperezr left a comment

CPU Temperature (and similar values) support for Windows #1238

CPU Temperature (and similar values) support for Windows #1238

Conversation

pgrawehr commented Oct 30, 2020

Ellerbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joperezr commented Nov 4, 2020

pgrawehr commented Nov 4, 2020

joperezr commented Nov 4, 2020

joperezr commented Nov 4, 2020

joperezr commented Nov 5, 2020

richlander commented Nov 5, 2020

joperezr left a comment

Choose a reason for hiding this comment