proposal: bmp280: remove allocation on read sensor data

[muktihari]

When we read temperature or pressure, it will call this following line:
https://github.com/tinygo-org/drivers/blob/156d6e7c9ce473be185e1554c9da3c2983e6e26a/bmp280/bmp280.go#L220C1-L223C18

Which creates a slice with a dynamic n capacity. Since it's a dynamic slice, it's typically heap-allocated. The following program will prove that the allocation is indeed happening:

package main

import (
	"machine"
	"runtime"
	"time"

	"tinygo.org/x/drivers/bmp280"
)

func main() {
	machine.I2C1.Configure(machine.I2CConfig{
		Frequency: 400 * machine.KHz,
		SDA:       machine.GP6,
		SCL:       machine.GP7,
	})

	sensor := bmp280.New(machine.I2C1)
	sensor.Address = 0x76
	sensor.Configure(bmp280.STANDBY_125MS, bmp280.FILTER_4X, bmp280.SAMPLING_16X, bmp280.SAMPLING_16X, bmp280.MODE_FORCED)

	go func() {
		var init, memstats runtime.MemStats
		runtime.ReadMemStats(&init)
		for {
			runtime.ReadMemStats(&memstats)
			println("#", init.HeapAlloc, memstats.HeapAlloc, memstats.HeapAlloc-init.HeapAlloc)
			time.Sleep(1 * time.Second)
		}
	}()

	for {
		temp, _  := sensor.ReadTemperature()
		println("temp:", temp)
		time.Sleep(time.Second)
	}
}

Output:

➜ tinygo monitor
Connected to /dev/ttyACM0. Press Ctrl-C to exit.
# 16448 16496 48
temp: 31560
# 16448 16512 64
temp: 31560
# 16448 16528 80
temp: 31560
# 16448 16544 96
temp: 31570
# 16448 16560 112
temp: 31570
# 16448 16576 128
temp: 31570

I believe that reading from a sensor is considered a hotpath as users may frequently read data from it. So I propose fixing this by creating fixed slice from the caller (ReadTemperature and ReadPressure) and then pass the slice to readData method, instead of passing the capacity into it, e.g.:

func (d *Device) ReadPressure() (pressure int32, err error) 
- data, err := d.readData(REG_PRES, 6)
- if err != nil {
+ data := make([]byte, 6)
+ if err = d.readData(REG_PRES, data); err != nil {
	return
}

In my testing, this changes remove the allocation:

➜ tinygo monitor
Connected to /dev/ttyACM0. Press Ctrl-C to exit.
# 16448 16448 0
temp: 31880
# 16448 16448 0
temp: 31890
# 16448 16448 0
temp: 31890
# 16448 16448 0
temp: 31900
# 16448 16448 0
temp: 31900
# 16448 16448 0
temp: 31900

I can submit a PR if this approach is accepted. Thanks.

[aykevl]

Yes, this would be a very useful fix!

Many drivers also have a small array in the device struct for this purpose. Something like this:

type Device {
    // other fields
    buf [8]uint8
}

Which can then be used as a slice using d.buf[:6] for example.

[muktihari]

Thanks a lot for the suggestion, I notice a similar pattern in other drivers (e.g. on bmi160). I agree that having the same pattern would be very helpful for code maintainability. However, I have concern regarding this approach:

This sensor has two APIs: ReadTemperature and ReadPressure which might be called concurrently, I'm afraid that using a small shared array might lead to a data race condition. I don't know whether microcontroller can be fast enough to process the data that might lead to that condition, but there is tiny gap between reading from the buffer and reading from the bus (filling the buffer with new data) when we do it concurrently.

The chances are very small, but I believe it’s not zero, especially once tinygo support multicore (?) CMIIW.

For that reason, I propose creating the array/slice in the API functions. Since it's only 3 and 6 bytes respectively for each API, creating them in the stack would be very cheap.

[muktihari]

Please disregard my previous comment. It seems you're suggesting creating a large enough buffer ([9]byte in this case) then re-slicing it accordingly to avoid sharing memory (in case the buffer escape, we only allocate once). Am I understand that correctly?

[aykevl]

Drivers are typically not concurrency-safe, so this is not problem. People using these drivers should make sure to use proper locking if they want to use the same driver instance from multiple goroutines.

One reason for this is that the I²C bus is not concurrency-safe. Only one transaction can happen at a time.

[muktihari]

Ah, I see. I understand now. I will submit the PR shortly. Please let me know if any changes to the code are required.

[deadprogram]

Labeled to close this issue on next release. Thanks everyone!