add Sensor interface and Measurement type

[soypat]

Re-PRing because I broke my local branch while trying to rebase #321 to latest dev. I'll copy and paste the contents:

Some Background

Hey there peeps, here's part of the Update method proposal. This comes with a Sensor interface which implements said method. This interface then could be embedded in all subsequent sensor interface types, i.e:

type Sensor interface {
	Update(which Measurement) error
}

type IMU interface {
    Sensor
    Acceleration() (ax, ay, az int32)
    AngularVelocity() (gx, gy, gz int32)
}

type Barometer interface {
    Sensor
    Pressure() int32
}

Benefits

These changes were first discussed in add AHRS interfaces for vehicle attitude control and then added as a formal proposal in Proposal: Update method call to read sensor data from a bus. There has been little to no discussion on the matter. I will restate some of the benefits of the Update proposal:

1. Probably most importantly, error management: Where before errors were discarded now they are formally dealt with.
2. Less bus pressure
3. Idiomatic development of sensor frameworks. Provides us with high level interfaces useful for testing.

To be clear, I think we should still have the ReadMeasurement methods which perform an update followed by measurement read which are usually much simpler to understand and work with for people who are new to embedded systems.

Demonstrations and Proof of Concept

• Proof of concept using BMP180 and MCP3008: Sensor interface PR proof of concept #335
• drivers: add mpu6050 additional functionality #298 implements interface for MPU6050 acceleration/angular velocity sensor

Comments on current methodology

The way it is currently done there are methods for drivers that look like so:

// No apparent error handling
func (d Dev) ReadMeasurement() (x, y int16) 

// Explicit error handling
func (d Dev) ReadMeasurementWithErr() (x, y int16, err error) 

There are some benefits to having these

• Provides a "cleaner" APIs by not having a returned error method as is the case for most drivers today in this repo. i.e ReadPressure() (microbar int32) (but discards error)
• It's easier to understand if you are a newcomer to embedded systems
• Less importantly, if one prefers a memory optimized implementation, ReadMeasurement method requires no data to be stored before passing it to user. Nowadays I think storing a couple uint32's should not be a problem on modern controllers.

Community Comments

Yurii Soldak on adding a Measurement() method on top of the ReadMeasurement() method:

So, for say, Barometer, we gonna have 3 methods in a typical driver implementation: Will it not be confusing?
Or we willing to drop ReadPressure()? Shall we standardise on error emitted then incompatible Measurement type requested?

Ayke unsure about an Audio Measurement:

because audio is a bit different from the others. Unlike things like temperature, humidity, or time, audio requires a large amount of data to move around and is very time sensitive (a delay of 1ms may be far too long).

Patricio on how Update() chooses to update measurements

That said I believe it is worth discussing the side effects of Update with an example: on the MPU6050 the registers are ordered as such AccelXH_int8, AccelXL_int8, ... TempH_int8, TempL_int8, RotHX_int8, RotLX_int8, ... RotLZ_int8. Say you want Acceleration and rotation, as is common, you would certainly perform a streamed read from AccelXH to RotLZ as a single i2c transaction, but now you've also read the temperature into buffer. Now, if I've counted the possibilities correctly, you have two options which decide how you document Update:

1. // Update reads sensor data requested. Does not guarantee only the sensors requested are read. When calling Measurement, data is read straight from the buffer
2. // Update reads only sensor data requested.. This probably means you have to create struct field for each sensor data and only save to it the values requested. This takes up more space if also storing the buffer inside the struct. Goes from 24 bytes to 48 or so. A small benefit of this is that there need not be byte shifting and ORing on Measurement call since Update already does that for us.

[soypat]

It just occurred to me the arguments to Measurement() could be units, so

func (d Dev) Temperature(unit units.Temperature) (T int32) {
   switch unit {
       case units.Kelvin:
           return d.K
       case units.Celsius:
           return d.K-273
       /// etc.
   }
}

This would probably clutter the drivers package quite a lot if implemented at module level. Maybe a new package called units? Since these would clash with drivers.Temperature of type drivers.Measurement from this PR.
This may not be worth doing since it'd put a whole lot of strain on the driver developer. Maybe it is enough for Sensor type to return SI units. and declaring other methods alongside Temperature() such as Celsius()/ReadCelsius(). Doing it this last way would allow for the type Thermometer interface to really be standarized and allow for external libraries to be developed based on the SI units assumption.

[deadprogram]

That would be a good use for https://pkg.go.dev/periph.io/x/periph/conn/physic probably.

[deadprogram]

This is the file in question here: https://github.com/google/periph/blob/v3.6.8/conn/physic/units.go

If only it were in its own package with no other dependencies on periph's Conn. cc/ @maruel

[maruel]

@deadprogram this original repository was split in 3 last year. I documented the changes at https://periph.io/news/2020/a_new_start/

The package "conn" is now a standalone repository at https://github.com/periph/conn. It does import one package mainly to make testing easier; https://github.com/periph/conn/blob/main/go.mod

The current version is v3, so periph.io/x/conn/v3, instead of the previous repository with an additional "periph" in the path: periph.io/x/periph/conn.

I sent you an email about that on May 29th 2021 but you haven't replied yet.

[deadprogram]

embarrassed checking of enormous backlog of email is hereby promised. 😹

But what I mean is after clicking on those links, is if units.go was in sub-package periph.io/x/conn/v3/physic/units it seems like it would be so much easier to import here.

[maruel]

Do you mean periph.io/x/conn/v3/physic?

My only big regret there is physic.Env that I want to move out in v4. Nothing else is a struct.

[deadprogram]

We need to try this out then, seems like it could be really cool.

[soypat]

So I've reconsidered my position about adding units to the interface: Maybe it's not such a good idea? My drive-by comment on adding units to driver logic seems like a bad idea as the driver would then have to have unit processing logic inside of it. Maybe it's best to let the user bring in unit logic if they so desire and let drivers be just drivers (although we should standardize units on all implementations)

I'd imagine something of the sort:

tempSens := peripheral123.New(i2c0) // this is the drivers package implementation
tempSensExternal := unit.NewCelsius(tempSens) // recieves a drivers.Thermometer interface

microKelvin := tempSens.Temperature()
Celsius := tempSensExternal.Temperature()

[aykevl]

Maybe it is enough for Sensor type to return SI units.

Totally agree. Drivers should not be concerned with US units like Fahrenheit, for example. Instead, let's just use worldwide standards ;)

However, some people prefer different units (Fahrenheit, Kelvin, ...). That's of course fine. But I feel strongly that drivers should not be concerned with this and instead defer this to common code.

That would be a good use for https://pkg.go.dev/periph.io/x/periph/conn/physic probably.

