Add Waveshare S3 Tubes remote and v47 field UI - #71
Conversation
There was a problem hiding this comment.
I’m requesting changes because the PR currently alters several existing Tubes and WLED behaviors beyond what is needed to add the S3 profile.
The PR commit is already based on the current main, so a normal rebase may not resolve this by itself. Several of the edited files contain the shape of older code and directly delete newer behavior from main, which suggests the S3 work may have been applied from older local copies or reconciled at file level. Please compare each modified shared file against main, restore the current implementation, and then reapply only the S3-specific changes.
The regressions happen through these concrete changes:
Tubes.hremovesreadFromJsonState()andaddToJsonInfo(). Without those usermod callbacks, WLED no longer forwards/json/stateTubes operations tocontroller.readJsonOperations(), and the existing Tubes v3 status disappears from/json/info.Tubes.halso removes the HomeLight guards around setup, overlay drawing, and button handling. As a result, HomeLight Wi-Fi mode receives the Tubes transition, overlay, and button behavior even though that role is designed to leave its LEDs under normal WLED control.controller.handvirtual_strip.hremove the runtime-palette argument fromVirtualStrip::load(). Literal v3 palettes are still installed in the WLED segment, but internal Tubes layers reload the legacy palette ID instead of snapshotting the received colors, so coordinated devices can render different colors.const.hremoves the#ifndef DEFAULT_LED_COUNTguard. The header then redefines board-provided values such as 64 or 150 to 30 after they were supplied on the compiler command line; the Dig2Go build demonstrates this with redefinition warnings throughout the build, and core geometry receives the wrong default.- The new null bus uses type
95, which is inside WLED’s virtual-network range of80–95. The factory special case constructsBusTubesNull, but existing code still seesisVirtual() == true;BusManager::on()then statically casts it toBusNetwork, which is an invalid downcast and can access the wrong object layout during hostname resolution. - On a fresh or factory-reset device, WLED initializes its default bus before usermod
setup(). Because the S3 environment sets onlyPIXEL_COUNTS=60, that initial bus is a physicalTYPE_WS2812_RGBbus.recoverLedBussesIfNeeded()immediately returns when it sees the existing bus, while the AMOLED preview accepts onlyTYPE_TUBES_NULL, so the intended null framebuffer is never installed and the preview remains black. - The generation-1 visual marker and the broader invalid-segment recovery are also removed from
Tubes.h, changing current mixed-fleet and recovery behavior without being required by the S3 target.
Please review the resulting diff for minimal impact. Ideally, this PR should contain the Waveshare environment, board dependencies and partition layout, the display usermod, and the smallest core change needed for a safely classified memory-backed output. Existing Tubes role, protocol, palette, JSON, migration, and non-S3 board behavior should remain unchanged.
The current tests and builds pass because most of the new tests inspect source text or compile-time contracts; they do not execute first-boot bus selection, a power off/on cycle through BusManager::on(), HomeLight Wi-Fi rendering, or v3 literal-palette rendering. Please add focused behavioral coverage for those paths once the shared-code regressions are removed.
|
Addressed the requested changes with a narrow additive correction over the existing PR commit. What changed
Verification
No force-push; this is a normal fast-forward update. |
| void show() override {} | ||
| void setPixelColor(unsigned pix, uint32_t c) override; | ||
| uint32_t getPixelColor(unsigned pix) const override; | ||
| size_t getBusSize() const override { return sizeof(BusTubesNull) + _len * sizeof(uint32_t); } |
There was a problem hiding this comment.
Stepping back, do we need a new core bus type here at all? The simplest version may be to configure a normal 60-pixel LED bus on a confirmed-unused pin and just leave the strip disconnected. That gives WLED the logical geometry it needs without adding another bus implementation.
If the no-GPIO guarantee is important enough to keep the null bus, I do not think it needs its own pixel buffer. WLED already maintains the logical frame that strip.getPixelColor() reads, so this bus could provide the 60-pixel geometry and make output a no-op while the AMOLED reads from the existing strip buffer. That would remove _data, its allocation, and the duplicate set/get storage path.
There was a problem hiding this comment.
null bus still probably useful for the S3 since it has no wired output and GPIO is pretty tight. will simplify.
| -D TUBES_NULL_OUTPUT | ||
| -D TUBES_S3_FIELD_OS | ||
|
|
||
| -D PIXEL_COUNTS=60 |
There was a problem hiding this comment.
On a fresh configuration, this profile overrides the pixel count but still inherits the default LED type and pin, so cfg.cpp creates a 60-pixel WS2812 bus on GPIO 16. WLED calls beginStrip() before UsermodManager::setup(), which means that physical driver is initialized and sends a black frame before recoverLedBussesIfNeeded() gets a chance to replace it.
The later replacement fixes the AMOLED preview, but it does not make first boot free of physical output. If the null bus remains, could the environment make it the initial LED_TYPES value, with an appropriate sentinel DATA_PINS value, and reserve the recovery code for migrating stale configurations?
| }); | ||
|
|
||
| test('Waveshare effective environment has unique artifact identity, one enabled USB CDC definition, and immutable pins', () => { | ||
| const output = execFileSync('pio', ['project', 'config', '--json-output'], { |
There was a problem hiding this comment.
Could you have your agent fix the test-environment mismatch here? The existing testCdata CI job runs npm ci followed by npm test, without installing PlatformIO, so this currently fails there with spawnSync pio ENOENT.
My first choice would be to move this effective-PlatformIO assertion into a PlatformIO/firmware validation step, or skip it with a clear reason when pio is unavailable. Installing requirements.txt in the Node test job would also work, although there may be a cleaner way to keep the fast Node suite independent of the firmware toolchain.
SteveEisner
left a comment
SteveEisner
left a comment
There was a problem hiding this comment.
We've discussed the remaining points offline.
|
Updated PR71 from Steve main |
f1e4871 to
6c61509
Compare
theysayheygreg
changed the title
|
Updated PR71 to current v47 main. This is now the four-tile S3 baseline: canonical-frame Conductor, identity/topology Surveyor, read-only modern Channels, and an optional carrier build serving exact v47 Dig2Go/C3 images through the existing fleet-pull protocol. Both standard and carrier targets build; focused S3 contracts pass. Current hardware artifact is flashed and booting; latest UI visual acceptance is explicitly still pending. |
2 participants
Summary
Adds the Waveshare ESP32-S3-Touch-AMOLED-2.16 as a Tubes v47 handheld node, rebased directly on current
main(d21b3850). The S3 runs Steve’s existing Tubes engine and modern Beat, Pattern, Palette, device-report, and fleet-update contracts.Included
Product boundaries
Verification
pio run -e waveshare_s3_tubes_remotepassespio run -e waveshare_s3_tubes_carrierpassesgit diff --checkpassesmainHardware checkpoint
Device: Waveshare S3, ROM MAC
a4:cb:8f:d7:66:f8.Current carrier artifact:
3,910,416bytes9dcacd3411daa9f02e600fb09bf4510417d7ecb0f5bf6111ebb47494fd17da980x10000; esptool verified the write hashThe previously accepted PR71 canonical-frame/null-bus hardware proof remains preserved in this history; this update does not replace compile proof with a physical claim.