Tpkd day2 new telemetry name #436
Conversation
…italbuildings into tpkd-day2-naming
| - compressor_power_sensor | ||
| - refrigerant_open_percentage_sensor | ||
| - standby_compressor_status | ||
| - pump_speed_mode |
There was a problem hiding this comment.
i have changed to speed_mode, as this is referred to a component of a device which has states of MANUAL & AUTO.
There was a problem hiding this comment.
I have left a lot of comments; I am not convinced most of these points are needed, particularly alarms and the various valve open/closed points; are there not fields that tell us the position of the valve as OPEN/CLOSED?
If this is something we should discuss in a meeting, reach out; I would prefer to purge this list of anything that is unnecessary, and reduce the number of bespoke types and fields that will likely not be reused.
| @@ -120,3 +120,17 @@ CDWS_US_MTV_2081_1: | |||
| - return_water_isolation_valve_status_3 | |||
| - supply_water_isolation_valve_status_4 | |||
| - return_water_isolation_valve_status_4 | |||
|
|
|||
|
|
|||
| CDWS_TPKD: | |||
There was a problem hiding this comment.
if you remove condensing_water_ can most of this not be modeled with existing abstract types? It is fine if it is not canonical, but most of the fields on here can be covered by proper abstract types.
| - run_mode | ||
|
|
||
| CH_SS_TPKD: |
There was a problem hiding this comment.
Same comment as above. Is the only thing on here that can be modeled as an abstract type really SS?
|
|
||
| - motor_energy_sensor | ||
| - motor_frequency_sensor | ||
| - heat_percentage_sensor |
There was a problem hiding this comment.
what is the context for this?
There was a problem hiding this comment.
heat_percentage_sensor - Motor Heat Load (Motor heat loss, calculated by output mechanical power from the input electrical power)
There was a problem hiding this comment.
wouldnt this be in units of power then? something like heat_loss_thermal_power_sensor?
There was a problem hiding this comment.
The unit is percent %
There was a problem hiding this comment.
can the existing efficiency_percentage_sensor be used instead?
| - condensing_return_water_isolation_control_status: | ||
| - LOCAL | ||
| - REMOTE | ||
| - condensing_return_water_isolation_closed_alarm: | ||
| - ACTIVE | ||
| - INACTIVE | ||
| - condensing_return_water_isolation_open_alarm: | ||
| - ACTIVE | ||
| - INACTIVE | ||
| - condensing_return_water_isolation_reset_alarm: | ||
| - ACTIVE | ||
| - INACTIVE | ||
| - condensing_return_water_isolation_simulation_command: | ||
| - ON | ||
| - OFF | ||
| - condensing_return_water_isolation_alarm: | ||
| - ACTIVE | ||
| - INACTIVE | ||
| - condensing_supply_water_isolation_alarm: | ||
| - ACTIVE | ||
| - INACTIVE | ||
| - condensing_supply_water_isolation_closed_alarm: | ||
| - ACTIVE | ||
| - INACTIVE | ||
| - condensing_supply_water_isolation_open_alarm: | ||
| - ACTIVE | ||
| - INACTIVE | ||
| - condensing_supply_water_isolation_reset_alarm: | ||
| - ACTIVE | ||
| - INACTIVE | ||
| - open_command: | ||
| - ON | ||
| - OFF | ||
| - condensing_supply_water_isolation_valve_run_command: | ||
| - ON | ||
| - OFF |
There was a problem hiding this comment.
i am not convinced any of these are needed; some are malformed; some seem unnecessary outside the BMS. what is the use case for them?
There was a problem hiding this comment.
- condensing_return_water_isolation_closed_alarm
- condensing_return_water_isolation_open_alarm
- condensing_return_water_isolation_reset_alarm
- condensing_return_water_isolation_simulation_command
- condensing_supply_water_isolation_closed_alarm
- condensing_supply_water_isolation_open_alarm
- condensing_supply_water_isolation_reset_alarm
- condensing_return_water_isolation_alarm
- condensing_supply_water_isolation_alarm
The points stated above are not necessary so they are removed. The rest of the points should stay as they are isolation value points that can be used to verify why chiller cannot run (if chiller is turned on by mistake)
There was a problem hiding this comment.
There is also no such point name called "condensing_supply_water_isolation_valve_run_command", so it is removed as well
|
@Terry-Ter please see my comments regarding the different fields that look unnecessary. Can you please provide an update? |
|
@tasodorff I have updated the pull request with the changes for the comments, please review it. Thank you! |
| - condensing_water_isolation_valve_percentage_sensor | ||
|
|
||
| PSMC: |
There was a problem hiding this comment.
Several questions here:
- What kind of pump is this supposed to be? Is it a spray pump? A sweeper pump? Simply calling it pump isn't sufficient.
- You have defined speed_mode, which implies its a speed (see here for an example). Looks like you really mean to use control_mode, so this needs to be changed.
- You use speed_frequency_setpoint but I think you mean command.
- If this is really just speed control for a thing, please follow precedent. See here for an example of how to model speed control.
In general, this appears to be something like VSC, which already exists. I don't know that this type is necessary. Can you please explain?
There was a problem hiding this comment.
It is for the chill water pump
The pump_speed_mode is changing the mode between auto and manual, i will change to control mode
The pump_speed_frequency_setpoint is to the user input to set the 0-50Hz which is the speed of pump
There was a problem hiding this comment.
you will model chilled water pumps on their own (they are not integral to a device, so you dont need to use the pump subfield. We know its on a pump because its the run command on a pump entity.
They type you are describing seems to be this. Please check and adjust.
There was a problem hiding this comment.
The PSMC abstract type has been removed and the PMP types are updated
| - pump_speed_frequency_setpoint | ||
|
|
||
| CRWISVC: |
There was a problem hiding this comment.
CWRISOVM2X, to indicate it is actually two sets of valves.
There was a problem hiding this comment.
CRWISVC has been changed to CWRISOVM2X
There was a problem hiding this comment.
i dont see the changes reflected here. please make sure the PR is up to date.
There was a problem hiding this comment.
Changes has been pushed to the PR
| - SS | ||
| - PSMC | ||
| - VSFC | ||
| - CHWISVPCM |
There was a problem hiding this comment.
same question as above -- they have automatic valves isolating individual pumps?
There was a problem hiding this comment.
yes, the pump and the valve is inline
| ######################## | ||
|
|
||
| TK_TPKD: | ||
| description: "Tpkd water tank." |
There was a problem hiding this comment.
Is this a single tank with three sets of alarms at different positions in the tank? If this were three separate tanks they should be modeled independently.
If this really is one tank, it is probably good to create an abstract type for this. How about something like this:
WLAM:
description: "Water level alarm monitoring, generally for a water tank."
is_abstract: true
uses:
- water_high_level_alarm
- water_low_level_alarm
- water_overflow_level_alarm
WLAM3X:
description: "Water level alarm monitoring, generally for a water tank with three independent sets of alarms."
is_abstract: true
uses:
- water_high_level_alarm_1
- water_high_level_alarm_2
- water_high_level_alarm_3
- water_low_level_alarm_1
- water_low_level_alarm_2
- water_low_level_alarm_3
- water_overflow_level_alarm_1
- water_overflow_level_alarm_2
- water_overflow_level_alarm_3
TK_WLAM3X:
description: "A water tank with three sets of alarms for high, low, and overflow alerting."
implements:
- TK
- WLAM3X
There was a problem hiding this comment.
The abstract type has been created
|
|
||
| - motor_energy_sensor | ||
| - motor_frequency_sensor | ||
| - heat_percentage_sensor |
There was a problem hiding this comment.
wouldnt this be in units of power then? something like heat_loss_thermal_power_sensor?
|
@Terry-Ter is there an update for this? |
|
@Terry-Ter please advise on updates |
mschulze17
added
info requested from creator
|
heat_percentage_sensor unit is % |
| - condensing_water_isolation_valve_percentage_sensor | ||
|
|
||
| PSMC: |
There was a problem hiding this comment.
you will model chilled water pumps on their own (they are not integral to a device, so you dont need to use the pump subfield. We know its on a pump because its the run command on a pump entity.
They type you are describing seems to be this. Please check and adjust.
| - pump_speed_frequency_setpoint | ||
|
|
||
| CRWISVC: |
There was a problem hiding this comment.
i dont see the changes reflected here. please make sure the PR is up to date.
| - condensing_return_water_isolation_valve_status_1 | ||
| - condensing_return_water_isolation_valve_status_2 | ||
| - condensing_return_water_isolation_valve_mode |
There was a problem hiding this comment.
which valve though? you list two. which does it apply to?
| - condensing_return_water_isolation_valve_mode | ||
|
|
||
| CSWISVC: |
There was a problem hiding this comment.
the adjustments need to be made still. please ensure this PR is up to date with your local changes.
| - condensing_supply_water_isolation_valve_status_1 | ||
| - condensing_supply_water_isolation_valve_status_2 | ||
| - condensing_supply_water_isolation_valve_mode |
There was a problem hiding this comment.
unless you can designate which valve it belongs to, this should be removed.
There was a problem hiding this comment.
it is used for both valves
|
@Terry-Ter @tasodorff any updates on this? |
|
Hi Trevor, I have updated the PR for your review, my apologizes on the late update. |
|
@terryterpt are the changes in this PR still needed? |
|
Hi @shambergoldstein , yes this PR is still needed |
There was a problem hiding this comment.
all fields must have states or value ranges, plese adjust accordingly
| @@ -735,6 +783,9 @@ literals: | |||
| - STANDBY | |||
| - battery_charge_percentage_sensor | |||
| - brightness_percentage_command | |||
| - speed_mode: | |||
There was a problem hiding this comment.
field has since been added, please add to the states on the existing one as needed and remove this one from PR
There was a problem hiding this comment.
Added some comments/requests. In general these files are pretty outdated due to how old the PR is. Please update the branch, resolving any conflicts with the existing files to date, and revisit the more recently added fields and types to see if they can be used in place of some of the ones you have added here.
There was a problem hiding this comment.
TK already exists in the global namespace, please remove
There was a problem hiding this comment.
TANK.yaml has since been added, please move this type there if needed and remove this file
| @@ -104,5 +106,7 @@ volume: | |||
| - liters | |||
| time: | |||
There was a problem hiding this comment.
these have since been added, please remove
| @@ -47,6 +48,7 @@ flowrate: | |||
| - kilograms_per_hour | |||
| frequency: | |||
| - hertz: STANDARD | |||
| - revolutions_per_minute | |||
There was a problem hiding this comment.
this exists under rotationalvelocity, please remove
| @@ -37,6 +37,7 @@ energy: | |||
| - kilowatt_hours | |||
| - megawatt_hours | |||
| - therms | |||
| - newton_meters | |||
There was a problem hiding this comment.
this exists under torque, please remove
| @@ -756,6 +807,15 @@ literals: | |||
| - position_status | |||
| - condensing_water_valve_percentage_sensor | |||
| - entering_cooling_coil_temperature_sensor | |||
| - condensing_supply_water_isolation_valve_open_status | |||
There was a problem hiding this comment.
does the device have a singulary valve status that provides OPEN/CLOSED as its states that can be used in place of these?
| - condensing_supply_water_isolation_valve_closed_status | ||
| - condensing_supply_water_isolation_valve_open_command | ||
| - condensing_supply_water_isolation_valve_closed_command | ||
| - entering_chilled_water_temperature_sensor |
There was a problem hiding this comment.
is this the same as chilled_return_water_temperature sensor?
| - condensing_supply_water_isolation_valve_open_command | ||
| - condensing_supply_water_isolation_valve_closed_command | ||
| - entering_chilled_water_temperature_sensor | ||
| - leaving_chilled_water_temperature_sensor |
There was a problem hiding this comment.
is this the same as chilled_supply_water_temperature sensor?
| - condensing_supply_water_isolation_valve_closed_command | ||
| - entering_chilled_water_temperature_sensor | ||
| - leaving_chilled_water_temperature_sensor | ||
| - compressor_percentage_sensor |
There was a problem hiding this comment.
compressor_speed_percentage_sensor already exists, please remove
| @@ -695,6 +700,49 @@ literals: | |||
| - PRESENT | |||
| - ABSENT | |||
|
|
|||
| # Added for TPKD day2 | |||
| - condensing_supply_water_isolation_valve_mode: | |||
There was a problem hiding this comment.
Are these points necessary to bring in? We should be able to tell from the command and status if the valve is in manual override.
Added new point name for chiller, cooling tower and pump