core icon indicating copy to clipboard operation
core copied to clipboard

Add Libre Hardware Monitor integration

Open Sab44 opened this issue 1 year ago • 14 comments

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)
  • [x] 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

  • This PR fixes or closes issue: n/a
  • This PR is related to issue: n/a
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/37941
  • Link to developer documentation pull request: n/a
  • Link to frontend pull request: n/a

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [x] 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:

  • [x] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [x] 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:

Sab44 avatar Mar 12 '25 13:03 Sab44

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 avatar Mar 13 '25 07:03 Sab44

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?

joostlek avatar Mar 13 '25 10:03 joostlek

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.

Sab44 avatar Mar 13 '25 11:03 Sab44

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 avatar Mar 13 '25 11:03 joostlek

cc @lazytarget just as an FYI.

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

joostlek avatar Mar 13 '25 12:03 joostlek

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?

Sab44 avatar Mar 13 '25 12:03 Sab44

They are not optional for the quality_scale rule :)

joostlek avatar Mar 13 '25 12:03 joostlek

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 avatar Mar 13 '25 19:03 LazyTarget

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?

LazyTarget avatar Mar 13 '25 19:03 LazyTarget

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

joostlek avatar Mar 13 '25 19:03 joostlek

@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 avatar Mar 14 '25 07:03 Sab44

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 avatar Mar 16 '25 18:03 Sab44

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Apr 08 '25 16:04 home-assistant[bot]

@joostlek I have addressed all comments, did not resolve them yet so you can check them again. I structured all changes into hopefully easily reviewable commits, but I can of course squash them if desired. The docs PR is also updated to reflect changes made here.

Sab44 avatar Apr 17 '25 08:04 Sab44

Please send me a message on discord or put your username here so we can add you to the right groups

joostlek avatar Sep 01 '25 20:09 joostlek

Add me: sab.44

Sab44 avatar Sep 02 '25 10:09 Sab44