Skip to content

fix(examples): give Streaming_CreateChannel a sample count, not a byte size - #511

Open
nicolas-rabault wants to merge 1 commit into
mainfrom
fix/example-ring-buffer-sample-count
Open

fix(examples): give Streaming_CreateChannel a sample count, not a byte size#511
nicolas-rabault wants to merge 1 commit into
mainfrom
fix/example-ring-buffer-sample-count

Conversation

@nicolas-rabault

Copy link
Copy Markdown
Member

Independent of #508 / #509 / #510 — touches only examples/projects/product/laser/, based directly on main. No conflicts with the other three.

The bug

Streaming_CreateChannel takes ring_buffer_size in samples:

stream.end_ring_buffer = (void *)ring_buffer + (stream.data_size * ring_buffer_size);

The laser and galvo services passed sizeof(stream_buf), which is a byte size:

// laser.c
ratio_t stream_buf[4096];                                             // 16384 bytes
stream = Streaming_CreateChannel(stream_buf, sizeof(stream_buf), sizeof(ratio_t));

// galvo.c
pos_2d_t stream_buf[4096];                                            // 16384 bytes
stream = Streaming_CreateChannel(stream_buf, sizeof(stream_buf), 2 * sizeof(uint16_t));

ratio_t is { float } and pos_2d_t is a packed { uint16_t, uint16_t }, so both are 4 bytes and both channels were told they had 4× the samples they really have.

Verified

Computed with the real type definitions:

sizeof(ratio_t)=4  sizeof(pos_2d_t)=4

laser BEFORE   buffer=16384 B  ring_buffer_size arg=16384  -> end_ring_buffer at +65536 B  (+49152 past buffer end)
laser AFTER    buffer=16384 B  ring_buffer_size arg= 4096  -> end_ring_buffer at +16384 B  (       exactly at end)
galvo BEFORE   buffer=16384 B  ring_buffer_size arg=16384  -> end_ring_buffer at +65536 B  (+49152 past buffer end)
galvo AFTER    buffer=16384 B  ring_buffer_size arg= 4096  -> end_ring_buffer at +16384 B  (       exactly at end)

Each channel believed it owned 48 KB of memory belonging to something else, and that it held 16384 samples instead of 4096.

Why streaming.c cannot save you here

This is worth stating plainly, because it is the reason this is more serious than it looks: every bound streaming.c checks is derived from end_ring_buffer. Once that pointer is wrong, the capacity guard agrees with it and reports the channel as having plenty of room. The Streaming_PutSample fix in #508 does not help this case at all — it makes the guard self-consistent, not the declaration correct.

Concretely, on laser.c's non-streaming path:

Streaming_AddAvailableSampleNB(&stream, size / stream.data_size);

that guard compares against a 16384-sample capacity, so it will happily accept a request that walks 48 KB out of bounds.

The fix

Derive both arguments from the buffer itself:

stream = Streaming_CreateChannel(stream_buf,
                                 sizeof(stream_buf) / sizeof(stream_buf[0]),
                                 sizeof(stream_buf[0]));

Two arguments that must agree are now both computed from the same declaration, so they cannot drift, and the element type can change without anyone having to remember this line. For galvo this also replaces the hand-written 2 * sizeof(uint16_t) with the element size it was standing in for — same value, sizeof(pos_2d_t) == 4.

Tests

There is no test harness for the product examples, and neither project is built by CI. I built it by hand instead:

$ pio run          # examples/projects/product/laser, env nucleo_l476rg
Linking .pio/build/nucleo_l476rg/firmware.elf
RAM:   [===       ]  32.1% (used 31556 bytes from 98304 bytes)
Flash: [          ]   4.0% (used 41984 bytes from 1048576 bytes)
========================= [SUCCESS] Took 26.03 seconds =========================

That project compiles both laser.c and galvo.c, so both changes are covered.

Not verified: I have no laser/galvo hardware, so I have not observed the corruption or confirmed the fix on a real board. The numbers above are arithmetic on the real type sizes, and the build is real; the runtime behaviour is inferred.

Found while sweeping for the bug in #508

The engine-side bug in #508 was a sample-vs-byte mix-up in Streaming_PutSample. Sweeping the callers for the same confusion turned these up. The other Streaming_CreateChannel callers are correct: pipe.c uses data_size = 1 where the two units coincide, and the motor examples (motor.cpp, dxl.c, controller_motor.c) all pass genuine sample counts.

…e size

Streaming_CreateChannel takes ring_buffer_size in samples, it computes
end_ring_buffer as ring_buffer + data_size * ring_buffer_size. The laser and
galvo services passed sizeof(stream_buf), which is a byte size, so the
computed end of the ring landed data_size times too far.

Both buffers hold 4096 elements of 4 bytes. Passing 16384 instead of 4096
put end_ring_buffer 65536 bytes from the start of a 16384 byte buffer, so
the channel believed it owned 48 KB of memory that belongs to something
else, and believed it held 16384 samples instead of 4096.

Nothing in streaming.c can catch this. Every bound it checks is derived from
end_ring_buffer, so once that is wrong the capacity guard agrees with it.

Derive both arguments from the buffer itself so the two cannot drift apart
again, and so the element type can change without touching this line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant