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

Add additional sensors to ecoforest integration #102734

Merged

Conversation

pjanuario
Copy link
Contributor

@pjanuario pjanuario commented Oct 24, 2023

Proposed change

Some members of the community were using proxy that exposed additional sensors and they requested to include this sensors in the integration.

Add the following sensors to the ecoforest integration:

depression
working_hours
ignitions
live_pulse
pulse_offset
extractor
convecto_air_flow

Minor dependency upgrade:

pjanuario/pyecoforest@v0.3.0...v0.4.0

This dependency upgrade includes features to support new sensors and upgrades of internal dependencies.

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 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.

To help with the load of incoming pull requests:

@pjanuario pjanuario marked this pull request as ready for review October 28, 2023 07:35
@joostlek
Copy link
Member

So wait, not every ecoforest supports these entities?

@pjanuario
Copy link
Contributor Author

Not sure why you mentioned that, I can only test with my device, as mentioned in docs is the only tested device at this moment, there is no public documentation about the different devices, firmwares and APIs.
I would expect that most devices should have this sensors and the library should handle differences.
I set the default visibility to false, because I personally believe that most users wouldn't care about this sensors and are not required for the regular working modes. One user requested the depression sensor, because he used it some automation. Exposing it as it is in the pr I believe it allows for different flexibility while still not part of common usage.

@joostlek
Copy link
Member

Oh just out of curiosity, maybe not every ecoforest model supports every sensor. If that was the case, we could not create the entity at all. But if that isn't the case we can do this PR

@pjanuario
Copy link
Contributor Author

When we get feedback from other models we can make the entity creation conditional

@joostlek
Copy link
Member

Also, please include release notes or a diff of the dependency bump

Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Please include the release notes and diff, I have on remark, other than that it's good

homeassistant/components/ecoforest/sensor.py Show resolved Hide resolved
@home-assistant
Copy link

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

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft October 28, 2023 11:46
@pjanuario
Copy link
Contributor Author

I am not at the laptop, release changes are here.

https://github.com/pjanuario/pyecoforest/releases/tag/v0.4.0

Short story added functionality to support this additional sensors and dependency updates.

@joostlek
Copy link
Member

Put them in the description!

@pjanuario
Copy link
Contributor Author

Also, please include release notes or a diff of the dependency bump

I am on mobile, so it's hard to make changes and I will review it later if any other changes needed, but I see the changes in the description I guess you added.
Anything else needed on this topic?

@pjanuario pjanuario marked this pull request as ready for review October 28, 2023 16:51
@edenhaus
Copy link
Contributor

Please don't merge dev or add other things to the PR after it was approved (if not requested by the reviewer). This makes the approval stale, and we need to review it again.

@pjanuario
Copy link
Contributor Author

Sorry, I was trying to keep the branch in sync to be ready to be merged, wasn't aware of that part of the process.

@pjanuario
Copy link
Contributor Author

Should I revert last commit? Or leave as it is now?

@edenhaus
Copy link
Contributor

Should I revert last commit? Or leave as it is now?

Leave it, I'm already reviewing it

@edenhaus
Copy link
Contributor

@pjanuario You don't need to merge dev into your PR each day/week. If a reviewer thinks the branch it outdated, it will rebase dev onto it.

@joostlek
Copy link
Member

There already are docs linked :)

@edenhaus
Copy link
Contributor

There already are docs linked :)

The app didn't showed it... On desktop yes

@edenhaus edenhaus merged commit 71ecb39 into home-assistant:dev Oct 30, 2023
49 checks passed
@pjanuario pjanuario deleted the feature-ecoforest-additional-sensors branch October 30, 2023 08:42
@highlandr
Copy link

Nice 😃
When depression goes below 50, because the wind is forcing it, then you should turn off the stove. You might get a fire :) And it's a sign that something is wrong.
Thanks 👍

@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2023
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.

None yet

4 participants