Skip to content

Conversation

@lutorm
Copy link
Contributor

@lutorm lutorm commented Apr 6, 2021

This is kind of a large change, but it saves 1100 bytes of program space when only using SPI. which is significant. I assume the savings are similar if you use the other comms protocols.

Because the library used a runtime variable to determine which comm method to use, the compiler could not easily eliminate code that we know will not be used since the constructor sets the comms mode.

If we instead of using a runtime variable keep this information in a template, it is obvious to the compiler at compile time which methods won't be used and it can eliminate that code. The MicroOLED class is now three different classes for the SPI, I2C, and Parallel communication modes, with the functionality in a templatized common base class. This avoids always building in the I2C, SPI, and Parallel comm code even though only one will likely be used.

The drawback is that cases that DO use more than one comms mode will now use more space. If this is a use case that people care about, it could be accommodated most simply by adding another derived class.

The MicroOLED class is now three different classes for the SPI, I2C, and
Parallel communication modes, with the functionality in a templatized
common base class. This avoids always building in the I2C, SPI, and
Parallel comm code even though only one will likely be used.

When using SPI mode, this resulted in a hex file using 1092 bytes less,
not insignificant.
@PaulZC
Copy link
Contributor

PaulZC commented Apr 6, 2021

Nice work Patrik (@lutorm ) - thank you,
But, unless I'm mistaken, this breaks backward-compatibility? As a minimum, all users would need to change the name of the class to match their chosen interface? If so, then I'm afraid that is a step too far for a library as old as this. Especially with processor memory increasing the way it is. You would not believe the complaints we get when we break compatibility, users simply expect their existing code to keep working.
Unless you can come up with a way to let existing code keep running, then I'm unable to merge. You are of course welcome to maintain your own fork and we can point users looking for a truly minimal memory solution your way.
Best wishes,
Paul

@lutorm
Copy link
Contributor Author

lutorm commented Apr 6, 2021

Hmm yeah, good point, it does break backwards-compatibility although in a pretty trivial way. I think it would be pretty easy to add another class with the old name which preserves the old runtime-choice of comms, though. Let me update.

@lutorm
Copy link
Contributor Author

lutorm commented Apr 8, 2021

It got pretty convoluted with having to share the comm implementations between the "new" and a backwards compatible class, so I guess it's not worth it.

@PaulZC
Copy link
Contributor

PaulZC commented Apr 8, 2021

Thanks Patrik,
Feel free to maintain your own fork and we can point users looking for a truly minimal memory solution your way.
Best wishes,
Paul

@PaulZC PaulZC closed this Apr 8, 2021
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