hybridgroup / gobot

Golang framework for robotics, drones, and the Internet of Things (IoT)
https://gobot.io
Other
8.9k stars 1.04k forks source link

I²C reply length should be at least 8 bytes long #487

Closed conejoninja closed 2 years ago

conejoninja commented 6 years ago

The length of currentBuffer (platform/firmata/client/client.go) of the I²C reply is 7, the code assumes it's 8 (or bigger)

I'm getting a strange bug randomly, not sure how to reproduce it or if there's anything wrong with my setup (the board, the connection,... maybe the way I'm reading the i²c replies) I connect through TCPFirmata to an original Wemos D1 mini (it's an ESP8266-12S) I'm developing a new driver (for APDS9960 sensor), I'm reading a bunch of data (128bytes) from an address. I tried several ways, for reading the data (the whole buffer at once or in 32-bytes batches). Not sure if any of this has any relation to the bug, which appear to be random

panic: runtime error: index out of range

goroutine 24 [running]:
gobot.io/x/gobot/platforms/firmata/client.(*Client).process(0xc4200b2000, 0x0, 0x0)
    /home/conejo/workspace/go/src/gobot.io/x/gobot/platforms/firmata/client/client.go:498 +0x1e26
gobot.io/x/gobot/platforms/firmata/client.(*Client).Connect.func6(0xc4200b2000)
    /home/conejo/workspace/go/src/gobot.io/x/gobot/platforms/firmata/client/client.go:230 +0x96
created by gobot.io/x/gobot/platforms/firmata/client.(*Client).Connect
    /home/conejo/workspace/go/src/gobot.io/x/gobot/platforms/firmata/client/client.go:224 +0x6d4
exit status 2

The I²C Reply was [240 119 0 0 24 1 247]

This bugs happens when reading sensor information, sometimes it doesn't fail and read the sensor correctly, sometimes it does fail with the above error. Any idea?

In any case, I²C is assuming the length of the buffer is at least 8 bytes

Data:     []byte{byte(currentBuffer[6]) | byte(currentBuffer[7])<<7},

without any checking

deadprogram commented 6 years ago

There are some real limitations with the TCPFirmata/ESP8266 approach. The main one I ran into was that the Firmata protocol itself is just a little too chatty for the ESP8266 itself. I was locking up the microcontroller forcing it to reboot, due to this, and it took me a while to debug.

The alternative idea I was playing with was doing something based on https://github.com/maruel/homie-esp8266 and MQTT. I need to make some more progress before that could be viable.

deadprogram commented 6 years ago

I meant to link to the main repo on Homie: https://github.com/marvinroger/homie-esp8266

conejoninja commented 6 years ago

Thanks for the feedback, this happened with an Arduino Pro (through serial) too. I will give it another try, with different ways of reading the sensor I wasn't getting this error any more, but the ESP8266 was locking/rebooting itself.

deadprogram commented 6 years ago

@conejoninja a couple more questions. Which version of the ESP8266 Firmata are you running? How are you controlling the number of bytes being returned in a single message?

The Firmata spec says that the data being returned uses 7-bit encoding so requires 2 bytes for each byte being moved around in LSB/MSB order, as documented here: https://github.com/firmata/protocol/blob/master/i2c.md#i2c-reply

So there should not be a proper case where there is a single byte being returned. Hope that is clear.

Regarding the ESP8266 resetting itself, that is the HW behavior when a wdt reset condition occurs, which can occur for many different reasons. Might be worth it to update to the master branch version of https://github.com/esp8266/Arduino since there have been a few updates that look like they might apply.

conejoninja commented 6 years ago

I was using the one at firmatabuilder (configurable firmata 2.1.0) which should be the last stable.

I too saw the spec and it was strange, as it looks like the address is not being sent (or is 0?), there's some code for when the register is not specified and something about interrupts, which may be the issue here. I didn't have yet enough time to dig into this issue further.

I tried several ways for reading the data, reading as much bytes as the sensor told me, reading the same amount in 32-bytes batches (as the sensor really does) and reading byte by byte. And several combinations of them, reading less or more data to see if I could trigger the issue or find the bug.

I wasn't sure to open it since by the specs it shouldn't be happening, it was more of a question if anyone has experienced something similar before, I can't consistently reproduce the issue and I'm more inclined to think it's in my code. Feel free to close the issue until I could look further into it.

gen2thomas commented 2 years ago

@deadprogram it seems nobody else has this problem reported yet. Could we close the Issue?

deadprogram commented 2 years ago

Sounds good @gen2thomas now closing.