intel / intel/rohd-hcl

Axi4SubordinateMemoryAgent response-delay callbacks are dead code

Open
#303 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.