Repository navigation
Conversation
|
@barryw Wow, that was fast! Nice one! |
|
@chrisgleissner I have a feeling that this is just the start of the conversation ;-) Once @GideonZ gets ahold of it, I'm pretty certain I'll have some work to do lol |
|
I commented elsewhere about the format. I think you picked that up, right, @barryw ? |
@GideonZ I did. I waited until you and @chrisgleissner chimed in before I started on anything, so what's here is based on that complete conversation. I'm working with @chrisgleissner on changes he's requested on his OBS plugin and that may require changes here. I'll let you guys know if this pr has to change. |
|
@GideonZ @chrisgleissner Follow-up from the client review is in 9da3085: palette UDP now binds to the same source address as the FPGA VIC stream, and the firmware E2E enforces that source filter. The agreed 60-byte compatibility layout is unchanged. All five shipping |
|
@GideonZ @chrisgleissner Hardware validation on a dual-homed Ultimate 64 Elite found that forcing the software palette socket to the FPGA stream address breaks unicast. The FPGA VIC stream left Ethernet as Follow-up |
|
@GideonZ @chrisgleissner Final hardware validation is complete on an Ultimate 64 Elite running
This confirms the corrected routing behavior: multicast remains source-bound, while unicast permits LwIP to use the working software interface. CI is green for the final commit. |
| } | ||
| stream_config_t *stream = &streams[streamID]; | ||
| stream->enable = 0; | ||
| if (streamID == 0) { |
There was a problem hiding this comment.
Setting a variable requires a critical section??
There was a problem hiding this comment.
No 😄. This was over-defensive. I added it to mirror the snapshot in sendVicPalette(), but a critical section around only this byte does not establish an atomic invariant while stream->enable is written outside it. The store is atomic on these targets, and the palette task can tolerate seeing the previous value for one iteration. I will remove this and the matching start-side wrapper.
There was a problem hiding this comment.
Fixed in 26d7af0: the single-variable critical sections are gone. What remains guards state that must change together; details in the PR conversation.
|
Hi @barryw and @GideonZ , can this be merged? As soon as it's merged, I'd like to also merge the nice PR that Barry kindly left against C64 Stream to fully support this exciting new feature. See chrisgleissner/c64stream#131 |
I've done a rebase and am running e2e now just to make sure we're still clean. We should be good, but this is not a trivial change. |
7949843 to
b3bd458
Compare
Firmware now sends runtime palette packets from the VIC stream's interface and source port for unicast as well as multicast (GideonZ/1541ultimate#871). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
b3bd458 to
26d7af0
Compare
|
@GideonZ @chrisgleissner I rebased this branch onto current On the critical sections @GideonZ asked about in Firmware
Two changes reach beyond the palette:
E2E test
Validation Hardware: Ultimate 64 Elite, FPGA 125, core 1.50. The firmware was built from
The gate runs against the wired address because the device has Ethernet connected, and the A/V streams leave from that port. The complete default-gate log follows in the next two comments, split only because of GitHub's comment size limit.
|
|
Default E2E gate, Default gate log, part 1 of 2 |
|
Default E2E gate, Default gate log, part 2 of 2 |
|
Thanks for having added the full default E2E gate logs @barryw , and all the other changes and fixes - much appreciated! Stellar work! Did you check with @GideonZ , is he OK to have this merged, and shall this go into 3.15b or 3.16? We currently don't have the means to distinguish both - everything that currently goes into |
|
Thanks @chrisgleissner. No, I haven't had an explicit go-ahead from @GideonZ yet. His one review point (the single-variable critical sections) was addressed in 26d7af0, and he hasn't looked again since. On compatibility: existing clients see the same packets. Palette packets are only sent after That leaves two options: merge now so it ships in 3.15b, or hold it until master opens for 3.16. Either way I can split the multicast fix into its own PR if you'd rather decide on that separately. My preference is 3.15b, since the client side is compatible, but that's a judgement call. @GideonZ, your call. |
|
I hope to be up and running again within some weeks, @barryw |
26d7af0 to
30afb19
Compare
|
@chrisgleissner Rebased onto master 2c60bd8; it's now 30afb19. The only conflict was in Retested on the rebased build (Ultimate 64 Elite, FPGA 125, core 1.50): default gate 31/31, and |
|
Default E2E gate, Default gate log, part 1 of 2 |
|
Default E2E gate, Default gate log, part 2 of 2 |
|
uci-targets log |
|
Added 72a4373, which tightens the tests. The missing-feature and multicast checks were run red first; the reuse check could not be made to fail on its own (see below).
After the change, Not covered: the Ultimate 64-II. |
|
Hi @barryw , thanks for fixing the merge conflict in this PR. |
Emit the active 16-color RGB palette through the software UDP stack on VIC stream start and runtime updates. Reserve line 0x7fff so the FPGA stream remains unchanged. Refs GideonZ#850
Protect legacy video receivers from short palette datagrams while allowing opted-in clients to follow runtime VIC colors. Refs GideonZ#850
Pair each generation with one atomic RGB snapshot and cap burst updates at one packet per 20 ms. Exercise 240 runtime changes while checking coalescing, packet contents, and uninterrupted video sequencing.
A conditional indefinite wait can miss a rapid stop/start transition and leave runtime palette delivery asleep. Retain the proven one-second poll while preserving notification coalescing.
Binding the palette socket to the FPGA wired address drops unicast packets when a dual-homed Ultimate routes software UDP through Wi-Fi. Keep multicast bound to the FPGA source and let LwIP select the unicast interface.
Firmware: - Resolve a stream start into a copy and commit the source, destination, enable and palette opt-in together, so a failed start leaves the running stream and its palette packets untouched. - Carry the opt-in as a mode bit instead of the command's filename, which the task menu fills with the name of the highlighted browser entry. - Validate the palette parameter before the debug stream is stopped, so a rejected request changes nothing. - Bind the palette socket to the VIC stream's interface and its source port 53248, so palette and video arrive from one address and port for unicast and multicast alike. Connect it to the destination, drain it, and close it when the stream stops, so datagrams sent to it cannot hold lwIP's netbufs. - Keep the socket across transient send failures and pace failed attempts. - Program the palette registers inside the same critical section as the shadow copy and generation, so the VIC, GET_PALETTE and the stream agree. - Treat all of 224.0.0.0/4 as multicast, not only 232-239. - Space palette packets at least 20 ms apart and leave the palette task asleep when no stream has opted in. - Document that the opt-in is device-wide and the latest start sets it. Tests: - Move the stream checks into their own palette-stream scenario after the command interface scenarios, skipping when the firmware, the link or the -H address cannot carry the stream. - Drain the stream on a thread, judge the video by stalls rather than host drops, and use video as the positive control for the no-palette checks. - Require prompt palette packets on start, change and reset, and check the repeat interval. - Start the burst fixture only on GO=1 and reset before restoring the palette when a failure leaves it parked. - Share one palette parser in streams.py and pass stream start parameters through Arming. Refs GideonZ#850
…low multicast The palette-stream scenario used the rejected palette value as its capability probe, so firmware that lost the feature reported a skip and the run stayed green. The machines that lack it are now declared in the fix table (vic-palette-stream: only the C64 Ultimate's release firmware), and everywhere else a refused palette parameter fails the scenario. Two checks cover what the scenario left out. Twenty opted-in starts and stops must each still deliver a palette packet; lwIP has eight UDP sockets, so a palette socket that is not closed runs the pool dry well within that. And a stream to 230.0.1.64 must arrive as multicast, which is the 224-231 range this PR moved from unicast.
72a4373 to
5dd971c
Compare
|
Rebased onto master a1a1f44 to clear the conflict. The only conflict was in Retested on the rebased build (Ultimate 64 Elite, FPGA 126, core 1.50): default gate 31/31, and |
|
Default E2E gate, Default gate log, part 1 of 2 |
|
Default E2E gate, Default gate log, part 2 of 2 |
|
uci-targets log |
VIC streams carry 4-bit color indices but no runtime palette, so clients cannot reproduce palette changes made by software such as Heartbeat. This adds an explicit
palette=1option toPUT /v1/streams/video:startand sends compact palette metadata to the active video destination without an FPGA change.Existing clients remain unchanged: palette packets are opt-in, use the agreed 60-byte video-compatible pseudo-header, and ordinary video packets retain their current format. Firmware sends an atomic RGB/generation snapshot, repeats it once per second for UDP recovery, and coalesces rapid writes to at most one packet per 25 ms (20 ms rounded up to the 5 ms tick, so at most 40 packets/s). The opt-in is device-wide: the most recent
video:startsets it.Palette packets leave through the VIC stream's interface (
SO_BINDTODEVICE) from its source port 53248, so palette and video arrive from one address and port for both unicast and multicast, including on a dual-homed Ultimate. The socket is connected to the destination, drained after each send, and closed when the stream stops.One change reaches beyond the palette: the existing multicast test in
data_streamer.ccmatched only 232-239.x.x.x and now covers 224.0.0.0/4, so video/audio streams to groups in 224-231 are sent as multicast instead of through the gateway. It can move to its own PR if preferred.Validation (Ultimate 64 Elite, FPGA 125, core 1.50):
uci-targetspalette9/9 andpalette-stream10/10 (full logs in the comments)Companion OBS plugin PR: chrisgleissner/c64stream#131
Refs #850