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 support for SomfyHeatingTemperatureInterface #703

Merged

Conversation

egguy
Copy link
Contributor

@egguy egguy commented Jan 11, 2022

Updated implementation of the SomfyHeatingTemperatureInterface.

  • Use the temperature of the thermostat
  • Added 2 NumberSettings, but wasn't able to see them.
  • Add "Impossible to change from the box" mode selection (cooling/heating).
  • Mostly working
  • Need to add some constant to pyOverKiz

Sorry if the pull request is not in the good format. It's mostly my first one.

@tetienne
Copy link
Collaborator

Hi, many thanks for this 1st contribution, any help are welcome 🎉 I haven’t yet look at your code, as you can see you have too many files modified. It’s probably because your branch is out of sync with our repo. To solve this:

git fetch --all
git rebase upstream/master
# Check you haven’t any conflicts, if yes solve them. 
git push --force-with-lease

Once done, your PR will be ready for our review :)

@egguy egguy force-pushed the feature/climate/SomfyHeatingTemperatureInterface branch from 969d205 to 62538f1 Compare January 11, 2022 12:10
@egguy
Copy link
Contributor Author

egguy commented Jan 11, 2022

Ok, I followed your procedure, I hope I didn't make a mistake.

@tetienne tetienne changed the base branch from feature/climate/SomfyHeatingTemperatureInterface to master January 11, 2022 13:54
@tetienne
Copy link
Collaborator

I’ve updated the base branch. Apparently, we only have your modification now.

@iMicknl
Copy link
Owner

iMicknl commented Jan 16, 2022

@egguy I just updated your branch to use the latest fixes for the coordinator (double casting) and updates pyoverkiz to 1.1.1.

@egguy
Copy link
Contributor Author

egguy commented Jan 17, 2022

Pulled the changes. Now the OverkizNumber input are working. (need to comment the step parameter)
What's remain to allow the pull request to be integrated ?

Ps: What is the relation between ha-tahoma and overkiz in core ? Is ha-tahoma the test integration ?

iMicknl
iMicknl previously approved these changes Jan 17, 2022
Copy link
Owner

@iMicknl iMicknl left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's merge if another maintainer approves :-).

@iMicknl
Copy link
Owner

iMicknl commented Jan 17, 2022

Ps: What is the relation between ha-tahoma and overkiz in core ? Is ha-tahoma the test integration ?

This integration is a bit behind on some parts now, since we had great code reviews in core. However, the aim is to migrate the integration 1:1 to core, but that will take quite some time. Eventually ha-tahoma will be the test integration, and core will be the production.

See #697 for the progress and what is still due (a lot! 😄 ).

@iMicknl iMicknl requested a review from tetienne January 17, 2022 23:23
@egguy
Copy link
Contributor Author

egguy commented Jan 17, 2022

Ps: What is the relation between ha-tahoma and overkiz in core ? Is ha-tahoma the test integration ?

This integration is a bit behind on some parts now, since we had great code reviews in core. However, the aim is to migrate the integration 1:1 to core, but that will take quite some time. Eventually ha-tahoma will be the test integration, and core will be the production.

See #697 for the progress and what is still due (a lot! smile ).

The overkiz integration is mostly for covers at the moment ? Or does it manage the other types of devices ?

@iMicknl
Copy link
Owner

iMicknl commented Jan 17, 2022

The overkiz integration is mostly for covers at the moment ? Or does it manage the other types of devices ?

The Overkiz integration (1:1 copy from ha-tahoma) doesn't have the cover platform yet. See the issue I linked to see which platforms are already merged.

Corrected removed property by mistake.
Use more constants from pyOverkiz.
@vlebourl
Copy link
Collaborator

Are @tetienne 's requested changes included? if so, we should dismiss the review and proceed.

@egguy
Copy link
Contributor Author

egguy commented Jan 18, 2022

I have applied the requested changes. For the codespell error, it comes from a URL. :s

@egguy
Copy link
Contributor Author

egguy commented Jan 18, 2022

I have added the pre-commit to my env to prevent more problems in the future.

@iMicknl iMicknl linked an issue Jan 18, 2022 that may be closed by this pull request
1 task
Copy link
Owner

@iMicknl iMicknl left a comment

Choose a reason for hiding this comment

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

Thanks again! Happy to merge if the other users confirms that this works. #705

@iMicknl iMicknl merged commit 5716df3 into iMicknl:master Jan 26, 2022
@iMicknl
Copy link
Owner

iMicknl commented Oct 18, 2022

@egguy do you perhaps want to port this one to core? :)
Otherwise I will have a look in the coming weeks.

@egguy
Copy link
Contributor Author

egguy commented Oct 18, 2022

Hi, i'm already doing the port ATM. Hope to be able to commit soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants