zephyrCommon: Make configurable the max number of tones#152
zephyrCommon: Make configurable the max number of tones#152DhruvaG2000 merged 3 commits intozephyrproject-rtos:nextfrom
Conversation
d1fa1d4 to
41a8a6b
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the Zephyr Arduino core’s tone() implementation to support more flexible timer allocation by dynamically selecting unused timers, and adds a Kconfig option to control the maximum number of concurrent tones.
Changes:
- Add
CONFIG_ARDUINO_MAX_TONES(default-1) to configure the tone timer pool size. - Track timer usage via a per-timer
pinfield and allocate/release timers dynamically intone()/noTone().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cores/arduino/zephyrCommon.cpp | Implements dynamic allocation/release of tone timers and makes the pool size configurable. |
| Kconfig | Adds ARDUINO_MAX_TONES configuration option (default -1 for auto sizing). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f424836 to
2c4afe2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
13e2210 to
2c0babd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cores/arduino/zephyrCommon.cpp:281
- When frequency is 0, the function returns early without clearing the timer state (setting pt->pin = pin_size_t(-1)). This means the timer slot remains allocated to this pin even though it's no longer active. If this pin calls tone() again later, it will find and reuse the same timer, which is fine. However, if the maximum number of timers is reached and this pin's timer is not properly freed, it could prevent other pins from allocating timers. Consider adding
pt->pin = pin_size_t(-1);before the return on line 281 to properly free the timer slot.
if (frequency == 0) {
gpio_pin_set_dt(spec, 0);
return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3d7c27c to
06b1038
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ca91026 to
00dc900
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b5304d5 to
312374f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1ae6215 to
f9a2e52
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b837aad to
6fffccc
Compare
cores/arduino/zephyrCommon.cpp
Outdated
|
|
||
|
|
||
| if (pin == pin_size_t(-1)) { | ||
| unusedIndex = i; |
There was a problem hiding this comment.
This slot could be claimed by another thread right?
|
|
||
| k_spin_unlock(&arduino_pin_timers[i].lock, key); | ||
|
|
||
|
|
There was a problem hiding this comment.
I've overhauled this entire function.
cores/arduino/zephyrCommon.cpp
Outdated
| k_spin_unlock(&arduino_pin_timers[i].lock, key); | ||
|
|
||
|
|
||
| if (pin == pin_size_t(-1)) { |
There was a problem hiding this comment.
The function releases the lock immediately after reading pin, then uses that stale value. Between unlock and the check, another thread could claim that slot right?
cores/arduino/zephyrCommon.cpp
Outdated
| size_t unusedIndex = size_t(-1); | ||
|
|
||
| for (size_t i = 0; i < ARRAY_SIZE(arduino_pin_timers); i++) { | ||
| k_spinlock_key_t key = k_spin_lock(&arduino_pin_timers[i].lock); | ||
| size_t pin = arduino_pin_timers[i].pin; | ||
|
|
||
| k_spin_unlock(&arduino_pin_timers[i].lock, key); | ||
|
|
||
|
|
||
| if (pin == pin_size_t(-1)) { | ||
| unusedIndex = i; | ||
| } | ||
|
|
||
| if (pin == pinNumber) { | ||
| return &arduino_pin_timers[i]; | ||
| } | ||
| } | ||
|
|
||
| if (unusedIndex != size_t(-1) && !active_only) { | ||
| return &arduino_pin_timers[unusedIndex]; |
There was a problem hiding this comment.
oh I see copilot had already caught this? What was the reason for resolving?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pt->pin = pin_size_t(-1); | ||
| } else { | ||
| if (pin >= 0) { | ||
| if (pin != pin_size_t(-1)) { |
There was a problem hiding this comment.
why are these changes inside add tone_doremi sample? Can be very misleading, please split into another commit.
There was a problem hiding this comment.
Oops!
It was misoperation, Fixed. Thank you for catching.
Fixed a bug where tone() with duration=0 would stop the tone immediately instead of ringing infinitely. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Allows you to change the maximum number of notes that can be played with `tone()`. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Add a tone_doremi sample to demonstrate how to sounding with tone API Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Fixed a bug where tone() with duration=0 would stop the tone immediately instead of ringing infinitely.
Allows you to change the maximum number of notes that can be played with
tone().Add a tone_doremi sample to demonstrate how to sounding with tone API