Skip to content

Conversation

@soypat
Copy link
Contributor

@soypat soypat commented Sep 26, 2021

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
}

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.

@aykevl
Copy link
Member

aykevl commented Oct 22, 2021

I like this proposal!

I'm not entirely sure about the Audio measurement type, 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). However, that's only one detail.

@deadprogram @conejoninja @ysoldak @sago35 any opinion on this?

@ysoldak
Copy link
Contributor

ysoldak commented Oct 22, 2021

So, for say, Barometer, we gonna have 3 methods in a typical driver implementation:

Update(which Measurement) error // update
Pressure() int32                // read
ReadPressure() (int32, error)   // update & read

Will it not be confusing?
Or we willing to drop ReadPressure()?
Shall we standardise on error emitted then incompatible Measurement type requested?

@soypat
Copy link
Contributor Author

soypat commented Oct 22, 2021

@aykevl I'll remove the Audio type now. We can always add it later.

@ysoldak Maybe we can discard the ReadMeasurement methods, it does seem to lend itself to being confusing having two methods with similar name and functionality. There are some benefits to this method:

  • 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)
  • 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.

@ysoldak
Copy link
Contributor

ysoldak commented Oct 25, 2021

I wonder if we could have a feasibility study.

Branch from this PR and try and use this proposed interface in existing drivers.
Do not need to cover all the drivers, but at least several and more diverse -- the better.
Make a new related PR to show how it gonna look like.
Without this ground work it is hard to settle on anything, IMHO. Too many corner cases may pop-up that can be awkward to workaround or fix later.

@soypat
Copy link
Contributor Author

soypat commented Nov 30, 2021

Closing in favor of #345.

@soypat soypat closed this Nov 30, 2021
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