I’m a noob and can’t fugure out how to make this code more efficient

SOLVED. I just needed to add using System.Threading; Everyone online said that you needed to add using System.Threading.Tasks; but it was useless. Putting using System.Threading; is what fixed it.

So I want to have a 7 segment display inside my computer that tells the temperature of the CPU package. I did this:

>Console app which only runs in the background. It extracts the cpu temperature with a dll from https://openhardwaremonitor.org/ and sends it to the com port 3.

Arduino nano. It is housed inside the pc, connected to an internal USB connector. Its code reads the serial port and prints it to a 4 digit 7 segment display.

The Arduino code is done and runs perfectly. The console app runs, but not only does it run faster than I’ll ever need but if I run it from visual studio it throws out these three exceptions constantly:
Exception thrown: 'System.OverflowException' in OpenHardwareMonitorLib.dll
Exception thrown: 'System.ArgumentOutOfRangeException' in OpenHardwareMonitorLib.dll
Exception thrown: 'System.ArgumentNullException' in mscorlib.dll

It uses up to 6% of the CPU, and I want to make it way cheaper on resources so:
>I want to make it run only once per second or two
>Maybe fix these exceptions if it isn’t very complicated

Here’s the code:
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using OpenHardwareMonitor.Hardware;
using System.IO.Ports;

namespace Get_CPU_Temp5
{
class Program
{
public class UpdateVisitor : IVisitor
{
public void VisitComputer(IComputer computer)
{
computer.Traverse(this);
}
public void VisitHardware(IHardware hardware)
{
hardware.Update();
foreach (IHardware subHardware in hardware.SubHardware) subHardware.Accept(this);
}
public void VisitSensor(ISensor sensor) { }
public void VisitParameter(IParameter parameter) { }
}

static SerialPort SP;
static void GetSystemInfo()
{
SP = new SerialPort();
SP.PortName = "com3";
SP.BaudRate = 9600;
SP.ReadTimeout = 500;
SP.Open();

UpdateVisitor updateVisitor = new UpdateVisitor();
Computer computer = new Computer();
computer.Open();
computer.CPUEnabled = true;
computer.Accept(updateVisitor);
for (int i = 0; i < computer.Hardware.Length; i++)
{
if (computer.Hardware[i].HardwareType == HardwareType.CPU)
{
for (int j = 0; j < computer.Hardware[i].Sensors.Length; j++)
{
if (computer.Hardware[i].Sensors[j].SensorType == SensorType.Temperature)
{
Console.WriteLine(computer.Hardware[i].Sensors[j].Value.ToString());
SP.WriteLine(computer.Hardware[i].Sensors[j].Value.ToString());
}
}
}
}
computer.Close();
SP.Close();
}
static void Main(string[] args)
{
while (true)
{
GetSystemInfo();
}
}
}
}

Ok, the most obvious thing is that you are getting hardware info as fast as your computer can run it!!

Change your
This way, you will sleep for 10ms in between each iteration – your display will still update at around 100hz, but you’ll be using a fraction of the CPU.
Fixed formatting.

Hello, HiddenStoat: code blocks using triple backticks (“`) don’t work on all versions of Reddit!
Some users see this / this instead.
To fix this, indent every line with 4 spaces instead.
FAQ

I’ve tried the thread.sleep thing a long time ago, and visual Studio said the line was wrong and couldn’t run the code, so this is the problem that I have.
Edit: I don’t know how to tell you without sounding mean, I’m not good with words, so don’t get we wrong, I’m actually thankful.

What you’re telling me is exactly my problem. The program runs too fast, consuming an unncessary amount of resources, and I can’t fix that with thread.sleep because VisualStudio doesn’t like it for some reason.
I’m going to try the thread.sleep again. I’ll edit once done.

EDIT 2: Fixed it. Thank you. My problem is that using System.Threading.Tasks isn’t what I needed to put. It’s putting using System.Threading; that fixed it.

I hate to break it to you, but if your program is throwing and not handling exceptions, it’s very likely not all of your code is actually running.
I think that it throws them because it’s running too fast. All the received numbers on the arduino are correct (corroborated by icue), so I’m on the very unlikely part of the spectrum where even though it’s throwing exceptions, it works correctly.

Maybe try using thread.sleep(1000) in your while loop, to make the code only execute, ones every second.
await Task.Delay(1000);

In addition to the other good advice in the comments here I’d suggest switching to using Linq over using for loops. Something like…
I made some assumptions about the data along the lines of
There could be more than one hardware of time CPU
There could me more than one sensor of type temperature
If those aren’t true you could switch from Where to Single which would be more efficient. Won’t help with exceptions or the like but makes the filters cleaner.

Your not wrong, but using LINQ and lamba operators seems like overkill for what OP is seeking advice for.
While thread.sleep is a good start to verify that slowing down your program helps remove the errors you will be able to reduce the all amount of resources your program consumes by moving to asyn code execution and using task.delay, essentially it does the same thing but task.delay releases the thread to do other stuff while it is waiting.

In comparison thread.sleep keeps the thread so it is unable to do other work effective just idling, it might not be something you will see under normal load or even be able to measure in this app, but in the long run if you decide to code other stuff it will be worth considering.
Sadly I don’t have a link to a good tutorial or description readily available but if you search for c# async or task.delay there should be plenty of resources both examples and further information regarding the differences.

I’m not really interested in learning c# right now but this comment is saved! I’ll try to do it on this app. Since it’s just for a dumb aesthetic thing, the least performance it takes the best! Thanks for the tip! 🙂

I know you already posted that you found a solution using System.Threading, (likely Thread.Sleep(...) ) but I’d like to put in an additional suggestion that I haven’t seen here yet. I use cyclic executives quite a bit in C# and have settled on a good way to do it efficiently.

I have found in the past that sometimes Thread.Sleep() can still blast the CPU in a while(true) loop. While Thread.Sleep does stop the thread for a period of time, it doesn’t seem to free up the CPU cleanly during that time. Additionally, await Task.Delay(...) requires an async/await pattern which is not always necessary.

I would recommend looking into AutoResetEvent in System.Threading and using the .WaitOne(...) method when it’s time to sleep the CPU. The WaitOne(...) method will sleep the thread for a period of time and appears to completely free up the CPU in the meantime. You can implement it like this:

This method has an additional advantage that you can include a stop condition easily. Whenever you want to stop the loop safely, you can call _stopLoopEvent.Set(); This will stop immediately (as long as the thread is sleeping) as opposed to Thread.Sleep() which would need to wait until the end of the Sleep to evaluate the exit condition. (Imagine trying to exit a loop when the Thread.Sleep is 10+ seconds.)

I wish I could fully take credit for this solution, but I found it quite a while ago in a some Stackoverflow post when I was getting hit with high CPU usage using Thread.Sleep.

I used await Task.Delay and it improved the performance from 0.2% – 0.6% to 0.1% – 0.3% It isn’t much, but since it’s just for looks i’d like to waste as little performance as possible on it. I’ll try your suggestion, thanks 🙂
Also, yesterday I was looking for it on task manager and it turned out to be inside of a “runtime broker” task. What is this?

source