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

Support multiple heating circuits, boilers, etc #14

Merged
merged 4 commits into from
Oct 24, 2022

Conversation

LLukas22
Copy link
Contributor

This is a first draft of the changes mentioned in #13.

I tried to abstract the update method into the specific components. This allows me to easily work with lists of components e.g. update 4 different buffers.

I also tried to solve the problem with the setters for every holding value by creating a commit method that will write the current raw value of a DataValue to the modbus server.

With these changes the api could be used like this:

from pysolarfocus import SolarfocusAPI,PORT,Systems

#the modbus client was moved into the api
solarfocus = SolarfocusAPI("IP",buffer_count=4,heating_circuit_count=2 ,system=Systems.Therminator)
solarfocus.connect()

#Updates all buffers
solarfocus.update_buffer()

#print all buffer
for buffer in solarfocus.buffers:
    print(buffer)

#print 3. buffer
print(solarfocus.buffers[2])

#Get Second Heating Circuit
hc_2 = solarfocus.heating_circuits[1]
#Set the value to 30 Degree
hc_2.indoor_temperatur_external.set_unscaled_value(30)
#write the value to the heating system
hc_2.indoor_temperatur_external.commit()

To implement this i had to refactor some of the scaling code, would be great if you could verify if the values are still correct for your system. I already tested it for mine and it seams to work.

@LavermanJJ LavermanJJ self-requested a review October 22, 2022 17:29
@LavermanJJ LavermanJJ added the enhancement New feature or request label Oct 22, 2022
example.py Outdated Show resolved Hide resolved
@LavermanJJ LavermanJJ added this to the 2.0.0 milestone Oct 22, 2022
@LavermanJJ
Copy link
Owner

Nice 👍 LGTM! Thanks for the quick and thorough work! Much appreciated!

@LavermanJJ
Copy link
Owner

FMPOV it can be merged. However, I'm waiting for your final confirmation before merging, as you wrote "This is a first draft of the changes".

@LavermanJJ
Copy link
Owner

One small remark, would you mind also adapting the Usage section in the REAMDE.md? Thanks!

@LLukas22
Copy link
Contributor Author

One small remark, would you mind also adapting the Usage section in the REAMDE.md? Thanks!

I Updated the README.md to showcase the new init() and the support for multiple components.

FMPOV it can be merged. However, I'm waiting for your final confirmation before merging, as you wrote "This is a first draft of the changes".

I still consider this a draft, because i don't know how you would implement this in the home assistant implementation.
If you don't need any other properties or setters to integrate this into the HA implementation you can merge this PR.

I would also like to remove the direct getters and setters from the api, but that would break the compatibility with the current HA implementation. Maybe we could change this in the future? WDYT?

@LavermanJJ
Copy link
Owner

Sounds reasonable. My initial idea was to merge this PR as it is backwards compatible and doesn't break something, and in a second step adopt the HA-integration (with the risk however, that we would need to adapt something again). Once the HA-integration is ready, we can get rid of the "compatibility stuff". However, I can also park it for now, until there is a concrete idea for the HA-integration.

What do you mean with "direct getters and setters"? Are your referring to:

@property
 def hc1_supply_temp(self) -> float:
...
def hc1_set_target_supply_temperature(self, temperature) -> bool:

If so, that would be ideal, to get rid of them.

@LLukas22
Copy link
Contributor Author

What do you mean with "direct getters and setters"? Are your referring to:

@property
 def hc1_supply_temp(self) -> float:
...
def hc1_set_target_supply_temperature(self, temperature) -> bool:

If so, that would be ideal, to get rid of them.

Yeah i would like to remove those in the future, accessing those values over the components directly would be a lot cleaner.
For now you could merge this PR and then have a look at the HA implementation.
If we need to change something later we can change it without taking backwards compatibility into consideration.

@LavermanJJ LavermanJJ merged commit 673902f into LavermanJJ:main Oct 24, 2022
@LavermanJJ LavermanJJ linked an issue Oct 24, 2022 that may be closed by this pull request
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple heating circuits, boilers, etc
2 participants