New UserMod: Voice Control via DF2301Q #329
Open
troyhacks wants to merge 5 commits intoMoonModules:mdevfrom
Open
New UserMod: Voice Control via DF2301Q #329troyhacks wants to merge 5 commits intoMoonModules:mdevfrom
troyhacks wants to merge 5 commits intoMoonModules:mdevfrom
Conversation
📝 WalkthroughWalkthroughAdds a DF2301Q I2C driver header and a DF2301QUsermod that detects and polls a DF2301Q voice module, maps CMDIDs to WLED actions, persists configuration and UI entries, and wires the usermod into the build (usermod ID, conditional registration, and platformio build flag). Changes
Sequence Diagram(s)sequenceDiagram
participant Sys as "System"
participant UM as "DF2301QUsermod"
participant Driver as "DF2301Q Driver"
participant I2C as "I2C Bus"
participant Mod as "DF2301Q Module"
participant WLED as "WLED Core"
Sys->>UM: setup()
UM->>Driver: new DF2301Q(addr)
UM->>Driver: detect()
Driver->>I2C: probe/ping @ addr
I2C->>Mod: I2C read/write
Mod-->>I2C: response
I2C-->>Driver: ack/result
Driver-->>UM: detection result
loop every pollInterval
UM->>Driver: poll()
Driver->>I2C: readReg(CMDID)
I2C->>Mod: read CMDID
Mod-->>I2C: cmdID
I2C-->>Driver: cmdID
Driver-->>UM: new cmdID (if changed)
UM->>UM: handleVoiceCommand(cmdID)
UM->>WLED: apply action (power/brightness/preset/effect/color)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@usermods/usermod_v2_voice_control/DF2301Q.hpp`:
- Around line 19-85: The task is being pinned to core 1 unconditionally in
DF2301Q::startTask via xTaskCreatePinnedToCore which fails on single‑core ESP32
variants; update the core selection so single‑core builds use core 0 by using
conditional compilation (check CONFIG_FREERTOS_UNICORE) and set
DF2301Q_TASK_CORE to 0 for unicore and 1 otherwise, or compute the core argument
inline in startTask before calling xTaskCreatePinnedToCore; modify the symbol
DF2301Q_TASK_CORE (or the call site in startTask that passes DF2301Q_TASK_CORE)
accordingly so the task is pinned to PRO_CPU (core 0) on single‑core targets and
to core 1 on multicore targets.
In `@usermods/usermod_v2_voice_control/usermod_v2_voice_control.h`:
- Around line 403-410: The local variable that stores the current effect/mode
(currently declared as uint8_t in the block handling cmdPrevEffect and used with
strip.getMainSegment().mode and strip.setMode) must be widened to uint16_t to
avoid truncation for mode IDs >255; update the declaration of mode in this
handler (and any other mode-index locals in this usermod) from uint8_t to
uint16_t, keep palette-related types as uint8_t, and ensure the wrap logic uses
strip.getModeCount() unchanged while still calling
strip.setMode(strip.getMainSegmentId(), ...), and leave
stateUpdated(CALL_MODE_BUTTON) and cmdID/cmdPrevEffect checks as-is.
- Around line 394-400: The branch handling cmdPrevPreset can underflow when
currentPreset is 0; update the conditional in the cmdPrevPreset block (where
cmdPrevPreset, cmdNextPreset, cmdID and applyPreset are used) to prevent calling
applyPreset(currentPreset - 1, CALL_MODE_BUTTON) with currentPreset == 0 —
either skip the action when currentPreset is 0 or compute a safe previous index
(e.g., wrap to last preset by using (currentPreset == 0 ? maxPresetIndex :
currentPreset - 1)) before calling applyPreset and keep the USER_PRINTLN message
unchanged.
- Around line 255-307: The volume range is inconsistent with DF2301Q (should be
1–7): update the dropdown generation in the UI (the SET_F("for(var
i=0;i<=20;i++)addOption(dd,i,i);") call created by addDropdown for 'Volume') to
only emit options 1 through 7, change the default moduleVolume value to a valid
in-range default (replace the current 10 with a value between 1 and 7), and
tighten the bounds in readFromConfig by changing the constrain(moduleVolume, 0,
20) call to constrain(moduleVolume, 1, 7) so moduleVolume is always clamped to
the hardware-supported range. Ensure all references to moduleVolume (including
addToConfig/readFromConfig) rely on that corrected range.
In `@wled00/usermods_list.cpp`:
- Around line 209-212: Remove the redundant self-define inside the conditional
block: in the section guarded by `#ifdef` USERMOD_VOICE_CONTROL, delete the line
that redefines USERMOD_VOICE_CONTROL and leave only the `#include` of
"../usermods/usermod_v2_voice_control/usermod_v2_voice_control.h" (keep the
surrounding `#ifdef/`#endif intact) so that external -DUSERMOD_VOICE_CONTROL=1
values are not clobbered and `#if` USERMOD_VOICE_CONTROL checks continue to work.
🧹 Nitpick comments (1)
usermods/usermod_v2_voice_control/DF2301Q.hpp (1)
151-192: Protect Wire I2C access from concurrent background task execution.The
readReg()andwriteReg()methods at lines 173–191 are called from the background FreeRTOS task (taskFunction), but multiple other usermods also access Wire from the main loop without mutual exclusion. Wrap these methods withesp32SemTake()andesp32SemGive()(or create a dedicated I2C semaphore) to prevent concurrent transactions that corrupt the bus.
…ED by default, but this will do it as usermods are callled one by one.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@usermods/usermod_v2_voice_control/usermod_v2_voice_control.h`:
- Around line 96-116: When DF2301Q detection fails in setup(), do not delete the
allocated voiceModule or set it to nullptr; remove the delete voiceModule and
voiceModule = nullptr statements in the else branch so the DF2301Q instance
remains allocated (voiceModule, DF2301Q::detect()) and the existing recovery
logic in loop() can retry initialization; optionally keep the USER_PRINTLN log
but leave the object in place to allow hot‑plug reconnection.
… removed from the else branch. Now when initial detection fails in setup(), the DF2301Q instance remains allocated, allowing the recovery logic in loop() to retry detection every 5 seconds for hot-plug reconnection.
Collaborator
|
@coderabbitai please re-review this PR. Use the new review instructions and coding guidelines (.coderabbit.yaml) that were just added in the main mdev branch. |
🧩 Analysis chain
Re-review requested by 🛠 Debug Information Gathered
Kicking off the full re-review now. Re-review requested by 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
usermods/usermod_v2_voice_control/usermod_v2_voice_control.h (1)
373-376:⚠️ Potential issue | 🟡 MinorKeep previous-preset navigation 1-based.
currentPreset == 1still reachesapplyPreset(0, ...)here. If preset0is not a real target in this mapping, the first preset should no-op or wrap instead of stepping to zero.Proposed fix
} else if (cmdPrevPreset > 0 && cmdID == cmdPrevPreset) { - if (currentPreset > 0) { + if (currentPreset > 1) { applyPreset(currentPreset - 1, CALL_MODE_BUTTON); } USER_PRINTLN(F("DF2301Q: Previous Preset"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/usermod_v2_voice_control/usermod_v2_voice_control.h` around lines 373 - 376, The previous-preset branch allows currentPreset==1 to call applyPreset(0,...), which moves to an invalid 0 index; update the guard in the block that checks cmdPrevPreset/cmdID so it only calls applyPreset(currentPreset - 1, CALL_MODE_BUTTON) when currentPreset > 1 (or implement explicit wrap behavior if wrapping is desired); locate the variables cmdPrevPreset, cmdID, currentPreset and the call applyPreset(..., CALL_MODE_BUTTON) and change the condition from currentPreset > 0 to currentPreset > 1 (or add wrap logic) so preset navigation remains 1-based.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/usermod_v2_voice_control/usermod_v2_voice_control.h`:
- Around line 297-300: The loaded pollInterval must be clamped away from zero to
avoid a tight I2C polling loop; after reading pollInterval (the assignment
pollInterval = top[FPSTR(_pollInterval)] | pollInterval;) enforce a minimum
(e.g., pollInterval = max(1, pollInterval); or pollInterval =
constrain(pollInterval, 1, <upper_bound>);) so saved 0 values become a small
positive interval; update the same config-loading block that sets moduleVolume
and wakeTime to apply this clamp to pollInterval.
- Around line 129-137: The recovery path in loop() uses voiceModule->detect(3,
100) which performs multiple blocking probes and can sleep ~200ms on each failed
attempt causing hitches; change it to perform a single non-blocking probe (e.g.,
call detect with a single attempt or otherwise invoke a one-shot check) so the
lastRecoveryAttempt rate-limit remains but each retry only does one quick probe;
keep the existing lastRecoveryAttempt logic and, on a successful single probe,
call voiceModule->setVolume(moduleVolume) and voiceModule->setWakeTime(wakeTime)
as before.
- Around line 353-356: The code handling the power-off command saves briLast
even when the strip is already off (bri == 0), causing a subsequent power-on to
restore 0; change the logic in the cmdPowerOff branch (where cmdPowerOff, cmdID,
briLast, bri and stateUpdated(CALL_MODE_BUTTON) are used) so briLast is only
assigned when bri > 0 (i.e., snapshot previous brightness only if non-zero)
before setting bri = 0 and calling stateUpdated(CALL_MODE_BUTTON).
---
Duplicate comments:
In `@usermods/usermod_v2_voice_control/usermod_v2_voice_control.h`:
- Around line 373-376: The previous-preset branch allows currentPreset==1 to
call applyPreset(0,...), which moves to an invalid 0 index; update the guard in
the block that checks cmdPrevPreset/cmdID so it only calls
applyPreset(currentPreset - 1, CALL_MODE_BUTTON) when currentPreset > 1 (or
implement explicit wrap behavior if wrapping is desired); locate the variables
cmdPrevPreset, cmdID, currentPreset and the call applyPreset(...,
CALL_MODE_BUTTON) and change the condition from currentPreset > 0 to
currentPreset > 1 (or add wrap logic) so preset navigation remains 1-based.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53092d7b-4b99-480e-9dd8-88b48409b2c2
📒 Files selected for processing (5)
platformio.iniusermods/usermod_v2_voice_control/DF2301Q.hppusermods/usermod_v2_voice_control/usermod_v2_voice_control.hwled00/const.hwled00/usermods_list.cpp
Comment on lines
+129
to
+137
| static unsigned long lastRecoveryAttempt = 0; | ||
| if (millis() - lastRecoveryAttempt > 5000) { | ||
| lastRecoveryAttempt = millis(); | ||
| USER_PRINTLN(F("DF2301Q: Attempting to reconnect...")); | ||
| if (voiceModule->detect(3, 100)) { | ||
| USER_PRINTLN(F("DF2301Q: Module reconnected!")); | ||
| voiceModule->setVolume(moduleVolume); | ||
| voiceModule->setWakeTime(wakeTime); | ||
| } |
There was a problem hiding this comment.
Use a single probe in the recovery loop.
Lines 133-137 call detect(3, 100), which can sleep for roughly 200 ms on every failed reconnect attempt. Because this runs from loop(), a missing module will periodically hitch animations and network work. Keep the retries in setup(), but make the recovery path a single probe.
Proposed fix
- if (voiceModule->detect(3, 100)) {
+ if (voiceModule->detect(1, 0)) {
USER_PRINTLN(F("DF2301Q: Module reconnected!"));
voiceModule->setVolume(moduleVolume);
voiceModule->setWakeTime(wakeTime);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/usermod_v2_voice_control/usermod_v2_voice_control.h` around lines
129 - 137, The recovery path in loop() uses voiceModule->detect(3, 100) which
performs multiple blocking probes and can sleep ~200ms on each failed attempt
causing hitches; change it to perform a single non-blocking probe (e.g., call
detect with a single attempt or otherwise invoke a one-shot check) so the
lastRecoveryAttempt rate-limit remains but each retry only does one quick probe;
keep the existing lastRecoveryAttempt logic and, on a successful single probe,
call voiceModule->setVolume(moduleVolume) and voiceModule->setWakeTime(wakeTime)
as before.
Comment on lines
+297
to
+300
| moduleVolume = top[FPSTR(_moduleVolume)] | moduleVolume; | ||
| moduleVolume = constrain(moduleVolume, 0, 20); // 0-20, higher values may distort | ||
| wakeTime = top[FPSTR(_wakeTime)] | wakeTime; | ||
| pollInterval = top[FPSTR(_pollInterval)] | pollInterval; |
There was a problem hiding this comment.
Clamp pollInterval away from zero.
A saved value of 0 makes the check at Lines 144-149 true on every pass through loop(), turning this into a tight I2C polling loop. Enforce a small positive minimum when loading config.
Proposed fix
moduleVolume = top[FPSTR(_moduleVolume)] | moduleVolume;
moduleVolume = constrain(moduleVolume, 0, 20); // 0-20, higher values may distort
wakeTime = top[FPSTR(_wakeTime)] | wakeTime;
pollInterval = top[FPSTR(_pollInterval)] | pollInterval;
+ if (pollInterval < 10) pollInterval = 10;
cmdPowerOn = top[FPSTR(_cmdPowerOn)] | cmdPowerOn;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/usermod_v2_voice_control/usermod_v2_voice_control.h` around lines
297 - 300, The loaded pollInterval must be clamped away from zero to avoid a
tight I2C polling loop; after reading pollInterval (the assignment pollInterval
= top[FPSTR(_pollInterval)] | pollInterval;) enforce a minimum (e.g.,
pollInterval = max(1, pollInterval); or pollInterval = constrain(pollInterval,
1, <upper_bound>);) so saved 0 values become a small positive interval; update
the same config-loading block that sets moduleVolume and wakeTime to apply this
clamp to pollInterval.
Comment on lines
+353
to
+356
| } else if (cmdPowerOff > 0 && cmdID == cmdPowerOff) { | ||
| briLast = bri; | ||
| bri = 0; | ||
| stateUpdated(CALL_MODE_BUTTON); |
There was a problem hiding this comment.
Preserve briLast on redundant "off" commands.
When the strip is already off, this stores 0 into briLast. The next power-on then restores 0 again and the light stays off. Only snapshot briLast when brightness is non-zero.
Proposed fix
} else if (cmdPowerOff > 0 && cmdID == cmdPowerOff) {
- briLast = bri;
+ if (bri > 0) briLast = bri;
bri = 0;
stateUpdated(CALL_MODE_BUTTON);
USER_PRINTLN(F("DF2301Q: Power OFF"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/usermod_v2_voice_control/usermod_v2_voice_control.h` around lines
353 - 356, The code handling the power-off command saves briLast even when the
strip is already off (bri == 0), causing a subsequent power-on to restore 0;
change the logic in the cmdPowerOff branch (where cmdPowerOff, cmdID, briLast,
bri and stateUpdated(CALL_MODE_BUTTON) are used) so briLast is only assigned
when bri > 0 (i.e., snapshot previous brightness only if non-zero) before
setting bri = 0 and calling stateUpdated(CALL_MODE_BUTTON).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The DF2301Q is a very inexpensive offline voice control board that you can also train for a custom wake-phrase as well as 17 other custom commands - and it understands over 120 generic commands without training.
It takes almost zero resources as everything is done on the board.
Product link: https://www.dfrobot.com/product-2665.html
List of the pre-trained phrases
To keep the data small, I only included a subset of the words it can recognize that are related to lighting or similar things.
Summary by CodeRabbit
New Features
Chores