Conversation
NickeZ
force-pushed
the
maint/oled-double-buf
branch
3 times, most recently
from
a4a1968 to
8bdd80f
Compare
benma
left a comment
benma
left a comment
There was a problem hiding this comment.
Slightly uncomfortable with the removal of the screen clearing before rendering/flushing, as it's a bit hard to reason about if the working buffer is really clean (from a previous commit) in all these cases.
src/ui/canvas.c
Outdated
|
|
||
| void canvas_commit(void) | ||
| { | ||
| _canvas_active = (_canvas_active + 1) % 2; |
There was a problem hiding this comment.
DeepSeek
Potential Race Conditions:
- The volatile
_canvas_activeswitch isn't atomic. On 32-bit MCUs, this could cause torn buffer states if interrupted during update. Consider using atomic operations or disabling interrupts briefly duringcanvas_commit().
There was a problem hiding this comment.
deepseek can fight gemini
There was a problem hiding this comment.
Well, you are not just writing the value, you are reading it too.
| /* | ||
| * Initialize canvases | ||
| */ | ||
| void canvas_init(void); |
There was a problem hiding this comment.
DeepSeek:
Buffer Initialization:
canvas_init()is declared in canvas.h but not implemented.
src/ui/canvas.c
Outdated
| @@ -0,0 +1,47 @@ | |||
| // Copyright 2025 Shift Cryptosecurity AG | |||
src/ui/canvas.c
Outdated
| #include <string.h> | ||
| #include <ui/canvas.h> | ||
|
|
||
| // This is sometimes updated from interrupts :( to be undone eventually |
There was a problem hiding this comment.
If you know what "sometimes" means exactly, please specify for clarity. BIP39 unlock? More?
"to be undone" you mean the volatile qualifier right?
There was a problem hiding this comment.
yup, volatile can be removed when we simplify all interrupts to just set flags.
src/ui/canvas.h
Outdated
| @@ -0,0 +1,58 @@ | |||
| // Copyright 2025 Shift Cryptosecurity AG | |||
test/hardware-fakes/src/fake_oled.c
Outdated
| @@ -0,0 +1,15 @@ | |||
| // Copyright 2025 Shift Cryptosecurity AG | |||
I guess I could return a dirty buffer and keep all the clears instead. I have no strong opinion, just thought it would be nicer like this. (if you didn't commit/switch the buffers the current buffer will never be drawn, so I guess it would be hard to miss?) |
NickeZ
force-pushed
the
maint/oled-double-buf
branch
6 times, most recently
from
1a6936f to
edfe7fe
Compare
NickeZ
force-pushed
the
maint/oled-double-buf
branch
from
edfe7fe to
5f7bb4f
Compare
NickeZ
force-pushed
the
maint/oled-double-buf
branch
12 times, most recently
from
515364f to
6d1fe77
Compare
Create a new module called "canvas" which is responsible for double buffering. Double buffering is required to enable asynchronous transfer of the frame buffer. While the "active" frame buffer is being transferred to the oled in the background, the ui will render to a "working" frame buffer. When the rendering is complete the buffers are flipped with "canvas_commit".
NickeZ
force-pushed
the
maint/oled-double-buf
branch
from
6d1fe77 to
a37dc14
Compare
2 participants
Create a new module called "canvas" which is responsible for double buffering. Double buffering is required to enable asynchronous transfer of the frame buffer. While the "active" frame buffer is being transferred to the oled in the background, the ui will render to a "working" frame buffer. When the rendering is complete the buffers are flipped with "canvas_commit".
I tested with both ssd1312 and sh1107 screens.