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

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

Merged
merged 5 commits into from
May 24, 2024

Conversation

ClutchplateDude
Copy link
Contributor

Change frequency to long

Resolves #403

Change frequency to long
@ClutchplateDude ClutchplateDude changed the title Update SH1106Wire.h Support Mega2560 board (and other 8-bit boards) Apr 5, 2024
Copy link
Member

@marcelstoer marcelstoer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am personally not a fan of these _ variable/parameter name prefixes either. I understand the compiler warnings but for consistency reasons one cannot change this only here.

@ClutchplateDude
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

  • all the hardware-related files (SSD1306* and SH1106*) use the _ prefix for members. Function arguments do not have a leading _ except for ctor args
  • all the OLED* files use camelCased member and argument names for all functions and ctors

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
Copy link
Member

I can do that in this PR if desired.

That'd be much appreciated, thanks.

@marcelstoer marcelstoer merged commit 469f227 into ThingPulse:master May 24, 2024
6 checks passed
marcelstoer added a commit that referenced this pull request May 25, 2024
@ClutchplateDude ClutchplateDude deleted the patch-1 branch May 25, 2024 16:31
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.

Small tweaks allows running on ATMega2560
2 participants