fix(display): fix nice!view display refresh

[snoyer]

#3294 introduced an optimization that may be a bit too aggressive for the nice!view display code.

This PR adjusts LS0XX_VCOM_THREAD_PRIO the UI thread priority so that the nice!view display refreshes properly.

This fix has been confirmed by users in #3392, and by me (see ZMK's Discord). The issue is also mentioned on Typeractive's Discord here, and possibly here (however these users haven't reported back after the config change has been suggested).

This fix works in my testing and my be a temporary solution, but more investigation is needed, see comments below

fixes #3392

@Nicell please have a look and see if you can "officially" reproduce and confirm the fix.

PR check-list

• Branch has a clean commit history
• Additional tests are included, if changing behaviors/core code that is testable.
• Proper Copyright + License headers added to applicable files (Generally, we stick to "The ZMK Contributors" for copyrights to help avoid churn when files get edited)
• Pre-commit used to check formatting of files, commit messages, etc.
• Includes any necessary documentation changes.

[joelspadin]

Based on the comments on the PR that introduced this change, this was done intentionally to prevent display updates from introducing keyboard latency.

I think it would be better to understand why running the display thread at a lower priority causes this issue. That might lead us to a solution that fixes the display refreshes without slowing down keyboard functions, if such a solution exists.

This sounds like it could be related to the comment at #3294 (comment)

[snoyer]

I think it would be better to understand why running the display thread at a lower priority causes this issue. That might lead us to a solution that fixes the display refreshes without slowing down keyboard functions, if such a solution exists.

Alright, shot in the dark then: ZMK_DISPLAY_DEDICATED_THREAD_PRIORITY is higher priority than LS0XX_VCOM_THREAD_PRIO (5 vs 11)

zmk/app/src/display/Kconfig

Lines 69 to 71 in 64daf69

	config ZMK_DISPLAY_DEDICATED_THREAD_PRIORITY
	int "Thread priority for dedicated UI thread/queue"
	default 5

So maaaaaybe we're sending screen updates faster that the display can refresh and that's the problem? In which case we could try drawing slower by lowering the UI thread's priority instead of refreshing faster by increasing the refresh thread priority 🤞

[snoyer]

After very limited testing it looks like CONFIG_ZMK_DISPLAY_DEDICATED_THREAD_PRIORITY=11 does do the trick. That would support my theory wild guess that the corruption is caused by the refresh thread being interrupted, and that's less likely to happen if the UI thread has lower priority.

But then if that assumption is correct (is it? how do we confirm?) maybe the actual fix would be in the ls0xx code? Would it need a scheduler lock when performing whatever goes on in the VCOM thread so that's not interrupted halfway through regardless of priority?

@galaxyeden do you have any insights?

[snoyer]

Holding on to the semaphore for one more line in the VCOM thread is also a candidate fix (on a "works on my machine" basis FWIW)

diff --git a/drivers/display/ls0xx.c b/drivers/display/ls0xx.c
index 109f13eeba..0be1ceb598 100644
--- a/drivers/display/ls0xx.c
+++ b/drivers/display/ls0xx.c
@@ -147,10 +147,10 @@ static void ls0xx_vcom_toggle(void *a, void *b, void *c)
 		} else {
 			LOG_ERR("memory display semaphore not available - cmd");
 		}
-		/* Sleep before giving semaphore based on errors in testing */
+		/* Sleep and release before giving semaphore based on errors in testing */
 		k_sleep(K_TICKS(LS0XX_BUS_RETURN_DELAY_TICKS));
-	    k_sem_give(&ls0xx_bus_sem);
 		spi_release_dt(&config->bus);
+		k_sem_give(&ls0xx_bus_sem);
 		k_msleep(config->serial_vcom_int);
 #endif /* DT_INST_NODE_HAS_PROP(0, extcomin_gpios) */
 	}

However I'm not qualified to work that close to the metal and I don't know if that has adverse implications. @galaxyeden please advise 🙏

[galaxyeden]

Hi! I've seen this and I'll look this evening – can you share your display config? What I can say is that raising the VCOM inversion thread priority won't help. This is only the priority of sending the serial commands to flip the polarity of VCOM (display common voltage) and protect against electrolytic degradation.

I did see similar in my testing and still do, but at the time was told that people found it even when not inverting VCOM at all (which will damage the display over time). It's been a few months and hopefully this evening I can dig into where these refreshes are being dropped.

[snoyer]

Hi! I've seen this and I'll look this evening – can you share your display config?

Hi! thanks for following up :)
I'm testing with a nice!nano and nice!view on a Corne board (all from https://typeractive.xyz) without any specific config, just compiling with -b nice_nano@2//zmk "-DSHIELD=corne_left nice_view_adapter nice_view", is that what you mean?

What I can say is that raising the VCOM inversion thread priority won't help. This is only the priority of sending the serial commands to flip the polarity of VCOM (display common voltage) and protect against electrolytic degradation.

Maybe it shouldn't help (and as @joelspadin explained above it would not be a real solution anyway), but it does make the problem go away for me and 2 other people over at #3392.

As I mentioned in an earlier message leaving the VCOM priority untouched and setting CONFIG_ZMK_DISPLAY_DEDICATED_THREAD_PRIORITY=11 instead also makes the problem go away (for me at least). Which is why I'm now suspecting it's not about the priority itself but about the drawing/updating code and VCOM code racing each other somehow and playing with priorities just so happens to make that less likely to happen.

Assuming this hypothesis is sensible (and I'll admit I'm only going off of vague notions that "this hardware stuff looks time-sensitive" and "threading is fiddly" here) the fix would be in the ls0xx code itself.
The diff I shared above makes the issue go away as far as I can tell, but I don't know if it's the proper fix (assuming it makes any sense to begin with there are other places where k_sem_give is done before spi_release_dt and that would need changing too?)

[galaxyeden]

@snoyer thanks for the ping, sorry I've been lost in the heatwave this weekend. Your suggestion didn't initially make sense to me but I wanted to look at why and I'm only more confused. k_sem_give hands the semaphore back to the kernel to let another process have it. You're giving it back sooner, yes – but that isn't good, you're giving it back before the hold on the SPI bus is being released.

I have a theory though – and that's that you're allowing the display drawing to start a little sooner. If the VCOM inversion interval is high enough, its possible you're exceeding the wait time.

This entire approach is a bit hacky – doing this in the App code is best, and I noted that in my PR to Zephyr – but this allows for not damaging displays even when the App hasn't been designed for serial VCOM inversion messages on this display. And that's a better tradeoff.

What's the interval between VCOM inversion commands you're using?

[galaxyeden]

I had that totally backwards @snoyer ! I just explained why what I did was wrong and would cause errors! Great spot! I'm writing an upstream PR now

[galaxyeden]

@snoyer I've opened an upstream PR for this – zephyrproject-rtos/zephyr#112225 – this PR should make the same changes as that one does.

[snoyer]

Thanks @galaxyeden! I'll close this PR as the problem should be fixed properly at the source once @petejohanson cherry-picks the changes. No need for hacky config changes in ZMK 🎉

[petejohanson]

The fix has been cherrypicked into our Zephyr fork.