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

feat: convert BACnet units to unit symbols (use SI where possible) #16

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kinnarr
Copy link
Member

@kinnarr kinnarr commented Apr 5, 2022

Fixes #3

Includes a script to check the conversation between BACnet units and unit symbols and also show missing units.

python helper_scripts/check_bacnet_units.py --all

@kinnarr kinnarr requested a review from a team April 5, 2022 12:30
@kinnarr kinnarr self-assigned this Apr 5, 2022
@kinnarr kinnarr added the enhancement New feature or request label Apr 5, 2022
Copy link

@tilsche tilsche left a comment

Choose a reason for hiding this comment

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

Generally it looks like it is all sane SI units plus Non-SI units accepted for use with the International System of Units. But I am not sure this will really help us towards our goal: Being able to simply slap on a SI-Unit prefix.

  1. So many existing prefixes, so we need to parse them. But there is a lot of ambiguity, e.g. "dB" "min"
  2. Parsing prefixes is even more error prone with squares
  3. Adding prefixes with squares is error prone
  4. Parsing and adding prefixes within ratios

At the end of the day, I think we need to find a library to actually achieve our goal (maybe this means in JS). And then we need to figure out if the library eats what the output of this.

229: "mSv",
230: "µSv",
231: "µSv/h",
234: "pH",
Copy link

@tilsche tilsche Apr 5, 2022

Choose a reason for hiding this comment

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

Unless that is picoHenry - the pH value is dimensionless.

Copy link
Member Author

Choose a reason for hiding this comment

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

So we need an empty unit here otherwise the BACnet unit name (pH) is used

Comment on lines +465 to +466
if unit_int in BACNET_TO_SI_UNITS:
unit = BACNET_TO_SI_UNITS.get(unit_int)
Copy link

Choose a reason for hiding this comment

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

Not sure why you'd combine if / get. Could use get(unit_id, unit) or try / except KeyError

167: "A/m",
168: "A/m²",
169: "A m²",
199: "dB",
Copy link

Choose a reason for hiding this comment

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

dB is explicitly a non-SI with the following remark:

In using these units it is important that the nature of the quantity be specified, and that any
reference value used be specified. These units are not SI units, but they have been accepted by
the CIPM for use with the SI.

Comment on lines +31 to +32
200: "dBmV",
201: "dBV",
Copy link

Choose a reason for hiding this comment

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

That may even be "acceptable non-SI" but parsing hell...

214: "g/l",
215: "mg/l",
216: "µg/l",
217: "g /m³",
Copy link

Choose a reason for hiding this comment

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

Suggested change
217: "g /m³",
217: "g/m³",

187: "N s",
188: "N/m",
96: "ppm",
97: "ppb",
Copy link

Choose a reason for hiding this comment

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

The terms “parts per billion”, and “parts per trillion”, and their respective abbreviations “ppb”,
and “ppt”, are also used, but their meanings are language dependent. For this reason
the terms ppb and ppt are best avoided. (In English-speaking countries, a billion is
now generally taken to be 10^9 and a trillion to be 10^12; however, a billion may still
sometimes be interpreted as 10^12 and a trillion as 10^18. The abbreviation ppt is also
sometimes read as parts per thousand, adding further confusion.)

96: "ppm",
97: "ppb",
98: "%",
99: "%/s",
Copy link

Choose a reason for hiding this comment

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

... maybe ....

@kinnarr
Copy link
Member Author

kinnarr commented Apr 5, 2022

To make this clear, the list of units is defined by BACnet and every unit not in the map will use is unit name from BACnet

@kinnarr
Copy link
Member Author

kinnarr commented Apr 5, 2022

Currently we use the following units:

264
amperes
bars
cubicMeters
cubicMetersPerHour
degreesCelsius
gramsOfWaterPerKilogramDryAir
hertz
kilovoltAmperes
kilovoltAmperesReactive
kilovolts
kilowattHours
kilowatts
litersPerMinute
litersPerSecond
metersPerSecond
millimeters
minutes
noUnits
partsPerMillion
pascals
percent
percentRelativeHumidity
pH
voltAmperes
volts
watts

And yes, 264 and noUnits are valid BACnet units

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.

Map BACnet units to SI units
2 participants