Fixes for ASUS Crosshair VIII Hero #540
Conversation
zeule
force-pushed
the
c8h-fixes
branch
2 times, most recently
from
6e05565
to
bb1cf3f
Compare
|
Testing on my Dark Hero board, looks good so far, BUT: Only T-Sensor is plugged in, Water In and Out are both unplugged. 216 (0xD8) seems to be some kind of default value. I remember having that value appear in my logs for short bursts of time (1 second) sporadically when I ran both LHM and HWinfo simultaneously. Back then I concluded that it probably is some race condition since it doesn't happen with only of them running. 216°C sounds like something well beyond the max temperature of most water cooling sensors anyways, so I'd argue that the reading 0xD8 should just be discarded and ignored. |
Probably means they moved the sensor in Dark Hero. I'll remove it then. Maybe they are in place of the chipset fan? |
|
Maybe you could take the T sensor and plug it in the water headers please, to test the register addresses? For C8H such re-plugging worked without power-cycling the board (which is naturally non-surprising, as the sensor is just a thermistor). |
|
Looks like 216 is the default value for non-connected probes. And all three work as expected when connected. (Small sidenote: I did forget that I still had FanControl running in the background. With all my fans being controlled by T-Sensor, they shot up straight to 100% while I was busy switching plugs. Made me jump a little 🤣) |
|
Thank you! I will add 0xd8 as the placeholder value for these two water sensors then. |
All three of them jump to 216 as seen in the screenshot above. |
My bad... Thanks! |
|
Added blank values for EC temperature sensors that can possibly be not plugged in (T Sensor and water). |
|
Looks good, the sensors stop and resume as expected 👍 |
Great! Thank you for testing! |
|
I just tried it on the Formula, everything seems ok! Thanks for your work! |
Thank you for testing this out! Added the Formula board to the Linux driver. |
|
|
||
| protected EmbeddedController(List<EmbeddedControllerSource> sources, ISettings settings) : base("Embedded Controller", new Identifier("lpc", "ec"), settings) | ||
| { | ||
| _sources = sources.ToArray(); |
There was a problem hiding this comment.
Maybe make _sources a list instead? Seems rather pointless to allocate an array here.
There was a problem hiding this comment.
I want to highlight that the _sources length does not change after creation. Is there a better way?
There was a problem hiding this comment.
You can make _source IReadOnlyList.
There was a problem hiding this comment.
Thank you, but from my point of view that is over complicated. Besides, I pass arrays to the EC reader object, so would need to convert on each call should I choose to leave the List here.
There was a problem hiding this comment.
_sources only seems to be used in Update to access the instances, so all you need to do change private readonly EmbeddedControllerSource[] _sources; to private readonly IReadOnlyList<EmbeddedControllerSource> _sources; 🙂
There was a problem hiding this comment.
For whatever reason I misread _sources for _registers, sorry. Yes, will replace with IReadOnlyList then.
| { | ||
| return ReadLoop<byte>(register, ReadByteOp); | ||
| } | ||
| if (registers.Length > data.Length) throw new ArgumentException("data buffer length has to be greater or equal to the registers array length"); |
There was a problem hiding this comment.
Replaced with Trace.Assert
LibreHardwareMonitorLib/Hardware/Motherboard/Lpc/EC/EmbeddedController.cs
Outdated
Show resolved
Hide resolved
This implements bank switching for the EC controller and uses that to read water temperatures. The sensors in the Nuvoton chips that were thought to be water temperatures turned out to be QFan sources and are removed for now.
|
Thank you for testing and reviewing! |
This implements bank switching for the EC controller and uses that to read water temperatures. The sensors in the Nuvoton chips that were thought to be water temperatures turned out to be QFan sources and are removed for now.
Also supplies correct default fan and controls names.
I plan to use ASUS WMI functions to read EC (and maybe Nuvoton) state to avoid race between LHM and ASUS AISuite and perhaps firmware when reading data. However, calling those methods in Windows did not work from the first attempt and hence this PR brings EC bank switching using the direct port write method, which is potentially dangerous (but other 3rd-party monitoring software use it anyway), and reduces the
IEmbeddedControllerIOto the only function, available in the ASUS DSDT code.