ThingPulse / esp8266-oled-ssd1306

Driver for the SSD1306 and SH1106 based 128x64, 128x32, 64x48 pixel OLED display running on ESP8266/ESP32
https://thingpulse.com
Other
2.01k stars 638 forks source link

Support Mega2560 board (and other 8-bit boards) #404

Closed ClutchplateDude closed 4 months ago

ClutchplateDude commented 6 months ago

Change frequency to long

Resolves #403

ClutchplateDude commented 4 months ago

What do you suggest? Change them all? We compile with warnings as errors so it was a blocker for us which is why we have forked it for now. But would like to use the official repo when possible.

marcelstoer commented 4 months ago

What do you suggest?

Not sure...not being too familiar with the early history of this lib makes this a difficult question for me. As changing the constructor parameters is the less invasive of the two options, I guess I would prefer that.

Also, while analyzing I realized the _ prefix wasn't applied consistently anyway. The geometry parameter (OLEDDISPLAY_GEOMETRY) is always just called g rather than _g.

ClutchplateDude commented 4 months ago

Yeah, there's often a tradeoff of readability/maintainability vs. backwards compatibility, or not making breaking changes. But I think internals should be open to change, since they should not be used by consumers. As far as coding standards go, in my C++ experience, using underscores is usually reserved for class members and function parameters get normal arguments. But that's like arguing about spaces vs. tabs.... you'll always find at least one of each side in any group :-)

ClutchplateDude commented 4 months ago

Looking at all the source, It's pretty inconsistent:

I would convert all ctors in the SSD* and SH* files to use camelCased arguments. I can do that in this PR if desired.

marcelstoer commented 4 months ago

I can do that in this PR if desired.

That'd be much appreciated, thanks.