Skip to content

lsm6ds3tr: avoid unnecessary heap allocations #766

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

Open
wants to merge 2 commits into
base: release
Choose a base branch
from

Conversation

ysoldak
Copy link
Contributor

@ysoldak ysoldak commented Jun 19, 2025

This re-uses internal buffer to avoid unnecessary heap allocations in lsm6ds3tr driver; switches away from the legacy I2C interface.

Copy link
Contributor

@soypat soypat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit that I think would do wonders for readability. I'd love to feature this in a youtube video once it is done if y'all don't mind!

Comment on lines 109 to 111
d.buf[0] = CTRL2_G
d.buf[1] = uint8(d.gyroRange) | uint8(d.gyroSampleRate)
err = d.bus.Tx(d.Address, d.buf[0:2], nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we abstract these calls to something like d.writeReg8/readReg8 ?

func (d *Device) writeReg8(addr, value byte) error {
   d.buf[0] = addr
   d.buf[1] = value
   return err = d.bus.Tx(d.Address, d.buf[0:2], nil)
}

func (d *Device) readReg8(addr byte) (byte,error) {
   d.buf[0] = addr
   err := d.bus.Tx(d.Address, d.buf[0:1], d.buf[1:2])
   return d.buf[1], err
}

Copy link
Contributor Author

@ysoldak ysoldak Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just one of many drivers that scream for such improvement, a trial balloon if you wish.
Each of these drivers will have to repeat the pattern.

Any ideas how could we implement these helper functions once and re-use them, instead of implementing in every driver anew?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soypat @aykevl I'm fine with adding helper functions. Shall I just add them and replicate the pattern in other drivers? Or is there a better way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created regmap. Thanks for the suggestion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add them for now and we can port it to use regmap after regmap has been merged

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I agree with @soypat some helper methods would make the code a bit easier to read.

@ysoldak
Copy link
Contributor Author

ysoldak commented Jun 19, 2025

Nit that I think would do wonders for readability. I'd love to feature this in a youtube video once it is done if y'all don't mind!

Nothing against from my side. All kudos to @aykevl for the idea, btw.

@ysoldak
Copy link
Contributor Author

ysoldak commented Jun 21, 2025

Alright, I've implemented the helper functions. Had to implement not only pure read and write, but setBits() too -- for convenience.

Also, I've decided to simply pass value size to readValue() with contract being the data is written into shared buffer with offset 1 -- first byte is needed for the register number.
We could probably pass a destination slice, but we still need to source it somewhere, and in the scope of this driver it always be a part of the shared buffer anyway. And also anyway, we have to know the internals of the helper method and take into account the size of the shared buffer to chop the destination slice correctly.

An alternative could be we return read data w/o offset, i.e. read into the shared buffer from index 0.
Probably this is a cleaner interface but can be fragile: this assumes too much about I2C implementation, what it does not clear the destination slice first, for example, effectively wiping the register number.
UPD: I've tried, this does not work ☝️
UPD2: This actually works, while still being fragile and assuming perhaps too much about I2C implementation, see #769

@ysoldak ysoldak force-pushed the no-legacy-lsm6ds3tr branch from 890aae3 to a6949c5 Compare June 21, 2025 16:29
@soypat
Copy link
Contributor

soypat commented Jun 22, 2025

An alternative could be we return read data w/o offset, i.e. read into the shared buffer from index 0.

It is unclear to me what is gained from sharing the first byte, other than 1 byte of memory (likely not gained due to alignment requirements).

If for some reason you want your data read to be at the first byte of the buffer you can declare two static buffers:

type Dev struct { cmdbuf [1]byte; databuf [N]byte }

func (d *Dev) read(n int) error {
   return d.bus.tx(addr, d.cmdbuf[:], d.databuf[:n])
}

Now the intention is clear, there is a databuf which accumulates data read which is already quite self-documenting

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