Add support for SW blink #2452
Add support for SW blink #2452
Conversation
Not used anymore now what we support text attributes rather
Useful for chips which can't handle HW blink correctly or future drivers that don't support HW blink at all. Added a new setting named display_force_sw_blink which forces drivers which support HW blink to use SW blink. For now the SW blink cycle is 1sec on/1sec off to make it more evident while testing.
src/main/drivers/display.c
Outdated
| if (attr & ~instance->supportedTextAttributes) { | ||
| // We can't overwrite s, so we write char by char | ||
| int ret; | ||
| for (unsigned ii = 0; ii < strlen(s); ii++) { |
There was a problem hiding this comment.
This is suboptimal. strlen isn't declared pure or const, so gcc will evaluate it each loop iteration.
There was a problem hiding this comment.
Wel, not necesarily. If strlen is declared pure gcc might pre-compute it since s is const. But I wouldn't count on that.
src/main/drivers/display.c
Outdated
| int displayWriteWithAttr(displayPort_t *instance, uint8_t x, uint8_t y, const char *s, textAttributes_t attr) | ||
| { | ||
| if (attr & ~instance->supportedTextAttributes) { | ||
| // We can't overwrite s, so we write char by char |
There was a problem hiding this comment.
Can't we reserve a buffer large enough and process the string in chunks? We fill the buffer, once it's filled - we flush it and continue with the rest of s.
src/main/io/displayport_max7456.c
Outdated
| textAttributes_t attr = TEXT_ATTRIBUTES_NONE; | ||
| TEXT_ATTRIBUTES_ADD_INVERTED(attr); | ||
| TEXT_ATTRIBUTES_ADD_SOLID_BG(attr); | ||
| if (!displayConfig()->force_sw_blink) { |
There was a problem hiding this comment.
In general drivers shouldn't use PGs. We need to pass the required configuration to max7456Init via vcdProfile_t.
This way we don't need to check for force_sw_blink in every driver, the display system takes care of it.
This avoids calling into displayWriteCharWithAttr() for each character. Note that support for arbitrary length string is disabled behind a compile time option because it takes ~120 bytes of flash to support it and right now the biggest screen width we support is 30 characters. Instead, we use a buffer which can hold up to 30 characters to perform any required rewrites.
| .resync = oledResync, | ||
| .txBytesFree = oledTxBytesFree | ||
| .txBytesFree = oledTxBytesFree, | ||
| .supportedTextAttributes = NULL, |
There was a problem hiding this comment.
Why NULL? This is not a pointer.
There was a problem hiding this comment.
It is, the display driver vtable contains a supportedTextAttributes function pointer but the display caches it in a field. We can make the drivers change the field themselves before calling displayInit() instead, but it will be a bit less flexible. It saves 16 bytes of flash.
There was a problem hiding this comment.
Oh, ok, the cached value field name is the same which is confusing. How about refactoring function pointers in the vtable to have a Fn suffix?
There was a problem hiding this comment.
I think that's a bit too much for this PR. What about renaming the field to cachedSupportedTextAttributes?
src/main/drivers/display.c
Outdated
| if (displayEmulateTextAttributes(instance, buf, s, blockSize, attr, &blockAttr)) { | ||
| // Emulation requires rewriting the string, which might be bigger than our buffer. | ||
| int ret = instance->vTable->writeString(instance, x, y, buf, blockAttr); | ||
| #ifdef DISPLAY_ATTRIBUTE_EMULATION_MULTIPLE_WRITES |
There was a problem hiding this comment.
I'm not sure that DISPLAY_ATTRIBUTE_EMULATION_MULTIPLE_WRITES will be ever used.
There was a problem hiding this comment.
We can delete that, I guess we can always recover it from the history if we ever need it.
There was a problem hiding this comment.
By deleting it we can also simplify displayEmulateTextAttributes() since it can now just overwrite the received attributes rather than copying them, saving 32 bytes of flash. Let's go for it.
Saves 32 bytes of flash, since we can simplify displayEmulateTextAttributes().
supportedTextAttributes -> cachedSupportedTextAttributes Cosmetic change. Having the same name for the caching field than the function pointer in the vtable could be confusing.
Allows displays which don't support blinking or with faulty blink implementations (like some MAX7456) to. Each display drivers declares the capabilities it supports and the display system handles blinking for the drivers that don't support it.
To workaround HW issues in MAX7456, a new setting has been introduced which makes the MAX7456 driver report its capabilities without support for blinking, so SW blink takes over.
Fixes #2419