Something like it, definitely. That's what I was aiming for with #332. However, I'm a bit concerned that the periph package might be a bit too heavy for our uses. For example, the Celsius and Fahrenheit methods both return a float64, while a float32 would be good enough for nearly all practical purposes. Same for the units themselves: they often are int64 while int32 is often enough for all practical purposes (many sensors only return 16-bit data, even).
I do however fully agree with the general design of the package. With it, a driver could simply return a physic.Temperature (or similar) and let the user pick the unit they'd like to use via methods. No Go interfaces needed.
(Sidenote: #332 was inspired on time.Duration, and it looks like the physic package was as well).

[maruel]

To clarify, https://periph.io/x/conn/v3/physic and not the old package.

For Temperature, I thought about using int32 but it was a bit more constraining on the range and I didn't want to enforce arbitrary limitations. Having a lot of precision makes it significantly more forgiving for poorly designed integer operations.

I used Nano everywhere to be like time.Duration. It made everything more coherent; people don't have to guess the resolution when working in one metric versus another. Was it micro, milli, nano? It's easy to get confused for new users. Even a new user sees that all the units are in Nano, it reduces cognitive overhead.

float32 versus float64 is a fair point. I think most soft float libraries do the calculation in 32 bits resolution anyway so I suspect it's likely mostly a storage issue.

[aykevl]

For Temperature, I thought about using int32 but it was a bit more constraining on the range and I didn't want to enforce arbitrary limitations. Having a lot of precision makes it significantly more forgiving for poorly designed integer operations.

Hmm, that's a fair point. It's a bit of a convenience vs performance tradeoff here, I guess. It doesn't matter on 64-bit systems, but might matter (not sure about that) on 32-bit systems. Add/sub of int64 is fast on a 64-bit system (only half as slow in the ideal case) but mul/div can be significantly slower and cost more in code size because it needs to be implemented in software.

I used Nano everywhere to be like time.Duration. It made everything more coherent; people don't have to guess the resolution when working in one metric versus another. Was it micro, milli, nano? It's easy to get confused for new users. Even a new user sees that all the units are in Nano, it reduces cognitive overhead.

Also a fair point. Although that can be reduced a bit by providing related methods: temperature.Millidegrees(), temperature.Microdegrees(), etc.

float32 versus float64 is a fair point. I think most soft float libraries do the calculation in 32 bits resolution anyway so I suspect it's likely mostly a storage issue.

No. Floating point operations are very precisely defined, and the Go standard library depends on it. In fact tinygo test math doesn't pass on systems without FPU because the softfloat library we're using (compiler-rt) doesn't implement subnormal values for all operations (IIRC it doesn't for division). That's a bug in compiler-rt, but shows that it is generally very precise.
Also, related: right now float64 doesn't work on AVR but that's because we use avr-gcc, we'll switch to compiler-rt and picolibc some day which fixes this.

In general, I'm leaning towards using periph.io/x/conn/v3/physic even though I have some concerns about its use on microcontrollers.

[soypat]

I'd like to make a case for not having units be part of the drivers main package. Like Ayke said, it would be a heavy weight on microcontrollers which users will not be able to avoid in using drivers. If an abstraction similar to periph's physic was desired I'd propose a separate package inside drivers repository with sensor to units functions.

One way of implementing this would be having pure functions which return physical quantity types, much like periph's physic.

package sensor // or units, physic, 

// Temperature represents millicelsius. This could be taken from physic directly.
type Temperature uint32

// the drivers.Thermometer type would be possible with this this PR.
func GetTemperature(sensor drivers.Thermometer) (Temperature, error) {
    // ...
}

func (t Temperature) Farenheit() float64 { return t.Celsius()*celsiusToFarenheit }

These functions could then be the goto examples on how to use TinyGo with sensors. They'd be easy to use and opt-in, which for me is a a huge advantage since I prefer to avoid float and other data transformations on the microcontroller and prefer to defer that to a remote computer.

[deadprogram]

@soypat I think your points here are right on. How do you think we should we proceed from here?

[maruel]

If needed, I'm open to refactor the periph.io/x/conn/v3 library, i.e. make a v4.

[soypat]

@deadprogram Well, before any of this gets off the ground we should define tinygo's user-level API going forward.

