Skip to content

Update Rainforest Eagle to use eagle100 instead of uEagle#70177

Merged
balloob merged 5 commits intohome-assistant:devfrom
hastarin:eagle100-fix
May 24, 2022
Merged

Update Rainforest Eagle to use eagle100 instead of uEagle#70177
balloob merged 5 commits intohome-assistant:devfrom
hastarin:eagle100-fix

Conversation

@hastarin
Copy link

@hastarin hastarin commented Apr 17, 2022

Proposed change

In the Rainforest Eagle integration, replace uEagle with eagle100.

The Rainforest Eagle integration uses a package called uEagle for communicating with the older Eagle 100 devices. There is a known issue with this package that means the device stops responding entirely. There has been a pull request pending on that repository for almost a year.

I've created a new package with the pull request merged in and published it as eagle100. The proposed changes simply use the new package instead.

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)
  • 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: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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
  • The code has been formatted using Black (black --fast 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@homeassistant
Copy link
Contributor

Hi @hastarin,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@probot-home-assistant
Copy link

Hey there @gtdiehl, @jcalbert, mind taking a look at this pull request as it has been labeled with an integration (rainforest_eagle) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@bdraco bdraco changed the title Update Rainforest Eagle to use eagle100 instead of uEagle. Update Rainforest Eagle to use eagle100 instead of uEagle Apr 17, 2022
@frenck
Copy link
Member

frenck commented Apr 18, 2022

Thank you for your contribution thus far! 🎖 Since this is a significant contribution, we would appreciate you added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

@hastarin
Copy link
Author

Please, add your GitHub username to the manifest.json of this integration.

I hadn't done that as I haven't actually contributed any code, just packaged things up, but have done so now.

@hastarin
Copy link
Author

NOTE: I haven't ticked the box to say the code has been tested locally as I've only just figured out how to use a custom component to override the existing one and test my changes. So far it works, but I'll need to wait until things fail to see if the code changes from @toddsay have done the trick of working around the issue with the Eagle 100.

From experience this usually happens within 24hrs so I'll happily tick that box once I see it do it's magic.

@frenck
Copy link
Member

frenck commented Apr 19, 2022

I haven't ticked the box to say the code has been tested locally as I've only just figured out how to use a custom component to override the existing one and test my change

Please make sure to set up a development environment and test your changes:

https://developers.home-assistant.io/docs/development_environment

@hastarin
Copy link
Author

hastarin commented Apr 19, 2022

Please make sure to set up a development environment and test your changes:

https://developers.home-assistant.io/docs/development_environment

I got that up and going and have tested the changes. Previously trying to use the integration would lock up my Eagle 100 and it would stop sending data to Wattvision. With this change it logs a couple of Errors in the logs but does recover.

image

I tried using the "Eagle-200 Total Meter Energy Delivered" for the energy dashboard but due to the resets that's not working.

With this change it may be possible to use the Riemann sum integration to derive the value from the "Eagle-200 Meter Power Demand".

Just a quick edit to say I have now also tested with the sensor as I mentioned above and that has worked great. The value for energy used in the last hour shown in Wattvision is identical to that shown in Home Assistant.

@thecode
Copy link
Member

thecode commented Apr 20, 2022

@hastarin have you tried to contact @jcalbert asking to add you as a maintainer or transfer this package to you? I know it sounds hopeless but I have seen few cases that the owner didn't have time to maintainer but was still active enough to add additional maintainers.

@hastarin
Copy link
Author

@hastarin have you tried to contact @jcalbert asking to add you as a maintainer or transfer this package to you?

No I haven't. There has been no response for a year on the pull request which I took as a dead project. I only had a week off work to spend some time on this not chase windmills.

@frenck
Copy link
Member

frenck commented Apr 20, 2022

So, looking at this conversation happening in this PR, I'm not under the impression the fork will not have a commitment to long-term maintenance either.

Some notable quotes from the above

I hadn't done that as I haven't actually contributed any code, just packaged things up,

I haven't ticked the box to say the code has been tested locally as I've only just figured out how to use a custom component to override the existing one and test my changes.

I only had a week off work to spend some time on this not chase windmills.

And things like disabled issue trackers:

image

I'm not saying the current library has a good track record of responding to PRs, but this feels like a quick package up to fix a bug, without the long-term commitment. That is also not a good move.

For that reason, I'm leaning towards declining this change.

../Frenck

@hastarin
Copy link
Author

So, looking at this conversation happening in this PR, I'm not under the impression the fork will not have a commitment to long-term maintenance either.

I'll make an effort as long as I own and use the device. Beyond that I won't promise anything beyond reviewing pull requests.

Some notable quotes from the above

I hadn't done that as I haven't actually contributed any code, just packaged things up,

I didn't want to take credit for simply packaging things and not writing code. Is that such a horrible thing?

I haven't ticked the box to say the code has been tested locally as I've only just figured out how to use a custom component to override the existing one and test my changes.

And the box is now ticked as I've figured out how to test locally and have been running it since.

image

I only had a week off work to spend some time on this not chase windmills.

My apologies that was sent from my mobile and could certainly have been worded better. Let me put this another way.

I value my time and prefer to be efficient in it's use. I did not see value in trying to contact someone who hasn't responded to any mentions on here or a ticket that was opened in Nov 2020. I am not even sure on other ways I would go about contacting @jcalbert beyond these mentions?

Besides the project also contains code for micropython that I'm not interested in maintaining, hence why I took the micro branch and made it main in my main branch. I've just deleted the other stale branches to make that clearer.

And things like disabled issue trackers:

image

This wasn't disabled, it just wasn't enabled as I didn't realize that was the default. It is now enabled.

I'm not saying the current library has a good track record of responding to PRs, but this feels like a quick package up to fix a bug, without the long-term commitment. That is also not a good move.

For that reason, I'm leaning towards declining this change.

I understand the sentiment. Hopefully I've alleviated some of that concern and you allow the change so other Eagle 100 users can use this integration without resorting to things like power cycling the device whenever it stops responding.

If not then I thank you for the consideration and at least I have things working myself and have learned some things about the whole process.

@hastarin
Copy link
Author

hastarin commented May 6, 2022

So today I updated Home Assistant and it seems to have broken things for me again. I guess I need to override the bundled version again. @frenck please let me know if you're definitely not going to accept this and I'll have to come up with a more permanent work around for myself and others that may be interested. 😞

EDIT: In my case I had to hard reset the device and then my workaround (running a dev version of this as a custom component) kicked in again.

@frenck
Copy link
Member

frenck commented May 24, 2022

Thanks for the clarifications @hastarin, that was very helpful ❤️

We are going to merge it.

../Frenck

@balloob balloob merged commit 777c9c0 into home-assistant:dev May 24, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants