[RSDK-1001] Add support for the ADXL345 accelerometer #1640
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viambot
added
the
safe to test
|
(removing Rand as a reviewer: she's busy, and Nicolas saw the original version of this.) |
viambot
added
safe to test
|
Thanks to @stevebriskin for pointing out that we have a naming convention for these things, and the field should be named |
penguinland
changed the title
npmenard
reviewed
Dec 7, 2022
| } | ||
| localB, ok := b.(board.LocalBoard) | ||
| if !ok { | ||
| return nil, errors.Errorf("board %s is not local", cfg.BoardName) |
npmenard
reviewed
Dec 7, 2022
| } | ||
| bus, ok := localB.I2CByName(cfg.I2cBus) | ||
| if !ok { | ||
| return nil, errors.Errorf("can't find I2C bus '%s' for ADXL345 sensor", cfg.I2cBus) |
npmenard
reviewed
Dec 7, 2022
| // back the device ID (0xE5). | ||
| deviceID, err := sensor.readByte(ctx, 0) | ||
| if err != nil { | ||
| return nil, errors.Errorf("can't read from I2C address %d on bus %s of board %s: '%s'", |
There was a problem hiding this comment.
prefer errors.Wrap or errors.Wrapf
npmenard
reviewed
Dec 7, 2022
| // To do this, we set the Power Control register (0x2D) to turn on the 8's bit. | ||
| err = sensor.writeByte(ctx, 0x2D, 0x08) | ||
| if err != nil { | ||
| return nil, errors.Errorf("unable to put ADXL345 into measurement mode: '%s'", err.Error()) |
npmenard
reviewed
Dec 7, 2022
| // Now, turn on the background goroutine that constantly reads from the chip and stores data in | ||
| // the object we created. | ||
| sensor.activeBackgroundWorkers.Add(1) | ||
| utils.PanicCapturingGo(sensor.pollData) |
There was a problem hiding this comment.
define the function here, we prefer to do it this way so no one else can call this function with later potential work on this driver
There was a problem hiding this comment.
Subtle but wise choice! I should also do that in the MPU code; should that be in this same PR or a separate PR?
There was a problem hiding this comment.
Nicolas and I discussed: it can go in this same PR. I've made similar changes re: helper functions in utils/math.go and reordering the select cases for the MPU6050 code, too.
npmenard
reviewed
Dec 7, 2022
| } | ||
|
|
||
| // A helper function: takes 2 bytes and reinterprets them as a little-endian signed integer. | ||
| func toSignedValue(data []byte) int { |
There was a problem hiding this comment.
can you use one of the utils/math.go function instead? if it doesn't exists might make sense to add it
npmenard
reviewed
Dec 7, 2022
| // The chip starts out in standby mode. Set it to measurement mode so we can get data from it. | ||
| // To do this, we set the Power Control register (0x2D) to turn on the 8's bit. | ||
| err = sensor.writeByte(ctx, 0x2D, 0x08) | ||
| if err != nil { |
There was a problem hiding this comment.
nit: if err := sensor.writeByte(ctx, 0x2D, 0x08); err != nil {
....
}
npmenard
reviewed
Dec 7, 2022
| rawConfig config.Component, | ||
| logger golog.Logger, | ||
| ) (movementsensor.MovementSensor, error) { | ||
| cfg := rawConfig.ConvertedAttributes.(*AttrConfig) |
There was a problem hiding this comment.
cfg,ok := rawConfig.ConvertedAttributes.(*AttrConfig)
npmenard
reviewed
Dec 8, 2022
|
|
||
| for { | ||
| select { | ||
| case <-adxl.backgroundContext.Done(): |
There was a problem hiding this comment.
nit: prefer to have this case last
npmenard
reviewed
Dec 8, 2022
| address = 0x53 | ||
| } | ||
|
|
||
| backgroundContext, cancelFunc := context.WithCancel(ctx) |
npmenard
reviewed
Dec 8, 2022
viambot
added
safe to test
viambot
added
safe to test
|
Excellent feedback! I think I've addressed it all. |
viambot
added
safe to test
|
|
Tried one last time at my desk: both the ADXL and MPU chips still work fine! |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(and while I was looking at it, return any errors encountered when querying the MPU6050 accelerometer, too.)
This PR is adapted from https://github.com/viam-labs/adxl345-accelerometer/pull/1, and when this is merged we should probably delete that other repo.
Tried at my desk: results look plausible. I've got two accelerometers at my desk now, and although they both give okay results, the MPU6050 tends to think gravity is a pretty consistent 13.7% stronger than the ADXL345 thinks it is. Is this because I coded one of them up wrong, or because one of the chips is miscalibrated? Part of me suspects the former, but I don't actually know. 😬
but at the very least, they both give values that are the right order of magnitude, in the right direction.
Sorry the PR is so long; I would have preferred two PRs half the length, but didn't see an easy way to break it up. Maybe next time I can go for the skeleton code, and then the implementation itself?
I'm tagging Rand because she reviewed the MPU6050 code (#1611), and Nicolas because he reviewed https://github.com/viam-labs/adxl345-accelerometer/pull/1. I kinda feel like only one of you needs to review this, but I don't know which that should be.