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 fixes #10377

Merged
merged 6 commits into from
Oct 3, 2020
Merged

OLED driver fixes #10377

merged 6 commits into from
Oct 3, 2020

Conversation

sigprof
Copy link
Contributor

@sigprof sigprof commented Sep 20, 2020

Description

Fix several problems in the OLED driver:

  1. oled_write_pixel() was unconditionally setting the dirty bit for the touched block, even if the pixel value was not actually changed. This made oled_write_pixel() unusable inside oled_task_user() if the implementation of oled_task_user() was redrawing the whole image unconditionally.
  2. oled_write_pixel() did not handle the 90° or 270° rotation properly.
  3. Attempting to override OLED_BLOCK_COUNT with a custom value in config.h resulted in a compile error, even though the documentation listed OLED_BLOCK_COUNT as one of the configurable values.
  4. Using a custom OLED_BLOCK_COUNT could result in out-of-range accesses inside oled_render(), because in this case some bits in oled_dirty may not correspond to existing blocks, but some code could set those bits.
  5. oled_write_char() could dirty more blocks than needed, because it tried to dirty the block corresponding to the location just beyond the last column of the character, instead of that last column.
  6. #define OLED_BLOCK_TYPE uint32_t did not work properly on AVR, because some bit shifts were performed on int values, which are 16 bit on AVR.

Fixes are in separate commits, but they touch the same code, therefore all of them are in a single PR.

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).

Set the dirty bit for the block only if oled_write_pixel() actually
changed the buffer state.  Without this check oled_write_pixel() could
not be used inside the oled_task_user() code using the “redraw always”
style, because the blocks touched by oled_write_pixel() would always
appear dirty, and oled_render() would not proceed beyond the first such
dirty block.
Use oled_rotation_width instead of OLED_DISPLAY_WIDTH, so that a rotated
display would be handled correctly.
Some OLED sizes (e.g., 64×48) may require a nonstandard value of
OLED_BLOCK_COUNT.  The documentation says that this value may be
redefined in config.h, but actually trying to redefine it caused a
compile error, because the macro was redefined in oled_driver.c.
Make the OLED_BLOCK_COUNT definition in oled_driver.c respect any
user override, and do the same for OLED_BLOCK_SIZE just in case.
If a custom OLED_BLOCK_COUNT value is specified, some bits in oled_dirty
may not correspond to existing blocks; however, if those bits are set
somewhere (e.g., by code with sets oled_dirty to ~0 or even -1),
oled_render() would try to handle them and could access memory beyond
oled_buffer and perform hardware operations with out of range values.
Prevent this by masking off unused bits in oled_render(), and also avoid
setting those bits in other functions.
oled_write_char() tried to mark the position just beyond the written
character as dirty; use (OLED_FONT_WIDTH - 1) to dirty the last position
still belonging to the character instead.
Using uint32_t as OLED_BLOCK_TYPE did not work properly on AVR, because
some bit shifts were performed using 16-bit int.  Add explicit casts to
OLED_BLOCK_TYPE to those shifts.
Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

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

Can confirm, at least, that it doesn't appear to break anything on the AVR side of things.

