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

add AHRS interfaces for vehicle attitude control #299

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

Conversation

soypat
Copy link
Contributor

@soypat soypat commented Aug 30, 2021

Drivers level type to aid in the standarization of vehicle attitude, position estimation.

For sensors with rotation in degrees such as the MPU6050 we could add an additional type

type ARSdeg interface {
	// Acceleration returns sensor accelerations in micro gravities
	Acceleration() (ax, ay, az int32)
	// Rotation returns sensor angular velocity in micro degrees
	Rotation() (gx, gy, gz int32)
}

I've created a sample library for how this would look like in implementations: https://github.com/soypat/ahrs

@aykevl
Copy link
Member

aykevl commented Sep 1, 2021

How is this different from the existing ReadAcceleration and ReadRotation interface (except for the name)?

@soypat
Copy link
Contributor Author

soypat commented Sep 1, 2021

@aykevl

  • ReadMeasurement implies there is a transaction with the sensor over some communication protocol.
  • Measurement returns the measurement, which may have been done beforehand or may be done on the fly.

The reason's for this change are described #298 with a real world example. To put it simply, in my experience the less bus transactions, the better. Also there is no error indicator on the measurement done! How are we sure all bytes came through OK? This is solved adding a Get() error method for getting data and storing it in the struct. We know if the get failed by checking the error. Keep in mind accelerations could be used by more than one library in a project... in this case each library is fighting over the i2c bus. I've had issues with the MPU6050 which hangs if being requested frequently.

@ysoldak
Copy link
Contributor

ysoldak commented Sep 1, 2021

I'm not sure about this.
AHRS is more like application of sensors, their fusion.
While this repo is about drivers, ability of working with peripherals at all.

I'd probably buy imu.go to have a standard interface for IMU drivers...

@soypat
Copy link
Contributor Author

soypat commented Sep 2, 2021

@ysoldak Sounds reasonable. AHRS does imply the system has attitude and heading knowledge, which is not the case given the methods. I think ARS could be renamed to be IMU to reflect its nature better. Still thinking of a best name for current AHRS interface...

@aykevl
Copy link
Member

aykevl commented Sep 5, 2021

I'm not sure about the name either, but actually I don't think we need to define it anywhere anyway. (Although it would be good for documentation).

I do see the issue and I considered when I originally defined the interface. At the time I thought a simple ReadAcceleration and ReadRotation would be simpler to use. But you are correct: they do introduce a bit more overhead on the I2C bus. So I think adding new methods for this use case would be a good idea.

  • ReadMeasurement implies there is a transaction with the sensor over some communication protocol.

  • Measurement returns the measurement, which may have been done beforehand or may be done on the fly.

I think we should define the Measurement method in a way that it does not read from the device, but instead only returns the previously read value. This previously read value may be obtained from a previous ReadMeasurement call or a combined Get or Update method call.

All in all I agree this is a good idea. It does require a bit more memory to store the result, but this is very minimal (maybe 24 bytes total).

Regarding the update method, I would suggest the name Update (not Get). We might also want to make it more general. For example:

// in the drivers package
type Measurement uint32
const (
    Temperature = 1 << iota
    Humidity
    Rotation
    Acceleration
    MagneticField // not sure about this name
    // ...etc
)

// In a driver
// Update reads the various measurements in one batch, as far as possible.
func (d *Device) Update(which Measurement) error {
    if which & drivers.Temperature != 0 {
        // read temperature
    }
}

// In a user package
device := ...
for {
    err := device.Update(drivers.Acceleration | drivers.Rotation)
    if err != nil { handle }
    ax, ay, az := device.Acceleration()
    rx, ry, rz := device.Rotation()
    time.Sleep(time.Second)
}

Some devices have multiple sensors, not all of which would be useful to update all the time. For example, the BMI160 supports temperature, acceleration, and rotation, but most people will only be interested in just acceleration and rotation. So we need a way to pick only some measurements.

Maybe there is a simpler way than a bitfield?

@soypat
Copy link
Contributor Author

soypat commented Sep 6, 2021

I personally really like Update(which Measurement), I think it lends itself to really great abstractions as the system gets complex. You could group all Update receivers in a slice and iterate over them when you need to update the state while excluding unwanted values, for example.

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.

I'm unsure on the impact of either, although the second does feel less like magic.

@soypat
Copy link
Contributor Author

soypat commented Sep 9, 2021

@aykevl @ysoldak I've reduced the scope of the interface addition. I'll add a issue for the Measurement and Update ideas.

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