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!

[gen2thomas]

Thanks for the work on this issue!

I stumbled over this when I tested my implementation (uses BME280) this week without the sensor connected and the MCU freezes sooner or later (2min .. 1h). I call the driver each 10sec. What fixes it, is the same like @muktihari has now implemented in BMP280 for read and write, but unfortunately not for "Connected()". There is still this implementation, which IMO allocates heap 2 times on each call.

// Connected returns whether a BMP280 has been found.
// It does a "who am I" request and checks the response.
func (d *Device) Connected() bool {
	data := make([]byte, 1)
	legacy.ReadRegister(d.bus, uint8(d.Address), REG_ID, data)
	return data[0] == CHIP_ID
}

My current workaround, in my local BME280 driver, looks like this - I have introduced 2 different arrays for register and for the data, but maybe this is not really needed or at least not possible on driver side (without removing the "legacy" calls):

// Connected returns whether a BME280 has been found.
// It does a "who am I" request and checks the response.
func (d *Device) Connected() bool {
	//data := []byte{0}
	d.dataBuf[0] = 0
	data := d.dataBuf[:1]
	d.regBuf[0] = WHO_AM_I
	reg := d.regBuf[:1]
	//legacy.ReadRegister(d.bus, uint8(d.Address), WHO_AM_I, data)
	//d.bus.Tx(uint16(d.Address), []byte{WHO_AM_I}, data) // also freezes after some time
	d.bus.Tx(uint16(d.Address), reg, data)
	return data[0] == CHIP_ID
}

Most likely there is still a heap allocation in line 212 of the BMP280, which can be corrected at driver side:

legacy.WriteRegister(d.bus, uint8(d.Address), REG_CTRL_MEAS, []byte{byte(config)})

Would it be possible to fix the "Connected" function in the same way? Can we take over the complete fix for "BME280"? Maybe it would be possible also to take over this for "BMP180", "BMP388".

[gen2thomas]

@muktihari I also had the idea to print the heap during run, but than I found this options very useful:
"tinygo build -print-allocs=bme280 -size=short -target=main.go"

With this I can found most of my "bad heap implementations" in all of my packages.

[gen2thomas]

What fixes it, is the same like @muktihari has now implemented in BMP280 for read and write, but unfortunately not for "Connected()".

No, unfortunately not fixed - it runs only much longer than before, but freezes again. See my new issue for tinygo.

[gen2thomas]

See also discussion in #766 - looks like a generic way. Or using the regmap idea? So, removing legacy stuff from i2c drivers would be another issue.

@deadprogram no objections from my side to close this issue, because it is already contained since v0.32.0.