Skip to content

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