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

Barometer sensor: Soft-remove hPA unit for compatibility reasons #72

Merged
merged 1 commit into from
May 5, 2020

Conversation

dbrgn
Copy link
Contributor

@dbrgn dbrgn commented May 3, 2020

This way, an endpoint with a barometer sensor can be compatible with
both 0.13 and 0.14. See discussion in #69.

@dbrgn dbrgn requested a review from a team May 3, 2020 22:10
@dbrgn dbrgn mentioned this pull request May 3, 2020
This way, an endpoint with a barometer sensor can be compatible with
both 0.13 and 0.14. See discussion in #69.
@dbrgn dbrgn force-pushed the hpa-deprecation branch from b562c4a to b288005 Compare May 3, 2020 22:12
@gidsi
Copy link
Member

gidsi commented May 3, 2020

I'm not sure if we should add the unit if there is just one unit that you can choose from?
We have the same thing with wind direction and wind elevation.

I mean, it's more explicit that way, but also kinda useless.

@dbrgn
Copy link
Contributor Author

dbrgn commented May 4, 2020

I'm not sure if we should add the unit if there is just one unit that you can choose from?

It's mostly a compatibility thing, v0.13 requires the unit and if we remove the unit field we break compatibility.

It's also more open for extension in case there are other units in the future. We can of course say that all units must be normalized to standardized SI units, but for example for air pressure, would you specify Pa or hPa? Pa is the SI base unit but hPa is what's being used to specify air pressure.

I'd keep the unit, it doesn't break compatibility and it makes the sensor values self-describing.

@gidsi
Copy link
Member

gidsi commented May 4, 2020

I'm not sure if we should add the unit if there is just one unit that you can choose from?

It's mostly a compatibility thing, v0.13 requires the unit and if we remove the unit field we break compatibility.

It's also more open for extension in case there are other units in the future. We can of course say that all units must be normalized to standardized SI units, but for example for air pressure, would you specify Pa or hPa? Pa is the SI base unit but hPa is what's being used to specify air pressure.

I'd keep the unit, it doesn't break compatibility and it makes the sensor values self-describing.

It wouldn't break compatibility if we remove it, 0.13 should still have it and 0.14 wouldn't care.

Alternatively i would also add kPa (that's what i3 uses, 1 of 4 spaces that has barometer readings). Isn't Pa the SI unit and hPa and kPa are just factors and by that still a SI unit?

@dbrgn
Copy link
Contributor Author

dbrgn commented May 4, 2020

It wouldn't break compatibility if we remove it, 0.13 should still have it and 0.14 wouldn't care.

Oh, I forgot that we'll simply ignore unknown values... That is true. But it would only work if we'd take hPa as the unit, otherwise a v0.13 value of 1013 hPa would be treated as 1013 Pa in v0.14.

If we'd start from a green field, I would suggest to leave away the unit and to always force the use of appropriate SI units (Pa or hPa in this case), but I think since all sensors currently have a unit field I'd stick probably to that (and ensure that those contain reasonable but not too many units).

Alternatively i would also add kPa (that's what i3 uses, 1 of 4 spaces that has barometer readings).

Hm, yeah, we need to find a tradeoff between making it easy to enter the data (by just using the units that are available) versus making it easy to consume the data (since data needs to be normalized). kPa seems to be a common unit for car tire pressure, for example. Meteorology uses hPa mostly. I'm not sure whether we should add kPa. Maybe we should always add the base unit and the commonly used unit for the type of sensor being used (if different)? In this case that would be hPa and Pa. (Barometers measure atmospheric pressure, they're not used for measuring tire pressure. Manometers are. Maybe some spaces would like to track the pressure of gas bottles in their workshop? Then an appropriate sensor with an appropriate unit could be added.)

(Edit: I just saw that i3 also provides a compressed_air sensor with psi as unit. All their sensor values are invalid though since they're strings, not numbers.)


Some brainstorming: Another approach would be to specify the value together with a factor. So pressure would be Pa, hectopascal values would use a factor of 100, bar values would use a factor of 100000 and atm values would use a factor of 101325. But again, easier to consume but harder to provide. Using units is probably better, the consumers can map those units to factors if desired.

Copy link
Member

@rnestler rnestler left a comment

Choose a reason for hiding this comment

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

Nice! I like the approach of soft deprecation 🙂

@gidsi gidsi merged commit c1398f2 into master May 5, 2020
@gidsi gidsi deleted the hpa-deprecation branch May 5, 2020 05:26
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.

3 participants