@drashna drashna requested a review from a team September 21, 2020 04:12
@tzarc tzarc merged commit 459ccb6 into qmk:master Oct 3, 2020
Shinichi-Ohki added a commit to Shinichi-Ohki/qmk_firmware that referenced this pull request Oct 7, 2020
* 'master' of https://github.com/qmk/qmk_firmware: (178 commits)
  Docs: fix udev rules
  [CLI] Add c2json (qmk#8817)
  Improve LAYOUT macro searching (qmk#9530)
  CLI: update subcommands to use return instead of exit() (qmk#10323)
  Fixes small typo in docs (qmk#10515)
  Update personal keymap for Let's Split keyboard. (qmk#10536)
  [Keymap] Move my custom functions and keymaps to userspace (qmk#10502)
  [Keyboard] add support for ymd75 rev3 (qmk#10483)
  [Keyboard] Add soy20 PCB (qmk#10440)
  Fix for MIDI sustain effect issue (qmk#10361)
  format code according to conventions [skip ci]
  [Keyboard] Add Yugo-M Controller (qmk#10389)
  [Keymap] Add onekey keymap for OLED testing (qmk#10380)
  [Keymap] Add winterNebs keymaps  (qmk#10328)
  [Keymap] Added 333fred 5x6_5 keymap (qmk#10272)
  [Keyboard] Add hannah60rgb rev.2 PCB (qmk#10287)
  Adding VIA support to katana60 rev2 (qmk#10442)
  OLED driver fixes (qmk#10377)
  IS31FL3741 driver fixup (qmk#10519)
  add info.json for XD75 keyboard (qmk#10523)
  ...
kjganz pushed a commit to kjganz/qmk_firmware that referenced this pull request Oct 28, 2020
* Fix dirtying in oled_write_pixel()

Set the dirty bit for the block only if oled_write_pixel() actually
changed the buffer state.  Without this check oled_write_pixel() could
not be used inside the oled_task_user() code using the “redraw always”
style, because the blocks touched by oled_write_pixel() would always
appear dirty, and oled_render() would not proceed beyond the first such
dirty block.

* Fix oled_write_pixel() with 90/270 degree rotation

Use oled_rotation_width instead of OLED_DISPLAY_WIDTH, so that a rotated
display would be handled correctly.

* Fix compilation with custom OLED_BLOCK_COUNT and OLED_BLOCK_SIZE

Some OLED sizes (e.g., 64×48) may require a nonstandard value of
OLED_BLOCK_COUNT.  The documentation says that this value may be
redefined in config.h, but actually trying to redefine it caused a
compile error, because the macro was redefined in oled_driver.c.
Make the OLED_BLOCK_COUNT definition in oled_driver.c respect any
user override, and do the same for OLED_BLOCK_SIZE just in case.

* Fix handling of out-of-range bits in oled_dirty

If a custom OLED_BLOCK_COUNT value is specified, some bits in oled_dirty
may not correspond to existing blocks; however, if those bits are set
somewhere (e.g., by code with sets oled_dirty to ~0 or even -1),
oled_render() would try to handle them and could access memory beyond
oled_buffer and perform hardware operations with out of range values.
Prevent this by masking off unused bits in oled_render(), and also avoid
setting those bits in other functions.

* Fix potentially wrong dirtying in oled_write_char()

oled_write_char() tried to mark the position just beyond the written
character as dirty; use (OLED_FONT_WIDTH - 1) to dirty the last position
still belonging to the character instead.

* Fix `#define OLED_BLOCK_TYPE uint32_t` on AVR

Using uint32_t as OLED_BLOCK_TYPE did not work properly on AVR, because
some bit shifts were performed using 16-bit int.  Add explicit casts to
OLED_BLOCK_TYPE to those shifts.
oscarcarlsson pushed a commit to oscarcarlsson/qmk_firmware that referenced this pull request Nov 2, 2020
* Fix dirtying in oled_write_pixel()

Set the dirty bit for the block only if oled_write_pixel() actually
changed the buffer state.  Without this check oled_write_pixel() could
not be used inside the oled_task_user() code using the “redraw always”
style, because the blocks touched by oled_write_pixel() would always
appear dirty, and oled_render() would not proceed beyond the first such
dirty block.

* Fix oled_write_pixel() with 90/270 degree rotation

Use oled_rotation_width instead of OLED_DISPLAY_WIDTH, so that a rotated
display would be handled correctly.

* Fix compilation with custom OLED_BLOCK_COUNT and OLED_BLOCK_SIZE

Some OLED sizes (e.g., 64×48) may require a nonstandard value of
OLED_BLOCK_COUNT.  The documentation says that this value may be
redefined in config.h, but actually trying to redefine it caused a
compile error, because the macro was redefined in oled_driver.c.
Make the OLED_BLOCK_COUNT definition in oled_driver.c respect any
user override, and do the same for OLED_BLOCK_SIZE just in case.

* Fix handling of out-of-range bits in oled_dirty

If a custom OLED_BLOCK_COUNT value is specified, some bits in oled_dirty
may not correspond to existing blocks; however, if those bits are set
somewhere (e.g., by code with sets oled_dirty to ~0 or even -1),
oled_render() would try to handle them and could access memory beyond
oled_buffer and perform hardware operations with out of range values.
Prevent this by masking off unused bits in oled_render(), and also avoid
setting those bits in other functions.

* Fix potentially wrong dirtying in oled_write_char()

oled_write_char() tried to mark the position just beyond the written
character as dirty; use (OLED_FONT_WIDTH - 1) to dirty the last position
still belonging to the character instead.

* Fix `#define OLED_BLOCK_TYPE uint32_t` on AVR

Using uint32_t as OLED_BLOCK_TYPE did not work properly on AVR, because
some bit shifts were performed using 16-bit int.  Add explicit casts to
OLED_BLOCK_TYPE to those shifts.
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Nov 24, 2020
* Fix dirtying in oled_write_pixel()

Set the dirty bit for the block only if oled_write_pixel() actually
changed the buffer state.  Without this check oled_write_pixel() could
not be used inside the oled_task_user() code using the “redraw always”
style, because the blocks touched by oled_write_pixel() would always
appear dirty, and oled_render() would not proceed beyond the first such
dirty block.

* Fix oled_write_pixel() with 90/270 degree rotation

Use oled_rotation_width instead of OLED_DISPLAY_WIDTH, so that a rotated
display would be handled correctly.

* Fix compilation with custom OLED_BLOCK_COUNT and OLED_BLOCK_SIZE

Some OLED sizes (e.g., 64×48) may require a nonstandard value of
OLED_BLOCK_COUNT.  The documentation says that this value may be
redefined in config.h, but actually trying to redefine it caused a
compile error, because the macro was redefined in oled_driver.c.
Make the OLED_BLOCK_COUNT definition in oled_driver.c respect any
user override, and do the same for OLED_BLOCK_SIZE just in case.

* Fix handling of out-of-range bits in oled_dirty

If a custom OLED_BLOCK_COUNT value is specified, some bits in oled_dirty
may not correspond to existing blocks; however, if those bits are set
somewhere (e.g., by code with sets oled_dirty to ~0 or even -1),
oled_render() would try to handle them and could access memory beyond
oled_buffer and perform hardware operations with out of range values.
Prevent this by masking off unused bits in oled_render(), and also avoid
setting those bits in other functions.

* Fix potentially wrong dirtying in oled_write_char()

oled_write_char() tried to mark the position just beyond the written
character as dirty; use (OLED_FONT_WIDTH - 1) to dirty the last position
still belonging to the character instead.

* Fix `#define OLED_BLOCK_TYPE uint32_t` on AVR

Using uint32_t as OLED_BLOCK_TYPE did not work properly on AVR, because
some bit shifts were performed using 16-bit int.  Add explicit casts to
OLED_BLOCK_TYPE to those shifts.
@sigprof sigprof deleted the oled-driver-fixes branch January 8, 2021 13:32
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Jan 13, 2021
* Fix dirtying in oled_write_pixel()

Set the dirty bit for the block only if oled_write_pixel() actually
changed the buffer state.  Without this check oled_write_pixel() could
not be used inside the oled_task_user() code using the “redraw always”
style, because the blocks touched by oled_write_pixel() would always
appear dirty, and oled_render() would not proceed beyond the first such
dirty block.

* Fix oled_write_pixel() with 90/270 degree rotation

Use oled_rotation_width instead of OLED_DISPLAY_WIDTH, so that a rotated
display would be handled correctly.

* Fix compilation with custom OLED_BLOCK_COUNT and OLED_BLOCK_SIZE

Some OLED sizes (e.g., 64×48) may require a nonstandard value of
OLED_BLOCK_COUNT.  The documentation says that this value may be
redefined in config.h, but actually trying to redefine it caused a
compile error, because the macro was redefined in oled_driver.c.
Make the OLED_BLOCK_COUNT definition in oled_driver.c respect any
user override, and do the same for OLED_BLOCK_SIZE just in case.

* Fix handling of out-of-range bits in oled_dirty

If a custom OLED_BLOCK_COUNT value is specified, some bits in oled_dirty
may not correspond to existing blocks; however, if those bits are set
somewhere (e.g., by code with sets oled_dirty to ~0 or even -1),
oled_render() would try to handle them and could access memory beyond
oled_buffer and perform hardware operations with out of range values.
Prevent this by masking off unused bits in oled_render(), and also avoid
setting those bits in other functions.

* Fix potentially wrong dirtying in oled_write_char()

oled_write_char() tried to mark the position just beyond the written
character as dirty; use (OLED_FONT_WIDTH - 1) to dirty the last position
still belonging to the character instead.

* Fix `#define OLED_BLOCK_TYPE uint32_t` on AVR

Using uint32_t as OLED_BLOCK_TYPE did not work properly on AVR, because
some bit shifts were performed using 16-bit int.  Add explicit casts to
OLED_BLOCK_TYPE to those shifts.
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* Fix dirtying in oled_write_pixel()

Set the dirty bit for the block only if oled_write_pixel() actually
changed the buffer state.  Without this check oled_write_pixel() could
not be used inside the oled_task_user() code using the “redraw always”
style, because the blocks touched by oled_write_pixel() would always
appear dirty, and oled_render() would not proceed beyond the first such
dirty block.

* Fix oled_write_pixel() with 90/270 degree rotation

Use oled_rotation_width instead of OLED_DISPLAY_WIDTH, so that a rotated
display would be handled correctly.

* Fix compilation with custom OLED_BLOCK_COUNT and OLED_BLOCK_SIZE

Some OLED sizes (e.g., 64×48) may require a nonstandard value of
OLED_BLOCK_COUNT.  The documentation says that this value may be
redefined in config.h, but actually trying to redefine it caused a
compile error, because the macro was redefined in oled_driver.c.
Make the OLED_BLOCK_COUNT definition in oled_driver.c respect any
user override, and do the same for OLED_BLOCK_SIZE just in case.

* Fix handling of out-of-range bits in oled_dirty

If a custom OLED_BLOCK_COUNT value is specified, some bits in oled_dirty
may not correspond to existing blocks; however, if those bits are set
somewhere (e.g., by code with sets oled_dirty to ~0 or even -1),
oled_render() would try to handle them and could access memory beyond
oled_buffer and perform hardware operations with out of range values.
Prevent this by masking off unused bits in oled_render(), and also avoid
setting those bits in other functions.

* Fix potentially wrong dirtying in oled_write_char()

oled_write_char() tried to mark the position just beyond the written
character as dirty; use (OLED_FONT_WIDTH - 1) to dirty the last position
still belonging to the character instead.

* Fix `#define OLED_BLOCK_TYPE uint32_t` on AVR

Using uint32_t as OLED_BLOCK_TYPE did not work properly on AVR, because
some bit shifts were performed using 16-bit int.  Add explicit casts to
OLED_BLOCK_TYPE to those shifts.
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.

None yet

3 participants