Skip to content

Add Libre Hardware Monitor integration #140449

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

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from

Conversation

Sab44
Copy link
Contributor

@Sab44 Sab44 commented Mar 12, 2025

Proposed change

The current Open Hardware Monitor legacy integration is for a software that has not been updated in more than 4 years. It's successor is Libre Hardware Monitor.

While the legacy integration technically also works for Libre Hardware Monitor, it has many issues and is not maintained.

This integration has been rewritten from scratch following the best practices from the current developer docs, providing:

  • config flow
  • configurable scan interval
  • add only user selected hardware devices
  • entities grouped by hardware device
  • reconfigure anytime via UI reconfigure flow
  • robust against hardware changes on the host system
  • much improved code complexity and readability
  • extensive test coverage
  • quality scale: silver

In my opinion, we should deprecate the legacy integration and redirect users to this one.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Copy link
Contributor

@tr4nt0r tr4nt0r left a comment

Choose a reason for hiding this comment

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

There are a few things you have to consider for a new integration. First of all, the PR scope has to be kept small. Only implement the user config flow, the reconfigure flow can be added in a follow-up PR.
Also variable/configurable scan interval is not allowed in core integrations.

@Sab44
Copy link
Contributor Author

Sab44 commented Mar 13, 2025

Hi @tr4nt0r thanks for the feedback.

Regarding the reconfigure flow: sure I can take this out and submit it in a follow-up PR.

Regarding the configurable scan interval: could you point me to the developer docs where it is outlined that it cannot be configurable? I found the custom polling interval docs before, but it does not really say that a configurable scan interval is disallowed.
I just want to note that I did set limitations to adhere to HA's minimum polling interval of 5 seconds. I feel like for a System Monitor it is vital to allow the user to specify their desired polling interval - ideally in a intuitive way during integration setup. But of course, if this is not desired then I can set a sensible default value and redirect to the custom polling interval docs on the integration docs.

@Sab44 Sab44 force-pushed the add_librehardwaremonitor_integration branch from 457ff11 to c1f6f18 Compare March 13, 2025 10:41
@joostlek
Copy link
Member

Please note that someone is taking on the work on open hardware monitor and there's a PR for that at the moment.

Is the API of both applications the same?

@Sab44
Copy link
Contributor Author

Sab44 commented Mar 13, 2025

Hi @joostlek
wow, what are the odds after years of legacy status... Not sure what to make of this.

Here's my thought process when I started working on implementing this integration from scratch:

  • Open Hardware Monitor is legacy software, nobody should use it anymore. Instead they should use Libre Hardware Monitor. The integration name should reflect this, guiding new users intuitively to the right integration.
  • The new integration should reflect the current state of development i.e. fulfill at least all the bronze quality requirements.

After a quick look at the other PR for openhardwaremonitor, in comparison this new integration provides:

  • re-implementation instead of re-factor, avoiding pitfalls that are present in the old integration (and still are there after the re-factor, e.g. if hardware exposed by LHM changes, it will break the integration as sensors are based on indices that will no longer be present/valid)
  • allow users to select which devices they want to add to Home Assistant
  • high unit test coverage
  • updated documentation & higher quality icon

Regarding the API, Libre Hardware Monitor seems to be backwards compatible, which is why the current openhardwaremonitor integration does technically still work. However, I have not tested whether my implementation will work with Open Hardware Monitor installations, as I don't think this is a valid use case anymore. Presumably it should work though.

@joostlek
Copy link
Member

I am also not saying which one is better, PRs should be well scoped, so I don't think comparing PRs is fair here, as new integrations are always bigger and contain more feature than an integration that is trying to modernize.

@joostlek
Copy link
Member

cc @LazyTarget just as an FYI.

We just discussed this in the architecture meeting, and we think its fine to have them co-exist.

@Sab44
Copy link
Contributor Author

Sab44 commented Mar 13, 2025

Glad to hear that, thanks for clarifying and sorry for the hassle.

If someone could give me a quick hint why the tests on the CI are complaining about missing data_description translations when to my knowledge those are optional, I'd appreciate it. What am I missing there?

@joostlek
Copy link
Member

They are not optional for the quality_scale rule :)

@Sab44 Sab44 force-pushed the add_librehardwaremonitor_integration branch from c1f6f18 to c5f5867 Compare March 13, 2025 13:02
@LazyTarget
Copy link
Contributor

LazyTarget commented Mar 13, 2025

wow, what are the odds after years of legacy status... Not sure what to make of this.
@Sab44

What ARE the odds?! 😄

Cool to see that we both identify the need for an update, and interesting to see our different takes (even not all to different)
Really great job! I see you also put some hours into it.
I will see if can test yours out tomorrow

