Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement sequential read/write for 16-bit operations #43

Merged
merged 6 commits into from
Jun 11, 2024

Conversation

olkal
Copy link
Contributor

@olkal olkal commented Jun 10, 2024

Hi, this PR is implementing sequential read/write for 16-bit operations as described in the data sheet 3.2.1 page 13.
See test data below from running the MCP23S17_performance.ino sketch on an ESP32-S3@240mhz.

HW_SPI@8mhz before:
	TEST write16(mask): 13.50
	TEST read16(): 13.50

HW_SPI@8mhz with sequential read/write:
	TEST write16(mask): 10.50
	TEST read16(): 10.50

SW_SPI before:
	TEST write16(mask): 29.50
	TEST read16(): 29.50

SW_SPI with sequential read/write:
	TEST write16(mask): 20.00
	TEST read16(): 20.00

@RobTillaart
Copy link
Owner

Great addition!
Will look into it later this week

@RobTillaart RobTillaart self-assigned this Jun 10, 2024
@RobTillaart RobTillaart added the enhancement New feature or request label Jun 10, 2024
@RobTillaart
Copy link
Owner

This addition was on my todo list, so I'm happy you implemented and tested it.

The POR (power on reset) value of the SEQOP bit is zero ==> sequential read/write mode.

However as we will not be sure if the value is set to zero, I propose to clear that bit explicitly

Add in the begin() function. line 73++ something like

uint8_t reg = readReg(MCP23017_IOCR);
if (reg & MCP23017_IOCR_SEQOP)  //  check if already zero
{
  reg &= ~MCP23017_IOCR_SEQOP;  //  clear SEQOP bit for sequential read/write
  if (! writeReg(MCP23017_IOCR, reg)) return false;
}

image

@olkal
Copy link
Contributor Author

olkal commented Jun 11, 2024

Yes, I agree. Should I also remove line 73 then? // if (! writeReg(MCP23017_IOCR, MCP23017_IOCR_SEQOP)) return false; I don't see any any reason to keep it.

MCP23017_IOCR to MCP23x17_IOCR
@RobTillaart
Copy link
Owner

Please remove line 73 as it has no function

Removed line 73
@RobTillaart
Copy link
Owner

RobTillaart commented Jun 11, 2024

LGTM
The 16 bit calls now only transfer 4 bytes instead of 6 bytes which can be seen in your performance measurements.
Will squash and merge it now and bump the version number tomorrow.

@RobTillaart RobTillaart merged commit 7de0753 into RobTillaart:master Jun 11, 2024
3 checks passed
@RobTillaart
Copy link
Owner

@olkal
Did performance test on AVR UNO with the optimized 16 bit and the relative gain is again ~30%.
Especially for SW SPI the absolute improvement is substantial.

function 0.5.2 0.5.3 notes
SW SPI write16 450 300
SW SPI read16 446 298
HW SPI write16 42 26 1 MHz
HW SPI read16 42 28 1 MHz
HW SPI write16 28 18 2 MHz
HW SPI read16 30 18 2 MHz
HW SPI write16 22 16 4 MHz
HW SPI read16 24 16 4 MHz
HW SPI write16 20 14 8 MHz
HW SPI read16 20 14 8 MHz

RobTillaart added a commit that referenced this pull request Jun 12, 2024
- #43 optimize read/write16, kudos to Olkal
- add performance test output 0.5.2, 0.5.3
- add datasheet REV C and REV D to repo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants