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

[oximeter] Add some units from physical reality #6296

Merged
merged 2 commits into from
Aug 13, 2024
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Aug 12, 2024

In service of future changes to record data from power, temperature, and
fan speed sensors in Oximeter, this branch adds some physical quantities
to the Units enum: Volts, Amps, DegreesCelcius, and Rpm. I've
added all of these as whole numbers of the measured quantities, with the
expectation that they will probably be recorded as floating-point. We
could consider instead using a smaller unit like MilliAmps, and
recording them as integers, but that introduces a bunch of dimensional
analysis that I'm not sure if we want to be doing.

In service of future changes to record data from power, temperature, and
fan speed sensors in Oximeter, this branch adds some physical quantities
to the `Units` enum: `Volts`, `Amps`, `DegreesCelcius`, and `Rpm`. I've
added all of these as whole numbers of the measured quantities, with the
expectation that they will probably be recorded as floating-point. We
could consider instead using a smaller unit like `MilliAmps`, and
recording them as integers, but that introduces a bunch of dimensional
analysis that I'm not sure if we want to be doing.
@hawkw hawkw requested a review from bnaecker August 12, 2024 19:30
@bnaecker
Copy link
Collaborator

We could consider instead using a smaller unit like MilliAmps, and recording them as integers, but that introduces a bunch of dimensional analysis that I'm not sure if we want to be doing

This is the right call, IMHO. I would like to maintain base SI units insofar as possible!

Copy link
Collaborator

@bnaecker bnaecker left a comment

Choose a reason for hiding this comment

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

Thank you :)

@david-crespo
Copy link
Contributor

I think this will require a regen of the Nexus openapi spec and possibly others

@hawkw hawkw merged commit b08cce7 into main Aug 13, 2024
23 checks passed
@hawkw hawkw deleted the eliza/oximeter-units branch August 13, 2024 16:20
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