@LazyTarget
Copy link
Contributor

I am also not saying which one is better, PRs should be well scoped, so I don't think comparing PRs is fair here, as new integrations are always bigger and contain more feature than an integration that is trying to modernize.

We just discussed this in the architecture meeting, and we think its fine to have them co-exist.

@joostlek
Sounds good. Should I revisit my PR a bit and make sure avoid breaking changes as much as possible?
Like continue supporting the YAML configuration?

@joostlek
Copy link
Member

With the migration to the UI we should import the YAML config and that is an expected breaking change

@Sab44
Copy link
Contributor Author

Sab44 commented Mar 14, 2025

@Sab44
What ARE the odds?! 😄

Yep, I guess it really was time to finally update this as LHM is such a nice tool and the legacy integration was increasingly becoming a pain point.
Thank you too for your work and improvements! Let me know if you find any problems during your tests.

@Sab44 Sab44 force-pushed the add_librehardwaremonitor_integration branch 2 times, most recently from 5c0c784 to 259f931 Compare March 16, 2025 18:16
@Sab44
Copy link
Contributor Author

Sab44 commented Mar 16, 2025

There were a couple of code coverage warnings which are now resolved, so this is now ready for review.
If there is anything left to do from my side, please let me know.

@Sab44 Sab44 force-pushed the add_librehardwaremonitor_integration branch from 259f931 to babf625 Compare March 25, 2025 10:04
Copy link
Contributor

@LazyTarget LazyTarget left a comment

Choose a reason for hiding this comment

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

Like mentioned in the other comment, In my case Libre Open Hardware Monitor is not able to get any sensors from my motherboard, which failed the startup.

Everything else looks and works great, truly good job!

@home-assistant home-assistant bot marked this pull request as draft April 8, 2025 16:32
@Sab44 Sab44 force-pushed the add_librehardwaremonitor_integration branch from bb84471 to 8bb9cc5 Compare June 27, 2025 06:08
@Sab44 Sab44 marked this pull request as ready for review June 27, 2025 06:34
@home-assistant home-assistant bot requested a review from joostlek June 27, 2025 06:34
@frenck frenck requested review from Copilot and removed request for LazyTarget and tr4nt0r June 27, 2025 07:07
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new integration for Libre Hardware Monitor to replace the legacy Open Hardware Monitor integration. The changes include implementing a config flow with proper error handling, rebuilding the coordinator and sensor entities for improved robustness and maintainability, and adding comprehensive tests and dependency updates to support quality scale silver.

Reviewed Changes

Copilot reviewed 19 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/components/librehardwaremonitor/test_sensor.py Tests for sensor creation, update, and handling of missing data.
tests/components/librehardwaremonitor/test_coordinator.py Tests for coordinator error handling, data refresh, and orphaned device removal.
tests/components/librehardwaremonitor/test_config_flow.py Tests for proper error messaging and flow recovery in the config flow.
tests/components/librehardwaremonitor/conftest.py Common fixtures and sample data for the Libre Hardware Monitor tests.
tests/components/librehardwaremonitor/snapshots/* Snapshot tests ensuring sensor state consistency.
requirements_test_all.txt / requirements_all.txt Dependency updates to include the librehardwaremonitor-api package.
mypy.ini Updated mypy configuration for the new integration.
homeassistant/components/librehardwaremonitor/sensor.py Implementation of sensor entities with formatted numeric values.
homeassistant/components/librehardwaremonitor/coordinator.py Coordinator that fetches data and handles device changes, including orphan device removal.
homeassistant/components/librehardwaremonitor/config_flow.py Config flow implementation with error handling for connectivity and missing devices.
homeassistant/components/librehardwaremonitor/manifest.json, quality_scale.yaml, etc. Integration metadata and quality scale configuration.

Comment on lines +56 to +58
DeviceId(next(iter(device.identifiers))[1]): DeviceName(device.name)
for device in device_entries
if device.identifiers and device.name
Copy link
Preview

Copilot AI Jun 27, 2025

Choose a reason for hiding this comment

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

Consider using a more robust approach to extract the identifier by filtering the identifiers for the one associated with the integration rather than using next(iter(...)) directly, which may be error-prone if multiple identifiers are present.

Suggested change
DeviceId(next(iter(device.identifiers))[1]): DeviceName(device.name)
for device in device_entries
if device.identifiers and device.name
DeviceId(
identifier[1]
): DeviceName(device.name)
for device in device_entries
if device.identifiers and device.name
for identifier in device.identifiers
if identifier[0] == DOMAIN

Copilot uses AI. Check for mistakes.

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.

4 participants