Skip to content

spi_tranfer: factorize common code to simplify maintenance #1

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

Merged
merged 1 commit into from
Feb 7, 2020

Conversation

ABOSTM
Copy link

@ABOSTM ABOSTM commented Feb 7, 2020

Summary
Hi @stas2z
I review the PR
stm32duino#912
That is nice job and looks good to me.
Nevertheless I propose to factorize common code to improve readability and maintenance.
I does not impact performances. (tested on NUCLEO_L476RG with LCD/SD sadafruit shield).

@stas2z
Copy link
Owner

stas2z commented Feb 7, 2020

Actually, im sure it does impact perfomance cuz it have to check skipReceive at every byte sent instead of checking it just before transfer loop start

ive checked it with my lcds, it takes about 5% of transfer perfomance (simple buffer-to-screen test case) no matter of optimisations enabled (ive checked -Os/-O2/-O3 as most common)

sure it's not so much and probably we can waste it for better readability

@ABOSTM
Copy link
Author

ABOSTM commented Feb 7, 2020

I am surprised, because:

  • My LCD library call transfer() for each byte, so it doesn't make a difference
  • If library send multiple bytes at once, then you are right, the check is done at each byte, but it is done while byte is being transferred so it should not be visible.

Whatever, as you said 5% is worth for better readability.
So if you agree, you can merge this PR (as it is in your fork) and it will automatically push the update to Arduino_Core_STM32 into your original PR

@stas2z stas2z merged commit da74ede into stas2z:master Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants