OLED display update interval support

[mtei]

Description

This PR adds support for OLED_UPDATE_INTERVAL_MS to oled_driver.c.

Use of the OLED driver has degraded matrix scan performance.
The matrix scan rate can be improved by changing the update interval of the OLED display to the length specified by OLED_UPDATE_INTERVAL.

Example.

• OLED_DRIVER_ENABLE = no

*** yushakobo - Helix rev3 5rows connected -- 0x3265:0x3
  > matrix scan frequency: 929
  > matrix scan frequency: 1071
  > matrix scan frequency: 1073
  > matrix scan frequency: 1074

• OLED_DRIVER_ENABLE = yes

  *** yushakobo - Helix rev3 5rows connected -- 0x3265:0x3
    > matrix scan frequency: 220
    > matrix scan frequency: 301
    > matrix scan frequency: 299
    > matrix scan frequency: 299
  

  • #define OLED_UPDATE_INTERVAL 30

    *** yushakobo - Helix rev3 5rows connected -- 0x3265:0x3
      > matrix scan frequency: 765
      > matrix scan frequency: 989
      > matrix scan frequency: 992
      > matrix scan frequency: 989
    

  • #define OLED_UPDATE_INTERVAL 50

    *** yushakobo - Helix rev3 5rows connected -- 0x3265:0x3
      > matrix scan frequency: 894
      > matrix scan frequency: 1018
      > matrix scan frequency: 1022
      > matrix scan frequency: 1024
    

Tag: @XScorpion2

Types of Changes

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

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• 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).

[XScorpion2]

Hmm, it's not a bad idea, but I'm wondering if there isn't more to do with the oled driver usage in this keyboard case. IE: is the keyboard wiping out the oled display and rebuilding it every loop?

[fauxpark]

Yeah, the dirty flag should theoretically be preventing most pointless writes to the display.

[mtei]

The above test results were obtained using the following code.

https://github.com/yushakobo/qmk_firmware/blob/0383af9cc881881c9c5bce8c98a0ffee0ec463a7/keyboards/helix/rev3_5rows/oled_display.c

I think the above code is practically the same as the sample code in the OLED driver documentation.

#ifdef OLED_DRIVER_ENABLE
void oled_task_user(void) {
    // Host Keyboard Layer Status
    oled_write_P(PSTR("Layer: "), false);

    switch (get_highest_layer(layer_state)) {
        case _QWERTY:
            oled_write_P(PSTR("Default\n"), false);
            break;
        case _FN:
            oled_write_P(PSTR("FN\n"), false);
            break;
        case _ADJ:
            oled_write_P(PSTR("ADJ\n"), false);
            break;
        default:
            // Or use the write_ln shortcut over adding '\n' to the end of your string
            oled_write_ln_P(PSTR("Undefined"), false);
    }

    // Host Keyboard LED Status
    led_t led_state = host_keyboard_led_state();
    oled_write_P(led_state.num_lock ? PSTR("NUM ") : PSTR("    "), false);
    oled_write_P(led_state.caps_lock ? PSTR("CAP ") : PSTR("    "), false);
    oled_write_P(led_state.scroll_lock ? PSTR("SCR ") : PSTR("    "), false);
}
#endif

[XScorpion2]

Ok, so the slowdown is caused by just working with the oled buffer and not flushing to the screen. Hmm, I didn't think that was that expensive to cause nearly a 4x drop in scan rate. Thanks for the additional details. Approved.

[zvecr]

Also, while this wont break anything, https://docs.qmk.fm/#/pr_checklist?id=core-prs must now target develop branch. So the question becomes, "Is this PR exempt from that statement".

[mtei]

Also, while this wont break anything, https://docs.qmk.fm/#/pr_checklist?id=core-prs must now target develop branch. So the question becomes, "Is this PR exempt from that statement".

I wasn't sure if I should set the merge destination to master or develop. I'm still on the fence.

Personally, I'm fine with merging this into master.

I'd like to hear which one you other collaborators choose.

[noroadsleft]

My personal opinion on the master vs. develop for core PRs is currently:

PRs that enhance functionality (add features, extend capability) are likely safe to go into master, as they ideally aren't changing any existing behaviour. These changes, in every scenario I can think of, are invisible to the users, so there's little to no risk.

PRs that modify existing behaviour should go to develop.

[tzarc]

I'd like to hear which one you other collaborators choose.

Whilst I agree that this is a non-behavioural change and is unlikely to cause any issues on master, I'd prefer to have a consistently simple way to make the decisions as to whether a PR goes to master or develop -- as described it's "core goes to develop, bugfixes can go to master".

In my mind consistency is key here, as going forward we'd have to make judgement calls about suitability, attempt to infer whether it affects any existing builds, and likely have to justify to non-collaborators as to why they're not allowed to get things into master in their "super important" case, but others can. Again, I'd prefer consistency, so I'd recommend develop.

[drashna]

I would definitely prefer that this target develop, first.

[mtei]

OK, I switched the merge destination to 'develop'.