Mpu6050 rewrite by soypat · Pull Request #556 · tinygo-org/drivers

soypat · 2023-04-29T15:42:35Z

Resolves #544.

I have published the mpu6050 driver I've used independently for years now.

It has greatly increased functionality and is much faster and efficient than the existing implementation. I have broken API compatibility since the existing implementation was extremely limited. In doing so I've taken the liberty of implementing patterns from the RFC #310.

New Features:

Configurable acceleration range
Configurable angular velocity range
Extensible configuration type added for future non-breaking changes
Efficient sensor reading using one-shot methodology
Expose raw data for even more efficient algorithms for power users.

aykevl · 2023-04-29T21:20:21Z

examples/mpu6050/main.go

-		x, y, z := accel.ReadAcceleration()
-		println(x, y, z)
 		time.Sleep(time.Millisecond * 100)
+		err := mpuDevice.Update()


I believe Update should have a parameter with the sensor values it should update?

Yes, we should probably solve #345 before merging

#345 has been merged, so this can be updated.

I'd leave this for last since I still don't have a good rebase methodology. As soon as everything else is reviewed and OK I'll try my hand at rebasing one more time

mpu6050/mpu6050.go

aykevl · 2023-04-29T21:35:10Z

mpu6050/mpu6050.go

+func DefaultConfig() IMUConfig {
+	return IMUConfig{
+		AccRange:    ACCEL_RANGE_16,
+		GyroRange:   GYRO_RANGE_2000,
+		sampleRatio: 0, // TODO add const values.
+		clkSel:      0,
+	}


Any reason not to use the zero value for these options as the default value?

I'm not sure I understand- I've added them here redundantly just so I can mark it with a TODO

mpu6050/registers.go

deadprogram · 2023-05-20T13:47:52Z

examples/mpu6050/main.go


-	accel := mpu6050.New(machine.I2C0)
-	accel.Configure()
+	mpuDevice := mpu6050.New(machine.I2C0, mpu6050.DefaultAddress)


In other drivers, this has been solved by exporting Address and then just setting it before calling Configure. That way we do not have to break the existing API and all of the many projects that use this driver. I have several such projects myself 😺

mpuDevice := mpu6050.New(machine.I2C0) mpuDevice.Address = 0x66 // overrides default // now do everything else

With Go modules, I'm not too worried about breakage. Yes, it will break once you do a go get -u tinygo.org/x/drivers, but before that it will keep working just fine (and if you're doing a go get -u tinygo.org/x/drivers, you might as well add the default address).

I'm fine either way.

aykevl · 2023-05-20T15:46:38Z

examples/mpu6050/main.go


-	accel := mpu6050.New(machine.I2C0)
-	accel.Configure()
+	mpuDevice := mpu6050.New(machine.I2C0, mpu6050.DefaultAddress)


With Go modules, I'm not too worried about breakage. Yes, it will break once you do a go get -u tinygo.org/x/drivers, but before that it will keep working just fine (and if you're doing a go get -u tinygo.org/x/drivers, you might as well add the default address).

I'm fine either way.

aykevl · 2023-05-20T15:47:22Z

examples/mpu6050/main.go

+	err := mpuDevice.Configure(mpu6050.Config{
+		AccRange:  mpu6050.ACCEL_RANGE_16,
+		GyroRange: mpu6050.GYRO_RANGE_2000,
+	})


...and here we have another breaking change, that I think is actually really useful.

aykevl · 2023-05-20T15:48:00Z

examples/mpu6050/main.go

-		x, y, z := accel.ReadAcceleration()
-		println(x, y, z)
 		time.Sleep(time.Millisecond * 100)
+		err := mpuDevice.Update()


#345 has been merged, so this can be updated.

examples/mpu6050/main.go

mpu6050/mpu6050.go

aykevl · 2023-05-20T16:04:08Z

mpu6050/mpu6050.go

+func (p *Device) SetSampleRate(srDiv byte) (err error) {
+	// setSampleRate
+	var sr [1]byte
+	sr[0] = srDiv
+	if err = p.write8(_SMPRT_DIV, sr[0]); err != nil {
+		return err
+	}
+	return nil
+}


This is a new API.
Why not use time.Duration as an input? Or a sample rate in Hz, as an uint32? (Similar to machine.CPUFrequency() for example). The parameter that you're using here looks like a mpu6050-specific value.

aykevl · 2023-05-20T16:05:18Z

mpu6050/mpu6050.go

+// SetClockSource configures the source of the clock
+// for the peripheral.
+func (p *Device) SetClockSource(clkSel byte) (err error) {
+	// setClockSource
+	var pwrMgt [1]byte
+
+	if err = p.read(_PWR_MGMT_1, pwrMgt[:]); err != nil {
+		return err
+	}
+	pwrMgt[0] = (pwrMgt[0] & (^_CLK_SEL_MASK)) | clkSel // Escribo solo el campo de clk_sel
+	if err = p.write8(_PWR_MGMT_1, pwrMgt[0]); err != nil {
+		return err
+	}
+	return nil
+}


