Skip to content
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

[systeminfo] Add CPU frequency channels #16012

Merged
merged 6 commits into from Mar 18, 2024
Merged

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Dec 6, 2023

Resolves #15844
Resolves #15608

This PR adds CPU frequency (clock speed) channels to the binding, exposing underlying functionality from the OSHI library. Note that the availability of this information may vary by platform. In my tests on Windows, I could not get reliable information. It did work well on an RPi 4.

It also solves some documentation issues.

@mherwege
Copy link
Contributor Author

mherwege commented Dec 6, 2023

Build failed because of the integration tests that had minimal change. The failure was also not only in the added tests.
Is this a symptom of #13524?

@mherwege mherwege marked this pull request as draft December 7, 2023 14:05
@mherwege
Copy link
Contributor Author

mherwege commented Dec 7, 2023

Tests are failing now. I don't see the immediate reason for it yet.

@lolodomo lolodomo added the enhancement An enhancement or new feature for an existing add-on label Dec 7, 2023
@mherwege mherwege marked this pull request as ready for review January 16, 2024 15:38
@mherwege
Copy link
Contributor Author

mherwege commented Jan 16, 2024

Tests were failing because I included update instructions for the 2 added channels.

All of the tests create a thing with only the channel being tested. However, with the update instructions, when the newly created thing is added to the ManagedThingProvider:


it gets the new channels added to the thing as well (and not just the channels added with the ThingBuilder). The result for all of the tests is that the thing handler initialization fails.

To make this work again, all of the tests would have to be revisited to work with a full set of channels, and not just the channel being tested, unless someone has a better suggestions. I therefore opted to not include update instructions. The user will have to remove and recreate the thing if he wants to benefit from the extra channels.

@mherwege mherwege requested a review from J-N-K January 16, 2024 15:47
@J-N-K
Copy link
Member

J-N-K commented Jan 16, 2024

Why is the thing handler initialization failing?

@mherwege
Copy link
Contributor Author

I am not entirely sure. The next line in the test (assertion on the thing handler) times out whereby the thing handler is null. That would indicate the thing handler initalization is not even running. However, running it through a debugger, the handler factory is called and the handler creation is called from there.
If you want to try, the last commit in this PR removes the update instructions again. But you could roll back that commit to test with update instructions.

@J-N-K
Copy link
Member

J-N-K commented Jan 17, 2024

Just add .withProperties(Map.of("thingTypeVersion", "1")) to the thing builder to prevent updates. The other solution is to get the thing from the registry and before checking it has a handler. The thing changes during the upgrade, so the "original" thing is never initialized and never gets a handler.

@mherwege
Copy link
Contributor Author

mherwege commented Jan 17, 2024

Thanks, I used this:

.withProperties(Map.of("thingTypeVersion", "1"))

I believe I tried this before:

get the thing from the registry and before checking it has a handler

but I then ended up with a thing that also has the new channels. And that would mean I would need to mock the refresh on these channels for all tests as well (easy enough to do in the setup, but not necessary for each test).

@mherwege
Copy link
Contributor Author

DCO failure is because the revert commit was not signed off. If that is an issue, I can squash that with the original to solve it.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Only an ultra minor comment

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@mherwege
Copy link
Contributor Author

Build is now failing and I don't understand why. I rebased but it didn't help. The last commit is only a javadoc change, so cannot have an impact.

I have the impression there are other addon PR's suddenly failing the build.

I also have a failed local build.

@lolodomo lolodomo added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Mar 17, 2024
@lolodomo
Copy link
Contributor

Build was aborted after 240 minutes.
I try again.
I don't know if you have to rebase your branch or not after the Karaf changes of yesterday (my local builds are no more building today without a rebase).

@lolodomo lolodomo added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Mar 18, 2024
@lolodomo
Copy link
Contributor

Build finally succeeded.

@lolodomo lolodomo merged commit 08f210e into openhab:main Mar 18, 2024
3 checks passed
@lolodomo lolodomo added this to the 4.2 milestone Mar 18, 2024
@jlaur
Copy link
Contributor

jlaur commented Mar 18, 2024

I don't know if you have to rebase your branch or not after the Karaf changes of yesterday (my local builds are no more building today without a rebase).

This is not necessary. The PR build is performed on the PR branch merged with target branch.

@mherwege mherwege deleted the systeminfo branch March 18, 2024 16:32
magx2 pushed a commit to magx2/openhab2-addons that referenced this pull request Mar 24, 2024
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
4 participants