Axi4SubordinateMemoryAgent response-delay callbacks are dead code
- Dominant language
- Dart
- Stars
- 115
- Forks
- 39
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 1
Description
## Summary
`Axi4SubordinateMemoryAgent` (`lib/src/models/amba/amba4_bfm/axi4_memory_subordinate.dart`)
exposes two constructor callbacks intended to model delayed responses:
- `readResponseDelay` — "A function which delays the response for the given `request`."
- `writeResponseDelay` — same doc, for writes.
Both are stored and documented as functioning knobs, but the code that is
supposed to consult them is commented out in both response-handling
methods:
```dart
// TODO(kimmeljo): how to deal with delays??
// if (readResponseDelay != null) {
// final delayCycles = readResponseDelay!(packet);
// if (delayCycles > 0) {
// await sIntf.clk.waitCycles(delayCycles);
// }
// }
```
This block appears, commented out, in both `_respondRead()` and
`_respondWrite()`. As a result, `readResponseDelay`/`writeResponseDelay`
currently have **zero effect** on simulated timing — responses are always
pushed immediately regardless of what these callbacks return.
## Additional copy/paste bug
The commented-out block inside `_respondWrite()` itself references
`readResponseDelay` instead of `writeResponseDelay` — i.e. even a naive
uncomment of the write-side block would still consult the read delay
function rather than the write delay function.
## Impact
Any caller/test that supplies `readResponseDelay`/`writeResponseDelay` to
model backpressure or delayed-response timing silently gets baseline
(zero-delay) behavior instead. This is visible in
`test/amba/amba4/axi4_bfm_test.dart`'s `withRandomRspDelays: true` test
cases, which currently only re-validate ordinary read/write correctness
and do not exercise actual delayed-response timing, since the invocation
is dead code.
## Reproduction
Construct an `Axi4SubordinateMemoryAgent` with a non-null
`readResponseDelay` (or `writeResponseDelay`) that always returns a
non-zero delay, issue a request, and observe that the response is pushed
on the very next opportunity rather than after the requested number of
cycles.
## Suggested fix
Re-enable the delay logic in both `_respondRead()` and `_respondWrite()`,
fixing the copy/paste reference in the write path. Note that
`_respondRead()`/`_respondWrite()` are currently synchronous and invoked
without being awaited, once per lane per clock negedge from a single
sequential loop — naively making them `async` and directly awaiting the
delay (as in the commented-out code) would either reintroduce a
reentrancy issue (the same queue head could be reprocessed on the next
negedge while a previous call is still "sleeping") or serialize all
lanes/channels behind a single lane's delay if the call sites were
changed to await them. A per-channel delay countdown, decremented once
per existing per-cycle poll (rather than `await`ing a multi-cycle sleep
inline), avoids both issues while keeping each channel's delay
independent.
Contributor guide
Research direction
Start in lib/src/models/amba/amba4_bfm/axi4_memory_subordinate.dart, reading _respondRead(), _respondWrite(), and their per-lane clock-negedge call sites. Then run the withRandomRspDelays cases in test/amba/amba4/axi4_bfm_test.dart and inspect how response queues are polled. Done means both callbacks affect response timing independently without reprocessing a delayed queue head, and the tests verify non-zero read and write delays.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- dart
- Domain
- embedded-iot, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 65/100