1. I have proposed moving towards a more robust Update then Measurement methodology so as to replace the inconsistent ReadMeasurement API TinyGo drivers depend on today (see this PR's description).

2. Part of this API update requires also defining what units are represented by the uint32's returned by the Measurement methods. Maybe it makes sense to not have all of them be the same unit? I'm thinking units for each sensor type could be chosen to best represent the typical magnitude measured by a sensor. A typical magnetometer reading could be around 50 microteslas (assuming no neodymium magnets present in area) while a barometer starts absolute pressure readings at 100,000 pascals. If all measurements are micro units, then we have very poor resolution for measuring earth's magnetic field and we are also unable to meaningfully represent atmospheric pressure variations, much less any absolute pressure on this blue earth since MaxInt32=2147483647, or 2147 units.

3. After that it will be a long and arduous journey as new PRs are encouraged to use this new methodology of sensor data acquisition and old drivers are updated.

4. Once step 3 begins this sensor package can be written. I think makes sense to make use of physic package for this since it already has all the interesting conversions. These sensor.ReadMeasurement functions would be aware of the specific unit convention in drivers package and convert accordingly.

[soypat]

This proposal could be linked with #491 which proposes a new organization level package called tinyio. I think it is fitting Measurements type would fit under that package or a subpackage of tinyio. This is what the tinyio package could look like.

[soypat]

There's been a lot of attention guided towards the existing mpu6050 driver. It's been nearly a year since we started talking about changing the API of sensors in the drivers repo and things are still up in the air. I'm very keen on leading this towards a finished design. In my experience the Update() and Measurement() API has yielded great benefits for writing robust software. I'd really like for tinygo to adopt this methodology over the existing inconsistent way of doing things.

This PR would be the first step in this direction as it would allow for new drivers to be written to follow this convention and not break existing drivers.

I understand there were some reservations on where the design would go after this PR, but I feel that is besides the point. This is a first step and does not necessarily have to answer every question out there. It's simple enough to seem reasonably future proof and a huge improvement over existing APIs. Let's reach an agreement!

[aykevl]

I still quite like your original design.

As for the discussion on the units: I still like specific units like Temperature, Pressure, etc (see #332). It's indeed a question where to put them, I think putting them as a sub-package under the drivers repo (tinygo.org/x/drivers/units) would be fine.
Having these units also mostly solves the issue around usability, as they can provide convenience functions for particular units:

• A .Float() or .Celsius() method on a Temperature type could return the temperature as a floating point type.
• .Fahrenheit() (returned as a float?) could return the temperature in Fahrenheit for those preferring freedom units, no need to clutter drivers with these.
• Perhaps methods like .Millidegrees() could make it more obvious what kind of units we're working with.

I was originally concerned about the extra bytes necessary for storage, but honestly I don't think that's a big deal. There are things that take up far more space than a few sensor readings.

[maruel]

Initially I was very against floating points, as their processing on MCU as softfp is very expensive. I've softened my take over time as I understand using integer calculation is too difficult for most users.

I'd still recommend something similar to what I did with conn/v3/physics, where the units are different from the measurement type. The main difference would be to use float32 instead of int64.

[aykevl]

@maruel I still think it's a really bad idea to require the use of floating point. In fact, I'd say float32 is a lot worse than int64: with int64, you have a large integer but at least most operations on them are relatively fast. With floating point numbers, they are going to be pretty slow and require large softfloat libraries.

[soypat]

A .Float() or .Celsius() method on a Temperature type could return the temperature as a floating point type.
.Fahrenheit() (returned as a float?) could return the temperature in Fahrenheit for those preferring freedom units, no need to clutter drivers with these.

I like where this is going but I've got to ask if you mean this PR's contents should be in the same units package?
I feel for the moment as if these have different responsibilities, at least judging by the name of the package- units sounds like it deals with the SI and other systems. It does not so much hint towards sensors, drivers and whatnot.

The proposed Sensor type has the following purpose(s):

1. Precisely define I/O over a bus so that users can wield sensors intelligently: Don't read something you don't want or need. Read all you need in one fell swoop.
2. Give sensors a common data structure so that users can group a system of sensors under one signature. I've talked about this a bit before in Proposal: Update method call to read sensor data from a bus #310... I feel it's hard to overstate the potential of interfaces in this case, beyond the Update method.

So that said, with the help of chatgpt here are a few suggestions for names for a package:

1. sensors
2. sense
3. sensorctl/sensectl
4. sensorfusion
5. sfusion
6. sensecore/sensorcore
7. sensematic (maybe this one was me)

[aykevl]

I like where this is going but I've got to ask if you mean this PR's contents should be in the same units package?
I feel for the moment as if these have different responsibilities, at least judging by the name of the package- units sounds like it deals with the SI and other systems. It does not so much hint towards sensors, drivers and whatnot.

I don't have a strong opinion where all these things should be defined. I think I'd just put them in the bare drivers repo (both units and the Sensor type) but I'm fine with other options.
If we're going to separate out units in a units package, then yes I agree Sensor doesn't quite feel right in that location.

(Perhaps we're misunderstanding, I don't think I suggested defining Sensor in a units package?)

The proposed Sensor type has the following purpose(s):

1. Precisely define I/O over a bus so that users can wield sensors intelligently: Don't read something you don't want or need. Read all you need in one fell swoop.

2. Give sensors a common data structure so that users can group a system of sensors under one signature. I've talked about this a bit before in

Fully agree with this one.
As an experiment, I'm working on https://github.com/aykevl/board where I'd like to define a single Sensors object where you can call Update(drivers.Steps | drivers.LightIntensity) and then read the individual sensors like Acceleration(), Steps(), LightIntensity(), etc. This board API would work really well with the sensor API proposed here, and would essentially implement the same interface but abstracted over multiple sensor chips. (Or is that what you mean with "so that users can group a system of sensors under one signature")?

So that said, with the help of chatgpt here are a few suggestions for names for a package:

Why would you want to put it in a separate package? I think putting the Sensor interface in the bare drivers repo would be fine, just like Displayer and rotations are defined there.

[soypat]

I don't have a strong opinion where all these things should be defined. I think I'd just put them in the bare drivers repo (both units and the Sensor type) but I'm fine with other options. [...] Why would you want to put it in a separate package?

Agree 100%. I wasn't sold on the idea of defining a separate package. I feel they belong under drivers. I had misinterpreted what you said so I began to explore the idea by thinking out loud.

(Or is that what you mean with "so that users can group a system of sensors under one signature")?

What I meant is that roughly you'd be able to compose sensors and sensor systems much easier. For example some patterns I image will arise when working with large complex systems:

type DistillationPlant struct {
	thermometers [TOTAL_THERMOMETERS]drivers.Thermometer
	barometers   [TOTAL_BAROMETERS]drivers.Barometer
	// If one of these sensors fails we are in trouble.
	missionCritical SensorGroup
	redundant       SensorGroup
	// Data acquisition for analytics
	telemetry SensorGroup
}

type SensorGroup struct {
	senseMask uint32
	sensors   []drivers.Sensor
}

func (sg *SensorGroup) Update(which drivers.Measurement) error {
	which &= sg.senseMask
	for _, sensor := range sg.sensors {
		err := sensor.Update(which)
		if err != nil {
			// store error, log some stuff, but keep updating them.
		}
	}
	// Interesting to see how errors are composed so
	// exact place of fault can be programatically determined.
	return sg.makeErrorIfNotNil()
}

Final question: Is this PR ready to add as is? Should something more be added?

[aykevl]

The PR looks good to me. Did one last call in the Slack channel, but otherwise I'd say just merge it.

[deadprogram]

Thank you for working on this @soypat and to everyone who helped review. Now merging.

[aykevl]

In #587 I am adding support for the BMA42x accelerometer. This accelerometer also includes a step counter that is able to count steps while it consumes very little current.

The question is now: should this be a separate measurement or should it update along with drivers.Acceleration?

Reasons for making this a separate measurement:

• It makes sense: it really is a separate measurement and therefore it would make sense to update it separately.
• It reduces complexity and confusion in code.
• It improves performance, when the step count is needed but not the acceleration values.

Reasons for treating it as part of drivers.Acceleration:

• It's tied to the accelerometer, it can't work without it.
• It's not a real separate SI unit.
• We may run out of bits in drivers.Measurement if we add too many units.

The way I see it is that we should decide whether step counting (and perhaps other things, like tap gestures that are also part of the BMA425) are important enough to get their own bit in drivers.Measurement (of which we have only 32) or not.

[soypat]

Maybe it's worth having a measurement type for this kind of discrete values? i.e:

• drivers.DiscreteQuantity
• drivers.Discretized
• drivers.Quantity
• drivers.DiscreteValue

Tried asking chat gippity and it wasn't too helpful heh.