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

all: introduce a temperature type #332

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

all: introduce a temperature type #332

wants to merge 1 commit into from

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Oct 21, 2021

This type should be used whenever a sensor (or actuator?) works with a
temperature. For example, this commit changes the signature:

ReadTemperature() (int32, error)

to the following:

ReadTemperature() (drivers.Temperature, error)

I believe this is much clearer in intent. It also makes it trivial to
introduce common conversions. For example, there are already Celsius()
and Fahrenheit() methods to convert to the given units, as a floating
point. More units could be added as needed, for example a CelsiusInt().


This PR has a few possibly controversial changes:

  • I've changed the DHT driver a bit to be more in line with other drivers
  • I've removed the ReadTempC and ReadTempF functions from the adt7410 driver because those are now unnecessary

I am aware that this is a breaking change. However, we use versioning and Go modules for a reason, and I think this change will improve the drivers in general. I intend to also apply a similar change to other units such as pressure (and maybe acceleration etc.)

This type should be used whenever a sensor (or actuator?) works with a
temperature. For example, this commit changes the signature:

    ReadTemperature() (int32, error)

to the following:

    ReadTemperature() (drivers.Temperature, error)

I believe this is much clearer in intent. It also makes it trivial to
introduce common conversions. For example, there are already Celsius()
and Fahrenheit() methods to convert to the given units, as a floating
point. More units could be added as needed, for example a CelsiusInt().
@soypat
Copy link
Contributor

soypat commented Oct 22, 2021

This seems out of line with what I feel is the purpose of the drivers namespace, to provide a hardware abstraction layer. This seems more natural-sciences or physics library oriented. It also conflicts with this PR https://github.com/tinygo-org/drivers/pull/321/files, which implements the Temperature type to abstract a sensor type.

@soypat
Copy link
Contributor

soypat commented Oct 22, 2021

Would it be worth introducing a physics-like package, much like the periph.io package?

@aykevl
Copy link
Member Author

aykevl commented Oct 22, 2021

This seems out of line with what I feel is the purpose of the drivers namespace, to provide a hardware abstraction layer. This seems more natural-sciences or physics library oriented.
[...]
Would it be worth introducing a physics-like package, much like the periph.io package?

The Temperature type was indeed based on the same idea of periph.io. I didn't put it in a different package as I didn't see a need for it: it seemed to me like the base drivers package is good enough for that. I like to keep things in the same package unless there is a good reason not to.

It also conflicts with this PR https://github.com/tinygo-org/drivers/pull/321/files, which implements the Temperature type to abstract a sensor type.

Right. I didn't think of that. Maybe that's a good reason to move it to a different package.

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.

2 participants