Is this actually useful? Can you give me an example where this would be used in practice?

If not, I suggest removing it to declutter the API.

(Also, it contains a Spanish comment?)

aykevl · 2023-05-20T16:18:27Z

mpu6050/mpu6050.go

+// SetRangeGyro configures the full scale range of the gyroscope.
+// It has four possible values +- 250°/s, 500°/s, 1000°/s, 2000°/s.
+// The function takes values of gyroRange from 0-3 where 0 means the
+// lowest FSR (250°/s) and 3 is the highest FSR (2000°/s).
+func (p *Device) SetRangeGyro(gyroRange byte) (err error) {
+	switch gyroRange {
+	case GYRO_RANGE_250:
+		p.gRange = 250
+	case GYRO_RANGE_500:
+		p.gRange = 500
+	case GYRO_RANGE_1000:
+		p.gRange = 1000
+	case GYRO_RANGE_2000:
+		p.gRange = 2000
+	default:
+		return errors.New("invalid gyroscope FSR input")
+	}
+	// setFullScaleGyroRange
+	var gConfig [1]byte
+
+	if err = p.read(_GYRO_CONFIG, gConfig[:]); err != nil {
+		return err
+	}
+	gConfig[0] = (gConfig[0] & (^_G_FS_SEL)) | (gyroRange << _G_FS_SHIFT) // Escribo solo el campo de FS_sel
+
+	if err = p.write8(_GYRO_CONFIG, gConfig[0]); err != nil {
+		return err
+	}
+	return nil
+}


Can you make gyroRange a type? Like is done in the LIS3DH driver:

drivers/lis3dh/lis3dh.go

Line 62 in 820298b

func (d *Device) SetRange(r Range) {

drivers/lis3dh/registers.go

Lines 54 to 61 in 820298b

type Range uint8

const (

RANGE_16_G Range = 3 // +/- 16g

RANGE_8_G = 2 // +/- 8g

RANGE_4_G = 1 // +/- 4g

RANGE_2_G = 0 // +/- 2g (default value)

)

This makes it possible to easily change the type at a later time, if needed.

aykevl · 2023-05-20T16:21:17Z

mpu6050/mpu6050.go

+// It has four possible values +- 2g, 4g, 8g, 16g.
+// The function takes values of accRange from 0-3 where 0 means the
+// lowest FSR (2g) and 3 is the highest FSR (16g)
+func (p *Device) SetRangeAccel(accRange byte) (err error) {


Same here. Please don't use magic constants (0, 1, 2, 3) and instead instruct users to use named constants.

aykevl · 2023-05-20T16:22:50Z

mpu6050/mpu6050.go

+func DefaultConfig() Config {
+	return Config{
+		AccRange:    ACCEL_RANGE_16,
+		GyroRange:   GYRO_RANGE_2000,
+		sampleRatio: 0, // TODO add const values.
+		clkSel:      0,
+	}
 }


Instead of DefaultConfig, have you considered making the zero value the default config?
That way, users can just do Configure(mpu6050.Config{GyroRange: ...}) and know that the acceleration range is also set to some reasonable default.

I have pushed changes that make the zero value useful in latest commit!

…s; less spanish

soypat · 2023-05-27T18:01:46Z

Closing this PR in favor of #577

soypat added 3 commits April 29, 2023 12:32

rewrite mpu6050 driver implementation to be much more functional

8c80439

fix example

44b049c

remove useless int32 cast

ae9ac8a

soypat changed the base branch from release to dev April 29, 2023 15:42

soypat requested a review from deadprogram April 29, 2023 15:43

soypat mentioned this pull request Apr 29, 2023

Add MPU6886 driver #552

Closed

aykevl reviewed Apr 29, 2023

View reviewed changes

soypat added 2 commits April 30, 2023 10:27

apply Ayke suggestions; rename SetRange* methods

a2124d3

tidy up; remove RawReading; expose RawData field

c4c9bdb

deadprogram reviewed May 20, 2023

View reviewed changes

aykevl reviewed May 20, 2023

View reviewed changes

soypat force-pushed the mpu-rewrite branch from ee65e97 to 8887ad9 Compare May 20, 2023 18:42

add writeMasked; add range types; unexport config methods; less alloc…

d675bd4

…s; less spanish

soypat force-pushed the mpu-rewrite branch from 8887ad9 to d675bd4 Compare May 20, 2023 18:43

make zero value useful for configuration

6288ba5

soypat force-pushed the mpu-rewrite branch from 140f3fe to 6288ba5 Compare May 27, 2023 17:47

soypat mentioned this pull request May 27, 2023

Mpu sensor rewrite 2 #577

Open

soypat closed this May 27, 2023

	type Range uint8

	const (
	RANGE_16_G Range = 3 // +/- 16g
	RANGE_8_G = 2 // +/- 8g
	RANGE_4_G = 1 // +/- 4g
	RANGE_2_G = 0 // +/- 2g (default value)
	)

Conversation

soypat commented Apr 29, 2023

New Features:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

deadprogram May 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soypat commented May 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

deadprogram May 20, 2023 •

edited

Loading