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

adds DPRS, SPSW and TDSW #224

Merged
merged 1 commit into from Apr 17, 2024
Merged

Conversation

blip2
Copy link
Contributor

@blip2 blip2 commented Mar 20, 2024

added additional sensors as per #223

Copy link
Collaborator

@RitaLav RitaLav left a comment

Choose a reason for hiding this comment

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

Could the DPRS description change to ' differential pressure sensor' instead of 'differential pressure switch'?

@blip2
Copy link
Contributor Author

blip2 commented Mar 21, 2024

@RitaLav these are two different physical things (switch = boolean, sensor = analog value) do we add both?

@RitaLav
Copy link
Collaborator

RitaLav commented Mar 22, 2024

Sensor and switch can both be added with separate abbreviations.

DPRSW (actuator - differential pressure switch) already exists in the register (so we don't need to add another one), the sensor does not.

@RitaLav
Copy link
Collaborator

RitaLav commented Mar 27, 2024

one more comment added to issue #223 which concludes all abbreviations requested

@blip2
Copy link
Contributor Author

blip2 commented Mar 29, 2024

all comments addressed - resolves #223

@blip2 blip2 requested a review from RitaLav March 29, 2024 15:38
Copy link
Collaborator

@RitaLav RitaLav left a comment

Choose a reason for hiding this comment

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

We believe the switches should be best classified as switches (since you note on the issue that have normally open and closed states.

Therefore changes as follows (first item remains unchanged)

sensor - differential pressure sensor,DPRS,SENSOR,IfcSensor,PRESSURESENSOR
actuator - pressure switch,PRSW, ,IfcActuator,NOTDEFINED
actuator - static pressure switch, ,IfcActuator,NOTDEFINED
actuator - thermal dispersion flow proving switch,TDSW, ,IfcActuator,NOTDEFINED

Is that a correct interpretation?

@blip2
Copy link
Contributor Author

blip2 commented Apr 6, 2024

hi Rita, I don't feel any of those should be classed as an actuator. An actuator would normally be an output device, not an input.

I would say DPRSW is wrong - maybe added as a sensor for an actuator? but a differential pressure switch itself is a input/sensor.

@jgunstone
Copy link
Collaborator

hi -
I was just about to add PRSW but I'm still a bit confused...

to my logic:
sensors (IfcSensor) reads info i.e. pressure for a system
switches / actuators (IfcSwitchingDevice, IfcActuator) provides action.

actuators / switches provide fundamentally the same function but are for mechanical / electrical action respectively.

I agree with @blip2 that this is in wrong:
image
I think its incorrectly listed as an IfcActuator despite explicitly being called a switch in the description. proposee the following change:

switch - differential pressure,DPRSW,,IfcSwitchingDevice,NOTDEFINED
e.g.
https://www.wika.com/en-gb/psd_4.WIKA
both senses, and switches (digital and analogue outputs available) but does not actuate.

then if we add the other switches i'd suggest:
switch - absolute pressure,PRSW, ,IfcSwitchingDevice,NOTDEFINED
switch - static pressure,SPRSW ,IfcSwitchingDevice,NOTDEFINED

@RitaLav
Copy link
Collaborator

RitaLav commented Apr 10, 2024

Thank you @blip2 @jgunstone

Conclusion:

Will add:
sensor - differential pressure sensor,DPS,SENSOR,IfcSensor,PRESSURESENSOR
switch - differential pressure,DPSW,,IfcSwitchingDevice,NOTDEFINED
switch - absolute pressure,APSW, ,IfcSwitchingDevice,NOTDEFINED
switch - static pressure,SPSW, ,IfcSwitchingDevice,NOTDEFINED
switch - thermal dispersion flow proving switch,TDSW, ,IfcSwitchingDevice,NOTDEFINED

We will also need to amend the existing dew point switch abbreviation from 'DPSW' to 'DEWSW' and modify its ifc class to IfcSwitchingDevice

DPSW (previosuly dew point switch) is now DEWSW
@blip2
Copy link
Contributor Author

blip2 commented Apr 10, 2024

rebased due to major changes in master branch. should be good to go now.

@blip2 blip2 requested a review from RitaLav April 10, 2024 19:18
@RitaLav RitaLav merged commit 8c884cd into theodi:master Apr 17, 2024
1 check passed
@RitaLav
Copy link
Collaborator

RitaLav commented Apr 17, 2024

Thank you @blip2 - all done

@blip2 blip2 deleted the 223-additional-sensors branch April 17, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants