LIS3DH: modernize driver #798
Conversation
lis3dh/lis3dh.go
Outdated
|
|
||
| // enable all axes, normal mode | ||
| legacy.WriteRegister(d.bus, uint8(d.Address), REG_CTRL1, []byte{0x07}) | ||
| err := legacy.WriteRegister(d.bus, uint8(d.Address), REG_CTRL1, []byte{0x07}) |
There was a problem hiding this comment.
Ideally these function calls could be rewritten as a write8 method. Also protip: dev already has the regmap package which exposes a nice heapless API to interact with these type of devices and can potentially remove a lot of boilerplate driver register code: https://github.com/tinygo-org/drivers/blob/dev/internal/regmap/devices.go :)
There was a problem hiding this comment.
Yes, we should avoid the legacy code here. I would prefer to do that in a separate PR however, since there are different ways we can do this. (And also I'd like to get this one merged so I can use the new API in the tour).
I'm a bit late to the party, but I looked at the regmap package and I'm not sure whether this is the best API? I do like the direction, but:
- How should I call it in the
Deviceobject itself, as a member variable? I guess you'd call itregmapor something? - It's a bit clunky to have to write
Write8I2Cetc, when the device is only accessible over I2C anyway. Why not split the I2C and SPI versions of theDevice8into different types, likeDevice8I2Cor whatever? Then you can use the somewhat more ergonomicWrite8. - The size of the buffer is not configurable, it's always 10 bytes. Perhaps that's fine, 10 bytes isn't a lot on modern microcontrollers anyway (a pointer is 4 bytes, a function value is 8, etc). But perhaps this can be made configurable with generics?
(To be honest this is almost bikeshedding, but, wanted to raise the points anyway).
There was a problem hiding this comment.
Alright, we can leave it for future PR, if any.
I'm not sure whether this is the best API?
It's a good start I'd argue. We had nothing before. It is in an internal package so discussing and modifying is zero cost (I'll take care of it :))
I2C and SPI versions of the Device8 into different types, like Device8I2C or whatever?
Totes! We can add Device8I2C and Device8SPI and have these be the suggested types over Device8. I do feel like having Device8 is not a terrible idea...
- Maybe there are cases where developer does not want to have the i2c bus live in the struct (SPI or I2C multiplexer yielding several distinct i2c HALs from single peripheral, actually now I think of it this is reason enough to keep Device8 separate)
- Cases where one may want the bus be an argument so the compiler can more easily devirtualize the type (¿even a thing¿???).
- It is nice to be able to be able to use this type and requiring absolutely no initialization. A pattern I'd wish to see more often :)
Dunno, took some creative liberties there to explore the idea of a bare-minimum struct since we are not committing to anything yet since it is in internal package.
Then you can use the somewhat more ergonomic Write8
100% on board. I'll add Device8I2C and Device8SPI in a PR right now.
The size of the buffer is not configurable, it's always 10 bytes. Perhaps that's fine, 10 bytes isn't a lot on modern microcontrollers anyway (a pointer is 4 bytes, a function value is 8, etc). But perhaps this can be made configurable with generics?
10 bytes is the minimum size to read a 32bit integer from a 8 bit device through SPI due to requiring 5 byte transaction. I2C could get away with 5 bytes- though when creating the DeviceI2C that will align to 4 bytes on most microcontrollers leaving you with likely 4+4=8 bytes vs 12 bytes with the [10] array, not just 5. I'd argue it really is not worth adding complexity to save 4 bytes in the I2C case.
In any case 10 bytes is super small, shouldn't be a problem- lets encourage people to use Device8I2C and Device8SPI so if we wish to change this in the future it is not too hard to change
| func (d *Device) Update(which drivers.Measurement) error { | ||
| if which&drivers.Acceleration != 0 { | ||
| // Read raw acceleration values and store them in the driver. | ||
| err := legacy.WriteRegister(d.bus, uint8(d.Address), REG_OUT_X_L|0x80, nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = d.bus.Tx(d.Address, nil, d.accel[:]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Nice! :D
Question: Do we prefer nested if with the "measure" logic as shown here, or early return if?
func (d *Device) Update(which drivers.Measurement) error {
if which&drivers.Acceleration == 0 {
return nil
}
// ...There was a problem hiding this comment.
Hmm. I could argue both ways.
- Early return is common in Go code, and I agree with the general pattern. It makes code easier to read.
- A pattern in other drivers could be to match different
whichsensor types, and in that case early return becomes impossible.
No strong preference, I can switch it over if you like.
There was a problem hiding this comment.
I'd err on the early return where convenient and maybe a switch/ifelse statement in cases with multiple which bits.
Instead of printing an error, this driver really should be returning errors instead. Also, `Configure` didn't have a way to actually configure the driver. This is now added, and can be expanded in the future. This is a breaking change.
This adjusts the API to the one proposed in #345, which I think is much better than direct ReadAcceleration etc calls. I have also updated the code that converts raw acceleration values to normalized values. The new code should be faster (didn't measure) and avoids floating point math.
aykevl
force-pushed
the
lis3dh-updates
branch
from
281eb76 to
ff56002
Compare
|
Updated the PR:
|
Hmm, that's a good idea. Not sure how to best explain this though, my approach is very pragmatic. (Also, I tend to forget that not everybody knows the intricacies of which operations are fast and how to use integer operations in places where floats would have been a more natural fit). |
2 participants
This updates the LIS3DH driver to be more in line with our new API conventions, and also just best practices:
lis3dh.Configobject, so it's actually possible to configure the driver (instead of using the default configuration inConfigure).UpdateandAccelerationmethods, following add Sensor interface and Measurement type #345.This is a backwards breaking change, but I think it is one we need to make. The changes are relatively small though, and will result in compiler errors that should be obvious.
(Context: I'm working on a new part of the I2C tour that will use a driver instead of raw I2C calls, and I want to give a good example)