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

OLED Driver Feature #5288

Merged
merged 4 commits into from
Apr 20, 2019
Merged

OLED Driver Feature #5288

merged 4 commits into from
Apr 20, 2019

Conversation

XScorpion2
Copy link
Contributor

@XScorpion2 XScorpion2 commented Mar 2, 2019

Standalone OLED Driver based on PR #5151 but without the conversion of every keyboard. This is an opt in feature for keyboard owners to adapt to at their leisure, with the Sol being converted as an example (plus it's what I use myself, so easy to test daily)

Description

This is a standalone OLED Driver for ssd1396 based OLED Displays. The goals of this driver were to provide support on both AVR and ARM boards with faster performance and smaller size than what existed today as well as fix issues that exist in the current ssd1306 driver and its i2c usage causing lost data & deadlocks. In addition, this driver supports features the current options do not such as scrolling and 90 degree rotation.

By the numbers:
make sol/rev1:brianweyer

type ssd1306 oled driver difference
firmware size 26828 bytes 25050 bytes smaller by 1778 bytes
render time 15 ms 3 ms faster by 12 ms

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@and3rson
Copy link
Contributor

This would be absolutely awesome.

@XScorpion2 XScorpion2 force-pushed the features/oled_driver branch from 495e55c to c40fc73 Compare March 23, 2019 21:45
@XScorpion2
Copy link
Contributor Author

Force push rebase on latest master

@XScorpion2 XScorpion2 mentioned this pull request Mar 27, 2019
@drashna
Copy link
Member

drashna commented Mar 29, 2019

It looks like #4994 may compete with this, actually. And that PR may be a better implementation.

@XScorpion2 if you wanted to take a look at that PR? Because, personally, I'd like to see one unified implementation.

@XScorpion2
Copy link
Contributor Author

XScorpion2 commented Mar 30, 2019

@drashna I've looked at that pr a few times before.
It updates the internals of drivers/ssd1306 to use i2c_master instead of i2c.
This fixes the send data error that would occur with the old i2c, and gets the driver using the common i2c_master.

However, it still requires each keyboard to switch their implementation over as most keyboards pulled in ssd1306 and i2c files into their own directory and made changes to them. Which is the same catch with this PR. Though converting existing keyboards to that driver would be faster as it behaves identically to today.

Comparing API and features of the original ssd1306 driver vs this one, this driver requires less code for a user (or keyboard) to write for integration, it's faster, smaller firmware footprint, and has additional features: 90/270 degree rotation support, and scrolling.

@drashna
Copy link
Member

drashna commented Mar 30, 2019

Mostly my comment was a crosspost, and to make sure both of you have had eyes on the other PR.

@XScorpion2 XScorpion2 mentioned this pull request Mar 31, 2019
13 tasks
docs/feature_oled_driver.md Outdated Show resolved Hide resolved
keyboards/sol/keymaps/danielhklein/keymap.c Outdated Show resolved Hide resolved
quantum/quantum.c Outdated Show resolved Hide resolved
@drashna
Copy link
Member

drashna commented Apr 6, 2019

So I decided to test this out ....

~~The logo is stuck on the SOL one. I'm using the CRKBD logo file, and the font file... but .... ~~

Edit: Fixed. Was using the wrong file.

@Lenbok
Copy link
Contributor

Lenbok commented Apr 6, 2019

Working fine for me on crkbd, corne logo showing happily, my changes (cribbed from @r2d2rogers) are in Lenbok@a118a6d

@drashna
Copy link
Member

drashna commented Apr 14, 2019

Well, to be honest, as much as I like optimizing the firmware, I think this is one of those points where functionality is more important.

Would it be possible to have defines to set the rotation, but also have it available as a function/call/check/whatever-the-right-term-is? If so, I think that may be the "best" approach.

@drashna
Copy link
Member

drashna commented Apr 15, 2019

There are conflicts now.

@XScorpion2
Copy link
Contributor Author

@drashna conflicts? where? =P

@XScorpion2 XScorpion2 mentioned this pull request Apr 15, 2019
13 tasks
@XScorpion2
Copy link
Contributor Author

XScorpion2 commented Apr 16, 2019

Well, to be honest, as much as I like optimizing the firmware, I think this is one of those points where functionality is more important.

Would it be possible to have defines to set the rotation, but also have it available as a function/call/check/whatever-the-right-term-is? If so, I think that may be the "best" approach.

@drashna Firmware size increases by ~230 bytes (same with lto on or off) with the define enabled. Quick test converting that to a runtime switch (init param) would be about ~330 bytes. None of the necessary additional rotation checks are in any hot loops so perf hit would be minimal.

Edit: Final results are in, 354 bytes larger for moving 90 degree rotation support to a runtime switch.

@XScorpion2 XScorpion2 force-pushed the features/oled_driver branch from 439776d to a615e3c Compare April 16, 2019 03:33
@XScorpion2
Copy link
Contributor Author

Rebased & Compacted commits onto latest master

docs/feature_oled_driver.md Outdated Show resolved Hide resolved
docs/feature_oled_driver.md Outdated Show resolved Hide resolved
drivers/oled/oled_driver.h Outdated Show resolved Hide resolved
@drashna
Copy link
Member

drashna commented Apr 17, 2019

Also, passes Drashna CI. Been using it for a while. It works very well.

@@ -20,8 +20,6 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.

#pragma once

#define SSD1306OLED
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the user keymap changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because they wanted me update it. I'll get them in here to approve.

@XScorpion2 XScorpion2 force-pushed the features/oled_driver branch from c3fafe1 to 4064a09 Compare April 19, 2019 21:47
@XScorpion2
Copy link
Contributor Author

Image of QMK Logo

@Legonut
Copy link
Contributor

Legonut commented Apr 20, 2019

It's got my blessing, good work @XScorpion2

@noroadsleft
Copy link
Member

🍌 🚪

😆

Copy link

@brianweyer brianweyer left a comment

Choose a reason for hiding this comment

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

My keymap looks good. Thanks for the hard work on this one @XScorpion2!

@XScorpion2
Copy link
Contributor Author

So that's approval from keyboard developer, keymap owners, and removal of potential copyright issues. Anything else or is this good to go?

@skullydazed skullydazed merged commit d326828 into qmk:master Apr 20, 2019
@skullydazed
Copy link
Member

Build failures were unrelated.

Thanks!

@XScorpion2 XScorpion2 deleted the features/oled_driver branch April 20, 2019 15:25
@burlapsax burlapsax mentioned this pull request Aug 31, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants