Closed shger21 closed 1 year ago
pipiche38
commented
1 year ago I'm not convinced that it is the best to put it in the danfoss pooling. I would prefer that we handle it as a much more device parameter like we do for "DanfossTRVOrientation"
shger21
commented
1 year ago Hi Pipiche, I'm slighly confused about your message below
I would prefer that we handle it as a much more device parameter like we do for "DanfossTRVOrientation"
In my pull request, "DanfossCovered" is handed exacly like "DanfossTRVOrientation" right now. My interpretation of your message above is that you want to introduce one more parameter ;
1) "DanfossCovered": for Danfoss device in-build feature of covered radiator 2) "Danfossxxyyyyy": parameter for excluding thermostat internal sensor for room temperature calculation
I'm open for discussion about requirement specification. I can help refining the requirement, specificication and implementation. Let me know,
With Best Regards, Simo
On Fri, Mar 10, 2023 at 11:37:31AM -0800, Pipiche wrote:
I'm not convinced that it is the best to put it in the danfoss pooling. I would prefer that we handle it as a much more device parameter like we do for "DanfossTRVOrientation"
-- Reply to this email directly or view it on GitHub: https://github.com/zigbeefordomoticz/Domoticz-Zigbee/pull/1560#issuecomment-1464314678 You are receiving this because you authored the thread.
Message ID: @.***>
pipiche38
commented
1 year ago I think that I have been confused by the lines added in the danfoss.py under pooling part. So you are correct.
Could you clarify the behaviour when "DanfossCovered" is set ? I think that is my issue.
shger21
commented
1 year ago Could you clarify the behaviour when "DanfossCovered" is set ? I think that is my issue.
I understand you have a lot of things in your plate. Please let me try to clarify:
Parameter "DanfossCovered" is bringing up two things:
If we don't intrduce number 2. included pull request, we can't have common external temperature sensor(s) for several "covered" radiators in same room. There are work-arounds for situation, but I think my pull request is better. We can could have even more comple parameter set which is even more flexible...but...well...more complex.
Having more that one radiator in same room is complex situation. But it is common situaton.
Here is the backround of changes:
-recent firmmware of of Danfoss Ally added feature named "Covered radiator".
"Covered radiator" mode introduces good possibities for fine control, but it introduce several pittfalls also.
In my pull request, I have tried to make most simple short-cut of complex heating problem: 1) write into thermostat "Covered radiator" mode where thermostats follows only external temperatures sensor values 1) ignore totally the thermostat internal temperature sensor, when writing the "room temperature" into thermostat.
In my opinion, it is important to make it possible to ignore radiator internal sensor. It is fair assumption since parameter indicated that radiator is covered and internal temperature sensor is nonsense. If we don't add that, current code will use also thermostat internal sensor to caluclate room average temperature. That is: if there are several thermostats in same "DanfossRoom".
More can be done. However, the complexity comes for situation when one has: 1) several radioataors/thermostats in same room 2) several temperature sensors in same room
It is difficult to assess, what sensors should be used for each radiator. It would be nice to have all the freedom...but we cam confuse the average user...
Simo
On Fri, Mar 10, 2023 at 12:45:26PM -0800, Pipiche wrote:
I think that I have been confused by the lines added in the danfoss.py under pooling part. So you are correct.
Could you clarify the behaviour when "DanfossCovered" is set ? I think that is my issue.
-- Reply to this email directly or view it on GitHub: https://github.com/zigbeefordomoticz/Domoticz-Zigbee/pull/1560#issuecomment-1464447766 You are receiving this because you authored the thread.
Message ID: @.***>
shger21
commented
1 year ago Hi Pipiche, I read my response and decided to make a brief version:
I have been confused by the lines added in the danfoss.py
- confusing lines are probably the extra I did beyond adding new parameter:
- I have added ignoring the temperature reading of sensor that have the new "DanfossCovered" parameter
Furthermore...I think the "average room temperature" introduced is something in the grey area what zigbee driver should do. I mean, it is good and valid additoon and makes sense. BUT, it is someting that "home automation control" should do, not what device driver should do. If there are several sensors in room...well...that should be exposed to upper level and let upper level decide what to do with them. But after having several sensors we would need to introduce new interface for upper level to control the temperature value to feed into thermostat. Maybe what you have added is good middlegroud, let me know what you think.
Simo
On Sat, Mar 11, 2023 at 12:17:26AM +0200, Simo wrote:
Could you clarify the behaviour when "DanfossCovered" is set ? I think that is my issue.
I understand you have a lot of things in your plate. Please let me try to clarify:
- my pull request is trying ot introduce minimal set of changes to get control of several radiators in same room.
Parameter "DanfossCovered" is bringing up two things:
- Make the thermostat follow only external temperature sensor, making thermostat to ignore internal temperature sensor
- Don't use this thermostat internal sensor to calcultate room temperature average
If we don't intrduce number 2. included pull request, we can't have common external temperature sensor(s) for several "covered" radiators in same room. There are work-arounds for situation, but I think my pull request is better. We can could have even more comple parameter set which is even more flexible...but...well...more complex.
Having more that one radiator in same room is complex situation. But it is common situaton.
Here is the backround of changes:
-recent firmmware of of Danfoss Ally added feature named "Covered radiator".
- It is for radiator, where we know that the thermostat internal sensor is not to be trusted
- In this "Covered radiator" mode, thermostat relies only external temperature sensor, ignoring internal sensor
- In default "Exposed mode", thermostat uses both internal sensor and external sensor for adjustememnt
"Covered radiator" mode introduces good possibities for fine control, but it introduce several pittfalls also.
In my pull request, I have tried to make most simple short-cut of complex heating problem: 1) write into thermostat "Covered radiator" mode where thermostats follows only external temperatures sensor values 1) ignore totally the thermostat internal temperature sensor, when writing the "room temperature" into thermostat.
In my opinion, it is important to make it possible to ignore radiator internal sensor. It is fair assumption since parameter indicated that radiator is covered and internal temperature sensor is nonsense. If we don't add that, current code will use also thermostat internal sensor to caluclate room average temperature. That is: if there are several thermostats in same "DanfossRoom".
More can be done. However, the complexity comes for situation when one has: 1) several radioataors/thermostats in same room 2) several temperature sensors in same room
It is difficult to assess, what sensors should be used for each radiator. It would be nice to have all the freedom...but we cam confuse the average user...
Simo
On Fri, Mar 10, 2023 at 12:45:26PM -0800, Pipiche wrote:
I think that I have been confused by the lines added in the danfoss.py under pooling part. So you are correct.
Could you clarify the behaviour when "DanfossCovered" is set ? I think that is my issue.
-- Reply to this email directly or view it on GitHub: https://github.com/zigbeefordomoticz/Domoticz-Zigbee/pull/1560#issuecomment-1464447766 You are receiving this because you authored the thread.
Message ID: @.***>
pipiche38
commented
1 year ago All clear. Thanks for submitting the PR
Add DanfossCovered parameter for making curtain covered thermostat to ignore internal temp sensor. Dont include covered thermostats for room temperature calculation.