MCP23017 driver and Raspberry Pi - Read Buffer not correct

[barrowkwan]

I am trying to use MCP23017 with Raspberry PI to turn PortA pin 0-7 ON ( ie write "1" to port A , pin 0-7). However, the driver didn't keep already "ON" pin to stay "ON". Basically it can only keep one and only one of the pin 0-7 "ON". There is no way to turn more than one pin ON at the same time.

After some debugging, it turns out the "read" function in mcp23017_driver.go return the wrong buffer. "IODIR" supposed to be in buf[20] but it was in buf[19].

I don't know if this is a problem specific to RPi. I have tried both RPi2 and RPi3. In order to fix this, I have to change "read" function to return buf[register-1] instead.

if register != 0 {
	return buf[register-1], nil
} else {
	return buf[register], nil
}

[deadprogram]

@ulisesflynn any comments on this?

[ulisesflynn]

@barrowkwan I am looking into this now, I'll get back to you on this issue by tomorrow.

[deadprogram]

Hi @barrowkwan there is a PR from @ulisesflynn that appears should fix things that is located here:
#569

Are you able to test it out and ensure it solves your specific issues?

[barrowkwan]

Yes that fixed it. thanks!

[barrowkwan]

Hi
I think this clean up has introduce another bug. I used mcp23017 to drive a 16 relay board, before the fix, I can turn the relay on or off ( I have to use the PinMode method to set the pin input/output mode ). Right now the PinMode function has gone and I can't change the PIN's mode.

After looking at the code, I think the problem is with the WriteGPIO. We should be able to set 1 or 0 to the pin ( ie on or off ). However, if we try to send a value "1" ( ie write 1 to the output pin, it will change the pin to be input instead of output ).

[barrowkwan]

The problem is with the WriteGPIO function. It should always use clearBit and write to IODIR. so instead of

	// set or clear iodir value, a 1 sets the pin as an input, 0 as an output
	var iodirVal uint8
	if val == 1 {
		iodirVal = clearBit(iodir, uint8(pin))
	} else {
		iodirVal = setBit(iodir, uint8(pin))
	}
	// write IODIR register bit
	err = m.write(selectedPort.IODIR, uint8(pin), uint8(iodirVal))
	if err != nil {
		return err
	}

it should just

	// set or clear iodir value to make pin in an output mode
	err = m.write(selectedPort.IODIR, uint8(pin), uint8(clearBit(iodir, uint8(pin))))
	if err != nil {
		return err
	}

[deadprogram]

@ulisesflynn can you please help out again?

Seems I need to obtain a MCP23017 for the workshop so I can also help test this out.

[ulisesflynn]

I'll look at this tomorrow and see what's going on, I apologize for my slow response.

[ulisesflynn]

@barrowkwan I've pushed up a PR for the writegpio/readgpio code, let me know if this fixes it.

[barrowkwan]

yes, that fixed it. thanks!

[ulisesflynn]

Thanks again @barrowkwan for testing this out.

[deadprogram]

This has just been released as part of v1.11.1 so now closing. Thanks again!

[gen2thomas]

The rework of read() and other cleanups (except leftover #810) in PR #569 and #576 are fine and improve the code readability.

So, for completeness only, after some investigation for #810:
The root cause of the issue #568 is caused by the missing reset of read pointer in function read() before each read operation. This leads to something like a "random read" of any address after the last access.

The first assumption of the issuer, that "return buf[register-1], nil" would fix the bug, could maybe work in special cases, but will not work in general.

The smallest fix would be adding following lines before start reading, e.g. (first 3 lines with code added):

func (m *MCP23017Driver) read(reg uint8) (val uint8, err error) {
	// reset to position 0, that fixes the originated problem of #568
	// because the sequential read will change the pointer on each access
	if _, err = m.connection.Write([]uint8{0x00}); err != nil {
		return val, err
	}
	// read
	register := int(reg)
	bytesToRead := register + 1
	buf := make([]byte, bytesToRead)
	bytesRead, err := m.connection.Read(buf)