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

optimized drawPixel() and fixed Arduino IDE 1.8.6/1.8.7 issue #37

Closed
wants to merge 1 commit into from
Closed

optimized drawPixel() and fixed Arduino IDE 1.8.6/1.8.7 issue #37

wants to merge 1 commit into from

Conversation

MrBlinky
Copy link
Contributor

@MrBlinky MrBlinky commented Nov 9, 2018

The new compiler In Arduino IDE 1.8.6/1.8.7 has an optimization glitch where it fails to see the difference between a data structure in RAM and in PROGMEM when the data is the same and optimizes one out.

drawPixel() is unable to fetch the correct pixel mask from the bitshift_left array in PROGMEM causing junk to be drawn

The bitshift_left[] array has now been optimised out and drawPixel() will function properly. The optimisation uses the same number of cycles and saves 6 bytes

drawPixel() has also been made static so the Arduboy object pointer is no longer needlesly passed on to drawPixel() saving more bytes

DrawPixel has

The new compiler In Arduino IDE 1.8.6/1.8.7 has an optimization glitch where it fails to see the difference between a data structure in RAM and in PROGMEM when the data is the same and optimizes one out. 

drawPixel() is unable to fetch the correct pixel mask from the bitshift_left array in PROGMEM causing junk to be drawn

The bitshift_left[] array has now been optimised out and drawPixel() will function properly. The optimisation uses the same number of cycles and saves 6 bytes

drawPixel() has also been made static so the Arduboy object pointer is no longer needlesly passed on to drawPixel()  saving more bytes

DrawPixel has
@MLXXXp
Copy link
Owner

MLXXXp commented Nov 9, 2018

@MrBlinky,

  • You've removed the commented out C++ equivalent code. Please include comments containing a C++ equivalent (or at least C++ code that will perform the same from a "black box" point of view). This is so the function can be easily ported to a different architecture, and also to help people who don't know assembly to understand its operation.

  • I'd prefer if you just remove the old unused bitshift_left code, rather than commenting it out.

  • You've added a CLEAR_BUFFER parameter to the display() function call on line 34 with no explanation as to why. It doesn't seem to be related to the drawPixel problem. Please document the reason for this change and if it isn't related to drawPixel(), make it a separate PR or at least a separate commit.

@MrBlinky
Copy link
Contributor Author

MrBlinky commented Nov 9, 2018

  1. I removed the old C++ comments and replaced them newer at the assembly explainging what the parts do
// bit = 1 << (y & 7)

and

//row_offset = y / 8 * WIDTH + x;

Obviously not clear enough so I'll add some at the right side

  1. I left the bitshift_left code because it is referenced in BitStreamReader (although remarked)

Will remove both comments

  1. That one slipped in unintentionally.

Will remove

@MrBlinky MrBlinky closed this Nov 9, 2018
@MrBlinky
Copy link
Contributor Author

MrBlinky commented Nov 9, 2018

submitted change PR #38

@MLXXXp
Copy link
Owner

MLXXXp commented Nov 9, 2018

Obviously not clear enough so I'll add some at the right side

Sorry about being picky about this and not making my requests clearer.

The new comments on the right of the assembly are helpful, and can be left there, but I'd also like the C++ code as a commented out standalone working function following the "live" source code.

This way, for someone wanting to quickly port to a different architecture without using assembly, the existing "live" code can be removed or commented out, then the commented out C++ code can just be uncommented to replace it. The person doing the port doesn't have to figure how to remove the assembly code, and what comments to keep or uncomment and what to discard.

In other words, write a working C++ only equivalent. Then, comment it out and have it follow the "live" assembly/mixed version of the function.

You can look at paintScreen() for an example of this. The C++ version is commented out using a #if 0 block to make it easy to uncomment.


Also, (sorry, I should have looked more closely at your commit message), I try to follow the Tim Pope suggested 50/72 format for Git commit messages

  • Maximum 50 character first "title" line, written as a capitalised imperative sentence.
  • If necessary, more detailed explanatory paragraphs following a blank line, with lines wrapped at maximum 72 characters.
  • If used, bullet points have a "hanging indent".

E.g. you could use
Fix a compiler issue and optimise drawPixel()
as the title line.
The following paragraphs just need to be wrapped at <72.

@MrBlinky
Copy link
Contributor Author

The original code didn't have such a block but I guess you could just append

#if 0 // C++ version of above assembly code
  bit = 1 << (y & 7);
  row_offset = (y & 0xF8) * WIDTH / 8 + x;
#endif

But you could also use something as below then you don't need to comment and change those if directives.

#ifdef ARDUINO_ARCH_AVR
//assembly optimized code
#else 
//C++ version of above assembly
#endif 

I'll try to keep suggested git message format in mind.

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