Feature/advanced biamp - #187
Open
craigmillard86 wants to merge 64 commits into
Open
Conversation
added 30 commits
January 6, 2026 18:43
…DAC state tracking. Added debug logging.
…mponent to apply stored settings at startup and expose settings structure to http component, Updated html for the UI
…l_settings component architecture. Fully data-driven front-end
…log gain to tas5805m_settings
…equency, analog gain. Added support for readonly parameters
- Add missing Connection: close header to EQ settings handler (16KB response) - Increase delay between EQ schema and settings requests from 100ms to 300ms - Allows ESP32 memory recovery after 64KB schema response before 16KB settings
The fault monitoring task was crashing with stack overflow when faults were detected. The tas5805m_decode_faults() function can make up to 12 ESP_LOGW calls with formatted strings, each consuming 200-400 bytes of stack for vsnprintf operations. Combined with I2C operations and task overhead, this exceeded the 2048 byte allocation. Increased stack from 2048 to 4096 bytes to provide adequate headroom.
Contributor
|
@craigmillard86 can you include screeshot here for reference as well. I'm porting some of that into Raspberry Pi kernel driver, looking which options can be of use in Bi-Amp setup. |
Contributor
Author
No Problem, attached
|
craigmillard86
added a commit
to anabolyc/esp32-snapclient
that referenced
this pull request
Jan 25, 2026
# Conflicts: # components/lightsnapcast/player.c
Owner
|
Ok, so I missed that one and now there are conflicts @craigmillard86 |
Contributor
Author
|
@CarlosDerSeher have removed all the merge conflicts |
There was a problem hiding this comment.
Pull request overview
This PR adds an “Advanced Bi-Amp” DSP configuration path for TAS5805M (active crossover + per-output PEQ + loudness), plus corresponding preset export/import/reset APIs and substantial web UI + HTTP server robustness improvements (mobile reliability, retries, socket cleanup).
Changes:
- Introduces advanced bi-amp DSP + persistence (NVS), loudness compensation, and preset export/import/reset plumbing.
- Extends EQ web UI to render the new Advanced Bi-Amp schema layout (sections/subgroups/radio) and adds mobile responsiveness + retry/debounce behavior.
- Improves HTTP server reliability (timeouts/backlog, large-response connection close) and tightens query parsing/CORS behavior.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| partitions.csv | Fixes missing partition index for ota_1. |
| docs/ADVANCED_BIAMP_USER_GUIDE.md | Adds end-user documentation for advanced bi-amp crossover/PEQ/loudness/presets. |
| components/ui_http_server/ui_http_server.c | Hardens URL decode + query parsing, modifies CORS handling, adds bi-amp preset/reset endpoints, adjusts HTTP server config/timeouts. |
| components/ui_http_server/html/styles.css | Adds mobile responsiveness improvements for shared styles. |
| components/ui_http_server/html/settings-ui.js | Adds radio controls, conditional visibility, and grouped renderers for bi-amp/loudness UI layouts. |
| components/ui_http_server/html/index.html | Adds responsive navigation styling for mobile. |
| components/ui_http_server/html/eq-settings.html | Adds bi-amp UI rendering, preset buttons, retry logic, and debounced setting updates. |
| components/ui_http_server/html/dac-settings.html | Adds retry logic and better error messaging if scripts aren’t loaded. |
| components/tas5805m_settings/tas5805m_settings.c | Adds advanced bi-amp + loudness settings persistence/apply, schema generation updates, and preset import/export/reset. |
| components/tas5805m_settings/tas5805m_biamp.c | New DSP/biquad coefficient generation and targeted apply helpers for advanced bi-amp. |
| components/tas5805m_settings/include/tas5805m_settings.h | Adds types/NVS keys/APIs for bi-amp, loudness, and presets; introduces new EQ UI mode. |
| components/tas5805m_settings/include/tas5805m_biamp.h | New public bi-amp DSP API and band allocation definitions. |
| components/tas5805m_settings/CMakeLists.txt | Adds tas5805m_biamp.c to the component build. |
| components/lightsnapcast/player.c | Minor formatting change. |
| components/custom_board/tas5805m/tas5805m.c | Adds I2C mutexing, fault task stack increase, loudness-on-volume-change hook, and biquad feedback sign convention adjustment. |
| components/custom_board/CMakeLists.txt | Adds dependency/include path wiring for tas5805m_settings when TAS5805M is enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
|
Also please address Copilot comments where necessary |
craigmillard86
added a commit
to craigmillard86/snapclient
that referenced
this pull request
Apr 23, 2026
…and cleanup fixes - Fix CORS origin validation: replace prefix matching with is_local_origin() that verifies next char is digit/terminator, validates 172.16-31 range - Add httpd_recv_all() helper to handle partial reads in all POST handlers - Add 32KB max body size check (413 Payload Too Large) to all POST handlers - Fix biquad read/write asymmetry: negate a1/a2 on read to match write convention - Remove mutex creation from portENTER_CRITICAL section in tas5805m_init() - Remove unused BW/LR filter type selector from UI (always Linkwitz-Riley) - Fix frontend URLs to use getBackendUrl() for preset export/import/reset/sendBeacon - Fix documentation treble frequency (~5 kHz -> ~4 kHz to match code) - Revert whitespace-only changes in player.c
Contributor
Author
All now updated. |
Contributor
|
The commit is missing here. You pushed to a different branch. |
…and cleanup fixes - Fix CORS origin validation: replace prefix matching with is_local_origin() that verifies next char is digit/terminator, validates 172.16-31 range - Add httpd_recv_all() helper to handle partial reads in all POST handlers - Add 32KB max body size check (413 Payload Too Large) to all POST handlers - Fix biquad read/write asymmetry: negate a1/a2 on read to match write convention - Remove mutex creation from portENTER_CRITICAL section in tas5805m_init() - Remove unused BW/LR filter type selector from UI (always Linkwitz-Riley) - Fix frontend URLs to use getBackendUrl() for preset export/import/reset/sendBeacon - Fix documentation treble frequency (~5 kHz -> ~4 kHz to match code) - Revert whitespace-only changes in player.c
Replace Promise.all([schema, settings]) with staged sequential fetches (schema -> 300ms delay -> settings) in both the mode-change and auto-mode gain/preset reload paths, matching the pattern already used in loadSettings(). Prevents socket/memory issues on mobile clients.
Contributor
Author
Done, pushed to my repo instead of @anabolyc |
Contributor
Author
|
@CarlosDerSeher anything else needed on this branch? |
Owner
|
I don't think so. It had conflicts but other than that I think it is ok |
| static const char *TAG = "TAS5805M"; | ||
|
|
||
| /* Mutex for thread-safe I2C access */ | ||
| static SemaphoreHandle_t tas5805m_i2c_mutex = NULL; |
Owner
There was a problem hiding this comment.
I don't think we have this anywhere. Do we really need this?
Contributor
Author
There was a problem hiding this comment.
Yes witha bit of help from Claude, this driver is reached from three tasks concurrently, and nothing else
serialises it:
tas5805m_faults, the fault monitor started intas5805m_init()tas5805m_poll_play, started fromtas5805m_settings.c- the httpd task, whenever the UI writes DAC/EQ settings
It's true we don't do this in the other DAC drivers, but this one is a
different case: it doesn't go through i2c_bus (which has its own bus_lock)
— it calls the legacy i2c_master_cmd_begin() API directly, so there is no
locking underneath it at all.
Two things genuinely need it:
tas5805m_read_byte()/read_bytes()are write-address-then-read
sequences. An interleaved write between those two transactions returns
the wrong register's contents.- The book/page registers are device-global.
TAS5805M_SET_BOOK_AND_PAGE
selects a page, the caller then writes the target register — if another
task changes the page in between, the write lands in the wrong page.
You were right to poke at it though: (2) wasn't actually protected. The mutex
was only held per transfer, so each of the three writes inside
SET_BOOK_AND_PAGE was individually atomic but the overall
select → access → restore sequence was not. I've pushed a fix that holds the
mutex across the whole sequence in all seven paged functions
(set_mixer_gain, set_channel_gain, set_eq_mode, set_eq_gain_channel,
set_eq_profile_channel, and the two biquad coefficient functions). It was
already a recursive mutex, so the per-transfer locking still works underneath.
partitions.csv: revert. The only change was adding a trailing newline, no content change, so the file no longer belongs in this PR. ui_http_server.c: derive max_open_sockets from CONFIG_LWIP_MAX_SOCKETS instead of hardcoding 7. esp_http_server reserves 3 sockets for its own use (listener plus two UDP control sockets) and rejects anything above LWIP_MAX_SOCKETS - 3, so lowering that config would previously have made httpd_start() fail with ESP_ERR_INVALID_ARG. tas5805m.c: hold the I2C mutex across whole paged sequences. The book and page registers are device-global, so selecting a page and then accessing a register in it is only safe if no other task can change the page in between. The mutex was only held per transfer, which left every SET_BOOK_AND_PAGE -> access -> restore sequence open to interleaving. Three tasks reach this driver concurrently: the tas5805m_faults monitor, the tas5805m_poll_play task, and the httpd task when the UI writes settings. The mutex is already recursive, so the per-transfer locking still works underneath. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Building with any config that sets CONFIG_DAC_TAS5805M (e.g. sdkconfig_TAS5805M) failed to compile. Both faults predate this branch's review changes. tas5805m.c: remove the stale duplicate definitions of tas5805m_write_bytes and tas5805m_read_bytes. Two unconditional copies of each existed, with no preprocessor guard between them, so the translation unit could never compile. The removed copies are the older pre-mutex versions; the retained ones do the I2C locking. Introduced by 98c439d. player.c: move the player_pm_lock_handle declaration out of the USE_SAMPLE_INSERTION block. It is declared under "#if USE_SAMPLE_INSERTION / #if CONFIG_PM_ENABLE" but used under "#if CONFIG_PM_ENABLE" alone, so any config with power management on and sample insertion off failed with 'player_pm_lock_handle' undeclared. sdkconfig_TAS5805M is exactly that combination. Verified: sdkconfig_TAS5805M now builds, and the CI default builds for esp32 and esp32s3 still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
de08d8e removed the duplicate definitions of tas5805m_write_bytes and tas5805m_read_bytes, keeping the copies that do the I2C locking. Those copies still used the pre-CarlosDerSeher#218 tick macros, so retaining them silently reverted the pdMS_TO_TICKS conversion from 51815d4 for these two functions. Convert the retained copies as well. The helpers are now identical to the develop versions apart from the mutex handling. No behavioural change, 1000 / portTICK_RATE_MS and pdMS_TO_TICKS(1000) evaluate the same. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
PR: Advanced TAS58505 Bi-Amp Crossover with PEQ, Loudness Compensation, and UI Improvements
Summary
This PR introduces a comprehensive advanced bi-amp crossover system with parametric EQ, loudness compensation, and significant improvements to the web UI and HTTP server reliability.
Key Features
Bug Fixes