Spi panics when closing program with Raspberry and mcp3008

[tux-eithel]

Hello!

I'm using gobot @ 1.14.0, but the problem occurs also with 1.13.0.

I'm having a panic when I hit ctrl-c to close my program. Let's see the details.

What

I'm doing a simple example with mcp3008 chip and RaspberryPI. All works fine, until ctr-c is hit and the software crash. This is the stack:

^C2019/10/20 14:57:58 Stopping Robot Light Sensor ...
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0xec6a4]

goroutine 1 [running]:
periph.io/x/periph/host/sysfs.(*SPI).Close(0x1cb2000, 0x0, 0x0)
	/home/example-user/Go/pkg/mod/periph.io/x/[email protected]+incompatible/host/sysfs/spi.go:62 +0x70
gobot.io/x/gobot/drivers/spi.(*SpiConnection).Close(0x1c840c0, 0x1c6a8c0, 0x1c63e1c)
	/home/example-user/Go/pkg/mod/gobot.io/x/[email protected]/drivers/spi/spi.go:69 +0x28
gobot.io/x/gobot/platforms/raspi.(*Adaptor).Finalize(0x1c90120, 0x0, 0x0)
	/home/example-user/Go/pkg/mod/gobot.io/x/[email protected]/platforms/raspi/raspi_adaptor.go:123 +0x2fc
gobot.io/x/gobot.(*Connections).Finalize(0x1c740b0, 0x0, 0x0)
	/home/example-user/Go/pkg/mod/gobot.io/x/[email protected]/connection.go:64 +0x78
gobot.io/x/gobot.(*Robot).Stop(0x1c86050, 0x0, 0x1c82240)
	/home/example-user/Go/pkg/mod/gobot.io/x/[email protected]/robot.go:208 +0xb8
gobot.io/x/gobot.(*Robot).Start(0x1c86050, 0x0, 0x0, 0x0, 0x1, 0x1c84060)
	/home/example-user/Go/pkg/mod/gobot.io/x/[email protected]/robot.go:194 +0x1f0
main.main()
	/home/example-user/Go/src/gitlab.com/eithel/stupid-smart-things/mcp/main.go:38 +0x1bc

What I was expecting is a normal ending of the program.

How to reproduce

This is the simple program to reproduce the issue.

package main

import (
	"log"
	"time"

	"gobot.io/x/gobot"
	"gobot.io/x/gobot/drivers/spi"
	"gobot.io/x/gobot/platforms/raspi"
)

func main() {

	// Create the reaspberry.
	r := raspi.NewAdaptor()

	// Create the MCP driver.
	// This driver is useful to read some analogic.
	mcp := spi.NewMCP3008Driver(r, spi.WithSpeed(1350000))
	work := func() {
		for {
			newValue, err := mcp.Read(2)
			if err != nil {
				log.Println("Error:", err)
			}
			log.Println(newValue)
			<-time.After(200 * time.Millisecond)
		}
	}

	lightRobot := gobot.NewRobot("Light Sensor",
		[]gobot.Connection{r},
		[]gobot.Device{mcp},
		work,
	)

	err := lightRobot.Start()
	if err != nil {
		log.Fatalln("Unable to start robots:", err)
	}

}

after running the software, if you hit ctrl-c, panic occurs.

Why

Looking at the panic output, the nil pointer dereference occurs here https://github.com/google/periph/blob/v3.6.2/host/sysfs/spi.go#L62 because the s.conn.f is nil. As you can see, the function itself sets s.conn.f to nil (row 65). I thought that maybe that function is called twice.

Looking how a robot is closed, https://github.com/hybridgroup/gobot/blob/v1.14.0/robot.go#L201-L216 , I saw that all the devices will be Halt, and all the connections will be Finalize.

Here is where the double call happens: the device, mcp3008, close the spi connection here: https://github.com/hybridgroup/gobot/blob/v1.14.0/drivers/spi/mcp3008.go#L70-L74 then the raspberry spis connection will be finalized here https://github.com/hybridgroup/gobot/blob/v1.14.0/platforms/raspi/raspi_adaptor.go#L121-L127 . This second call causes the panic.

Thoughts for a solution

I don't know if the bug is imputable to:

1. my program is wrong
2. how gobot manage spi devices
3. a missing nill check on periph library
4. it behaves as design

If is "1", please, let me know what I'm doing wrong!

If you think is only the "3", feel free to close the issue, and eventually I'll open an issue on periph repo.

Looking at the current code state I think there is also a little bit of "2": every drivers/spi/* use Start function to initialize the driver.
This function use the connector to create the spi connection d.connector.GetSpiConnection(bus, chip, mode, bits, maxSpeed), so why don't we give the same responsibility - to the connector - also to close the channel?

If it is 4, sorry for this issue, feel free to close it.

For now, I've edit a little bit my program, so panic doesn't occur:

package main

import (
	"log"
	"time"

	"gobot.io/x/gobot"
	"gobot.io/x/gobot/drivers/spi"
	"gobot.io/x/gobot/platforms/raspi"
)

func main() {

	// Create the reaspberry.
	r := raspi.NewAdaptor()

	// Create the MCP driver.
	// This driver is useful to read some analogic.
	mcp := spi.NewMCP3008Driver(r, spi.WithSpeed(1350000))
        mcp.Start() // We manually start the device
	work := func() {
		for {
			newValue, err := mcp.Read(2)
			if err != nil {
				log.Println("Error:", err)
			}
			log.Println(newValue)
			<-time.After(200 * time.Millisecond)
		}
	}

	lightRobot := gobot.NewRobot("Light Sensor",
		[]gobot.Connection{r},
		work, // We don't pass anymore the device to the robot
	)

	err := lightRobot.Start()
	if err != nil {
		log.Fatalln("Unable to start robots:", err)
	}

}

Thanks for the help!

[deadprogram]

Hello @tux-eithel thanks you for looking at this.

I think you are absolutely correct, the Halt() of a Driver is only supposed to cleanup after itself, not close anything on the Connection.

This is the only one that is actually correct: https://github.com/hybridgroup/gobot/blob/v1.14.0/drivers/spi/ssd1306_driver.go#L273

All of the other SPI drivers need to have that line removed e.g. https://github.com/hybridgroup/gobot/blob/v1.14.0/drivers/spi/mcp3304.go#L72

We would be most grateful to receive a PR with this if you have time.

Thank you!

[tux-eithel]

Hi @deadprogram , sorry for the double commit in the issue history (i didn't signed-off the first one ), but the pull request has only one commit.

I've tried to do an integration test (like TestRobotStartAutoRun), using directly the NewMCP3008Driver but the I've got this error: import cycle not allowed in test....