refactor: use low priority workqueue for underglow and battery reporting #1870
Conversation
xudongzheng
force-pushed
the
custom-wq-pr
branch
from
720f42d
to
93db94d
Compare
xudongzheng
force-pushed
the
custom-wq-pr
branch
from
93db94d
to
d32e557
Compare
|
@caksoylar Thanks for the suggestions, both have been addressed. I've also confirmed that the underglow part works on RP2040. |
app/Kconfig
Outdated
| config ZMK_LOW_PRIORITY_THREAD_STACK_SIZE | ||
| int "Low priority thread stack size" | ||
| default 768 | ||
|
|
||
| config ZMK_LOW_PRIORITY_THREAD_PRIORITY | ||
| int "Low priority thread priority" | ||
| default 10 | ||
|
|
There was a problem hiding this comment.
This config is all reasonable, but I don't love where you're dropping it in this file. I would nest this under the "Advanced" menu somewhere.
app/include/zmk/workqueue.h
Outdated
| @@ -0,0 +1 @@ | |||
| extern struct k_work_q lowprio_work_q; | |||
There was a problem hiding this comment.
I don't love exposing this as an extern here. I'd vote for either a function (that would get inlined probably) or a C-macro/define at least.
In particular, if we have a space constrained device (RAM wise) we might want to just-reuse the system workqueue instead of adding a second queue, and using the define/function will let use swap those out w/o the consumer caring as much.
Thoughts?
There was a problem hiding this comment.
I've introduced zmk_lowprio_work_q() but generally I'm hesitant to make things like this configurable unless if there's a good concrete reason - ie 1KB is the difference between fitting and not fitting on a chip.
Furthermore currently the purpose of the low priority workqueue is not well defined. It may end up being used for other tasks that must run with a lower priority than the system workqueue on low resource MCUs, similar to how battery notification is for BLE.
| } | ||
|
|
||
| k_timer_start(&battery_timer, K_MINUTES(1), K_SECONDS(CONFIG_ZMK_BATTERY_REPORT_INTERVAL)); | ||
| k_timer_start(&battery_timer, K_NO_WAIT, K_SECONDS(CONFIG_ZMK_BATTERY_REPORT_INTERVAL)); |
petejohanson
added
enhancement
xudongzheng
force-pushed
the
custom-wq-pr
branch
2 times, most recently
from
8b63db2
to
9ebf522
Compare
|
Code looks reasonable, will test locally soon. |
xudongzheng
force-pushed
the
custom-wq-pr
branch
from
df96296
to
91fbd7d
Compare
|
A separate commit has been added for turning underglow off using the low priority work queue as well. Interestingly I observed quite a bit of LED flickering on RP2040 when |
app/include/zmk/workqueue.h
Outdated
| @@ -0,0 +1 @@ | |||
| struct k_work_q *zmk_lowprio_work_q(); | |||
There was a problem hiding this comment.
Functionality tested w/o issue, but as I peek at this code one more time, it seems this naming should be adjusted to be "namespaced" to match the header, e.g.:
| struct k_work_q *zmk_lowprio_work_q(); | |
| struct k_work_q *zmk_workqueue_lowprio_work_q(); |
Blocking operations on the high priority system workqueue may result in deadlocks, particularly when Bluetooth is in use.
xudongzheng
force-pushed
the
custom-wq-pr
branch
from
91fbd7d
to
1a85bcc
Compare
xudongzheng
force-pushed
the
custom-wq-pr
branch
from
1a85bcc
to
e0ae2b7
Compare
Blocking operations on the high priority system workqueue may result in deadlocks, particularly when Bluetooth is in use.