Open
Conversation
Contributor
Reviewer's GuideAdds a microphone-based audio visualization mode that samples the ADC, computes a dynamic audio level, and updates the LED framebuffer on a periodic task, along with basic audio driver initialization and build wiring. Sequence diagram for periodic audio visualization updatesequenceDiagram
participant TMOS_Scheduler
participant CommonTask
participant AudioModule
participant ADC_Hardware
participant LEDDriver
TMOS_Scheduler->>CommonTask: AUDIO_STEP event
CommonTask->>AudioModule: audio_visualize_poll()
activate AudioModule
AudioModule->>AudioModule: mic_adc()
activate AudioModule
AudioModule->>ADC_Hardware: ADC_ChannelCfg(11)
loop 64 samples
AudioModule->>ADC_Hardware: ADC_ExcutSingleConver()
ADC_Hardware-->>AudioModule: raw_sample
end
AudioModule->>AudioModule: compute average and subtract 2048
deactivate AudioModule
AudioModule->>AudioModule: store mic value in values buffer
AudioModule->>AudioModule: scan values to compute max
AudioModule->>AudioModule: scale mic = mic * 7 / max
AudioModule->>AudioModule: clamp mic to 0..7
loop for each LED column
AudioModule->>LEDDriver: fb[i] = amp_wav_lut[mic]
end
deactivate AudioModule
CommonTask-->>TMOS_Scheduler: clear AUDIO_STEP event
Updated class diagram for audio module and common task integrationclassDiagram
class AudioModule {
+uint16_t amp_wav_lut[8]
+uint16_t amp_wav_lut_w1[8]
+int16_t mic_adc()
+void mic_init()
}
class MainTasks {
+uint16_t AUDIO_STEP
+uint16_t fb[LED_COLS]
+void audio_visualize_poll()
+void mode_setup_audio_visualize()
+uint16_t common_tasks(tmosTaskID task_id, uint16_t events)
}
class SystemInit {
+int main()
}
SystemInit --> AudioModule : calls mic_init
SystemInit --> MainTasks : initializes tasks
MainTasks --> AudioModule : calls mic_adc
MainTasks --> AudioModule : uses amp_wav_lut
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
amp_wav_lutandamp_wav_lut_w1arrays are defined (not just declared) inaudio.h, which will cause multiple definition linker errors; move the definitions intoaudio.cand keep onlyexterndeclarations in the header. - In
audio_visualize_poll,maxcan be zero before the buffer is meaningfully populated or under very low/no input, which will lead to a divide-by-zero when computingmic * 7 / max; add a guard (e.g., skip scaling or set a minimummax) to avoid this. - The ADC sampling loop in
mic_adcis synchronous and relatively heavy (64 samples per call) and is now polled periodically; consider reducing the sample count or moving to an interrupt/continuous mode earlier to avoid potential timing/latency issues in the common task loop.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `amp_wav_lut` and `amp_wav_lut_w1` arrays are defined (not just declared) in `audio.h`, which will cause multiple definition linker errors; move the definitions into `audio.c` and keep only `extern` declarations in the header.
- In `audio_visualize_poll`, `max` can be zero before the buffer is meaningfully populated or under very low/no input, which will lead to a divide-by-zero when computing `mic * 7 / max`; add a guard (e.g., skip scaling or set a minimum `max`) to avoid this.
- The ADC sampling loop in `mic_adc` is synchronous and relatively heavy (64 samples per call) and is now polled periodically; consider reducing the sample count or moving to an interrupt/continuous mode earlier to avoid potential timing/latency issues in the common task loop.
## Individual Comments
### Comment 1
<location> `src/main.c:126-135` </location>
<code_context>
bmlist_drop(curr_bm);
}
+static void audio_visualize_poll()
+{
+ static uint8_t current = 0;
+ static int16_t values[64];
+ int16_t mic = abs(mic_adc());
+ int16_t max = 0;
+ values[current++] = mic;
+ if (current >= sizeof(values)/sizeof(values[0])) current=0;
+ for (int i=0; i<sizeof(values)/sizeof(values[0]); i++) {
+ if (values[i] > max) max = values[i];
+ }
+
+ mic = mic * 7 / max;
+ if (mic > 7) mic = 7;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against division by zero when `max` stays at 0.
If this is called before `values` has any non-zero samples, `max` remains 0 and `mic * 7 / max` will divide by zero. Add a guard (e.g., early-return when `max == 0` or clamp `max` to at least 1) before performing the division.
</issue_to_address>
### Comment 2
<location> `src/audio.h:6-15` </location>
<code_context>
+
+#include <stdint.h>
+
+const uint16_t amp_wav_lut[8] = {
+ 0b00000000000,
+ 0b00000100000,
+ 0b00001110000,
+ 0b00011111000,
+ 0b00111111100,
+ 0b01111111110,
+ 0b11111111111,
+ 0b11111111111,
+};
+
+const uint16_t amp_wav_lut_w1[8] = {
</code_context>
<issue_to_address>
**issue (bug_risk):** Move the LUT definition to a single `.c` file and declare it `extern` in the header.
Defining `amp_wav_lut` in the header creates a separate definition in every including translation unit, which can lead to multiple-definition link errors. Keep the definition in `audio.c` and only declare it in `audio.h` as `extern const uint16_t amp_wav_lut[8];`.
</issue_to_address>
### Comment 3
<location> `src/audio.h:17-15` </location>
<code_context>
+ 0b11111111111,
+};
+
+const uint16_t amp_wav_lut_w1[8] = {
+ 0b00000100000,
+ 0b00001110000,
+ 0b00011111000,
+ 0b00111111100,
+ 0b01111111110,
+ 0b11111111111,
+ 0b11111111111,
+ 0b11111111111,
+};
+
+int16_t mic_adc();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Apply the same `extern` pattern to `amp_wav_lut_w1` as for `amp_wav_lut`.
Define `amp_wav_lut_w1` in `audio.c` and declare it as `extern` in `audio.h`, matching the pattern used for `amp_wav_lut` to prevent multiple definitions.
Suggested implementation:
```c
extern const uint16_t amp_wav_lut_w1[8];
```
1. In `src/audio.c`, add the actual definition of `amp_wav_lut_w1` (matching `amp_wav_lut`'s pattern), for example:
```c
const uint16_t amp_wav_lut_w1[8] = {
0b00000100000,
0b00001110000,
0b00011111000,
0b00111111100,
0b01111111110,
0b11111111111,
0b11111111111,
0b11111111111,
};
```
2. Ensure `audio.c` already includes `"audio.h"` so the declaration and definition stay in sync.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
needs a way to setup the mode as work queue cuz the current implementation is polling inside the button interrupt routine.
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.
I used the branch in PR #104 and got it running now on the latest prototypes. Max audio level is now calculated dynamically to make sure it always shows correct levels. Additionally the audio mode uses the same way for updating as the other modes.
Summary by Sourcery
Add a microphone-based audio visualization mode that dynamically maps input levels to LED patterns and integrates it into the existing mode/task framework.
New Features:
Enhancements:
Build: