merging in backup control changes #155 #160
Conversation
kendallbaertlein
commented
Dec 9, 2024
•
kendallbaertlein
commented
- Reference the issue your PR is fixing
- Assign at least 1 reviewer for your PR
- Test with run_dwelling.py or other script
- Update documentation as appropriate
- Update changelog as appropriate
…nto backup_control_hvac
|
For posterity, why we added the deadband weight: At least one major manufacturer shows that when you run with an ASHP in heating or cooling, they tend to be pretty lopsided in terms of maintaining the setpoint. I tried to make that change consistently throughout the code. |
There was a problem hiding this comment.
Looks good to me. Unless @mnblonsky has final comments, I'll merge this in later this week.
ochre/Equipment/HVAC.py
Outdated
| @@ -326,6 +333,8 @@ def update_setpoint(self): | |||
| # updates setpoint with ramp rate constraints | |||
| # TODO: create temp_setpoint_old and update in update_results. | |||
| # Could get run multiple times per time step in update_model | |||
|
|
|||
| # FIXME: do we need a ramp rate with lockout for time after setpoint change | |||
There was a problem hiding this comment.
I'm pretty sure we don't, since we have the new "soft" lockout feature that checks that if temp is increasing it keeps the backup element lockout, which accomplishes something similar. But I want everyone to weigh in.
There was a problem hiding this comment.
We can get rid of ramp rate. Jeff to make that update. In the future for variable speed controls where you don't want to run at full part load, send a % of load that can be met.
ochre/Equipment/HVAC.py
Outdated
| self.timestep_count = 1 | ||
| # self.existing_stages = 0 # no staged | ||
| return 'Off' | ||
| # elif self.end_use == 'HVAC Cooling': |
There was a problem hiding this comment.
Probably want to remove? I don't know (and we don't have data) on whether anything like this would apply for 2 speed equipment, but that's the only use case I could think of.
ochre/Equipment/HVAC.py
Outdated
| hard_lockout_interval = dt.timedelta(minutes=hard_lockout) # minimum amount of time after a setpoint change that er stays off (strictly) | ||
| if hard_lockout_interval > min_interval: | ||
| min_interval = hard_lockout_interval # increase the minimum interval | ||
| print(f"minimum setpoint change duration ({min_setpoint_change_duration} minutes) updated to comply with hard lockout interval ({hard_lockout} minutes)") #TODO: raise warning ? |
There was a problem hiding this comment.
This should probably be an OCHRE warning instead of a print statement.
|
Is there a file or script to test the new functionality? I'd like to make sure it runs under a few conditions, e.g. 1 min vs 15 min resolution. |
ochre/Equipment/HVAC.py
Outdated
| # TODO: add option to keep setpoint as is, e.g. when using external control | ||
| er_setpoint = self.temp_setpoint - self.temp_deadband | ||
| temp_indoor = self.zone.temperature | ||
| def run_er_thermostat_control(self, temperature_offset = 1.6, min_setpoint_change_duration = 30, hard_lockout = 10, staged = False, max_outdoor_temp = 1.67): #temperature offset in C, ecobee default |
There was a problem hiding this comment.
I'll plan to clean this up a little bit, today or next week. Once that's done I think we're ready to merge
| self.prev_setpoint = self.temp_setpoint | ||
| # self.existing_stages = 0 # no staged | ||
| return 'Off' | ||
| else: # in case there is no outdoor temp limit provided, use a default |
There was a problem hiding this comment.
If we want a default, it should be specified in init. I think we didn't have a
default before, maybe we should remove this?
| temp_turn_on = er_setpoint - self.hvac_mult * self.temp_deadband / 2 | ||
| temp_turn_off = er_setpoint + self.hvac_mult * self.temp_deadband / 2 | ||
| if temperature_offset is not None: | ||
| temp_turn_on = er_setpoint - self.hvac_mult * temperature_offset |
There was a problem hiding this comment.
I don't understand this. Are we changing the deadband size of the ER element?
Should this be default behavior, or should there be a flag for it?
| # indoor and previous temp | ||
| self.prev_temp_indoor = self.temp_indoor | ||
| self.temp_indoor = self.zone.temperature | ||
|
|
There was a problem hiding this comment.
Should these be options that can be changed or switched off?
I'll add a script or at least run a test before merging |
