Packet Symmetry Refactor with More Sparse Dispatch Sink #50
Conversation
|
There's
We also have a lot of hamfisted comm timing logic in here, but that should get better with a known processor input buffer depth The biggest letdown is maybe that, for the DBSCAN tests, there's no performance boost. I used a "timeline" debugging function to investigate, and the reality is that there's rarely a period past the first dozen timesteps with more than a 1-timestep gap between any output neuron firing. At the end of the day, we're not going to get better performance than the network will let us, regardless of what schema the processor is trying to use. |
WalkthroughThe changes span multiple components of the FPGA and neuroprocessor code. In the Python code, new configuration classes (_IoConfig, InpConfig, OutConfig) have been introduced to encapsulate input and output settings, and method signatures have been updated to align with these new abstractions. Several enums have been modified by renaming and adding new values. In the FPGA constraints file, the configuration mode is now explicitly set to SPIx4. The SystemVerilog files have undergone extensive updates: local parameters and signal names have been standardized, package dependencies and imports have been reorganized, and several modules including dispatch_sink, dispatch_source, axis_processor, basic_processor, stream_source, and stream_sink now adopt new signal naming, parameter definitions, and control logic. A key modification is the re-design of the Dispatch Sink state machine to reduce communication verbosity by using opcodes such as RUN and SPK. Sequence Diagram(s)sequenceDiagram
participant DS as Dispatch Source
participant DSink as Dispatch Sink
DS->>DSink: Transmit packet with opcode (RUN/SPK)
alt Opcode is RUN
DSink->>DSink: Transition to RUNS state
DSink->>DSink: Update run counter (sparse time sync)
else Opcode is SPK
DSink->>DSink: Transition to SPKS state
DSink->>DSink: Process firing neuron ID
end
DSink->>DSink: Transition through IDLE, CLRD, and SYNC states as needed
Assessment against linked issues
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (14)
fpga/rtl/stream_source.sv (3)
14-17: Expose modules selectively to reduce namespace bloat.Exporting everything with
export *::*;might unnecessarily pollute the namespace if this package is imported in multiple places. Consider explicitly exporting only the required symbols to improve maintainability.
22-22: Consider specifying a default forPKT_WIDTH.If there's a commonly used or recommended packet width, setting a default might simplify instantiation in cases that don't require a custom width.
65-65: Review handshake condition fornet_en.
net_enis driven bysrc_valid && net_ready, so it’s asserted whenever downstream is ready and source is valid. If additional gating logic is needed (e.g., non-zero run counters in a related module), consider adding checks here to avoid partial-cycle enables.fpga/rtl/stream_sink.sv (2)
14-17: Limit exported items to essential symbols.Using
export *::*;insink_configmay expose more than necessary. For clarity, export only those parameters/types actually required by external modules.
21-23: Suggest default forPKT_WIDTHinnetwork_sink.Providing a sensible default for
PKT_WIDTHcan simplify instantiation and help avoid accidental mismatches.fpga/rtl/dispatch_source.sv (2)
23-23: ExposePKT_WIDTHdefault if practical.Defining a default
PKT_WIDTHcan reduce manual setup across modules and mitigate mismatch errors, especially if there's a commonly used width.
67-75: Potential indefinitenet_syncifnet_readynever asserts.
net_syncis set onSNCand cleared onnet_ready. Ifnet_readyis stuck low,net_syncremains high indefinitely. Verify that upstream logic can handle this scenario or consider a failsafe timeout.fpga/rtl/dispatch_sink.sv (2)
43-44: Consider aunique caseor default forstate_t
Using aunique casestatement or providing adefault:branch within the state machine can prevent unforeseen state encodings from causing undefined behavior, especially if states are extended in the future.
121-145: Keep the combinational output block concise
Thecalc_snkblock covers multiple states with a wide case statement. Consider factoring out repeated logic or using helper functions (in a separate module or package) to keep this section more readable and maintainable.fpga/_processor.py (3)
329-333: Avoid Yoda conditions for clarity
Rewrite"out_idx" == self._out.spk_fmt._infos[1].nametoself._out.spk_fmt._infos[1].name == "out_idx".-if len(self._out.spk_fmt._infos) > 1 and "out_idx" == self._out.spk_fmt._infos[1].name +if len(self._out.spk_fmt._infos) > 1 and self._out.spk_fmt._infos[1].name == "out_idx"🧰 Tools
🪛 Ruff (0.8.2)
331-331: Yoda condition detected
Rewrite as
self._out.spk_fmt._infos[1].name == "out_idx"(SIM300)
368-369: Document the newsyncparameter
With the updated signature of_hw_tx, you introduced asyncparameter. Provide a docstring or inline comment describing its behavior when set to True vs. False, and how it interacts withSNC.
578-579: Monitor potential performance bottleneck
max_bytes_per_runmultiplied by a factor of 10 for baud-based delay can lead to slower runs at higher widths. If largespk_fmtexpansions are expected, consider a dynamic approach or run-time configuration to reduce overhead.fpga/rtl/io_configs.sv (1)
33-40: Unify naming schemes for stream flags
flag_tdefinesSNC,CLR, andNUM_FLG. Using a placeholder likeNUM_FLGfor enumeration sizing can be confusing if new flags are added later. Consider using a separate constant or more descriptive naming.fpga/network.py (1)
99-99: Remove extraneous f-string prefixes.Several strings are prefixed with
fbut don't contain any placeholders.Apply this diff to fix the issue:
- f.write(f"endpackage\n\n") + f.write("endpackage\n\n") - f.write(f" input logic clk,\n") + f.write(" input logic clk,\n") - f.write(f" input logic arstn,\n") + f.write(" input logic arstn,\n") - f.write(f" input logic en,\n") + f.write(" input logic en,\n") - f.write(f");\n") + f.write(");\n") - f.write(f" .CHARGE_WIDTH(CHARGE_WIDTH),\n") + f.write(" .CHARGE_WIDTH(CHARGE_WIDTH),\n") - f.write(f" .CHARGE_WIDTH(CHARGE_WIDTH),\n") + f.write(" .CHARGE_WIDTH(CHARGE_WIDTH),\n")Also applies to: 102-104, 110-110, 193-193, 235-235
🧰 Tools
🪛 Ruff (0.8.2)
99-99: f-string without any placeholders
Remove extraneous
fprefix(F541)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
fpga/_processor.py(9 hunks)fpga/config/basys3/basys3.xdc(1 hunks)fpga/network.py(5 hunks)fpga/rtl/axis_loop_proc.sv(1 hunks)fpga/rtl/axis_processor.sv(2 hunks)fpga/rtl/basic_processor.sv(2 hunks)fpga/rtl/dispatch_sink.sv(2 hunks)fpga/rtl/dispatch_source.sv(3 hunks)fpga/rtl/io_configs.sv(1 hunks)fpga/rtl/macros.svh(0 hunks)fpga/rtl/stream_sink.sv(2 hunks)fpga/rtl/stream_source.sv(3 hunks)fpga/rtl/uart_processor.sv(1 hunks)
💤 Files with no reviewable changes (1)
- fpga/rtl/macros.svh
✅ Files skipped from review due to trivial changes (2)
- fpga/rtl/uart_processor.sv
- fpga/config/basys3/basys3.xdc
🧰 Additional context used
🪛 Ruff (0.8.2)
fpga/network.py
99-99: f-string without any placeholders
Remove extraneous f prefix
(F541)
102-102: f-string without any placeholders
Remove extraneous f prefix
(F541)
103-103: f-string without any placeholders
Remove extraneous f prefix
(F541)
104-104: f-string without any placeholders
Remove extraneous f prefix
(F541)
110-110: f-string without any placeholders
Remove extraneous f prefix
(F541)
193-193: f-string without any placeholders
Remove extraneous f prefix
(F541)
235-235: f-string without any placeholders
Remove extraneous f prefix
(F541)
fpga/_processor.py
331-331: Yoda condition detected
Rewrite as self._out.spk_fmt._infos[1].name == "out_idx"
(SIM300)
🔇 Additional comments (18)
fpga/rtl/stream_source.sv (2)
49-52: Ensurerst_pandrst_nusage aligns with design intent.The reset logic triggers on
src[PKT_WIDTH - CLR - 1]. IfCLRandPKT_WIDTHaren’t sized consistently, it could inadvertently cause perpetual resets or missed resets. Double-check that this signal is only asserted when intended and that the delay scheme forrst_nmeets your clocking requirements.
68-69: Protect against array indexing issues innet_inpassignment.The slice operation for
src[...]must remain valid. Confirm thatPFX_WIDTH + i*CHARGE_WIDTHdoesn’t exceedPKT_WIDTH. A compile-time check helps detect out-of-range slicing during synthesis.fpga/rtl/stream_sink.sv (3)
31-32: Check reset design withnet_arstn.You’re capturing
net_arstnon a negative clock edge, which might introduce partial-cycle resets or race conditions if external logic expects synchronous reset only. Confirm that this asynchronous approach is intentional and tested.
44-44: Validate handshake match forsnk_valid.
snk_validis tied directly tonet_en. Ensure that the downstream logic and the sink’s readiness conditions are consistent, so data isn’t lost or stalled for one cycle afternet_engoes high.
73-79: Check bit offsets in thecalc_snkblock.While setting
snk[(PKT_WIDTH - SNC - 1)]andsnk[(PKT_WIDTH - CLR - 1)], confirm thatSNCandCLRremain within valid ranges ofPKT_WIDTH. Misalignment could lead to over/underflow or unintended results in the output packet.fpga/rtl/dispatch_source.sv (3)
14-19: EnsurePFX_WIDTHandSPK_WIDTHalignment with opcode definitions.Using
$clog2(NUM_OPC)forPFX_WIDTHcan be susceptible to off-by-one errors ifNUM_OPCchanges. Double-checkSPK_WIDTHfor potential boundary issues ifNUM_INPis small or large.
51-53: Confirmrun_counteris never overrun or underflowed.
run_counteris only reduced whennet_enis high. Ensure that overcount or underflow can’t happen if theRUNopcode sets a large value or if handshake signals remain active longer than expected.
77-85: Check asynchronous reset usage fornet_arstn.The logic sets
net_arstnto 0 when receivingCLR, then back to 1 otherwise. In an asynchronous design, abrupt resets may lead to timing uncertainty. Ensure this pattern is fully synchronized or only used in known-safe contexts.fpga/rtl/dispatch_sink.sv (3)
16-19: Validate parameter handling for future expansions
The local parametersPFX_WIDTHandSPK_WIDTHdirectly hinge onNUM_OPCandNUM_OUT. Any future increase in opcode count or output lines could break dimension assumptions.Would you like a script to search the codebase for any usage of these parameters to catch possible mismatches?
50-57: Recheck asynchronous reset logic
This logic setsrstwhennet_arstnis deasserted, but relies on theCLRDstate transition to clear. Ensure that partial resets or bouncing ofnet_arstncan’t cause latch-like behavior or incomplete resets.
95-104: Double-check off-by-one scenarios infire_counter
fire_counteris loaded withNUM_OUTand decremented each timesnk_readyinSPKSstate. Verify that indexing withNUM_OUT - fire_countercan’t exceed array bounds whenfire_counteris 0.fpga/_processor.py (2)
68-70: Ensure code references for newDispatchOpcode.SNC
You introducedSNCinDispatchOpcode. Verify that existing call sites, especially for synchronization or flush logic, are updated to handle this new opcode.
74-75: Check consistency of default values inStreamFlag
SNC = 0andCLRis auto-assigned. Confirm that the enumerated numeric values match your downstream usage (e.g., bitfields, indexing) so that stream flags remain consistent across modules.fpga/rtl/io_configs.sv (1)
11-25: Clarify usage ofNUM_OPCindispatch_config
NUM_OPC = 4while theopcode_tenum has 8 possible named values (0throughR07). If only some rely on 2-bit codes, consider separating the two sets or providing inline documentation clarifying partial usage.fpga/rtl/axis_loop_proc.sv (1)
19-31: LGTM! Parameter scoping has been improved.The changes improve clarity by explicitly qualifying the width parameters with the
processor_confignamespace, making the source of these parameters more apparent.fpga/rtl/basic_processor.sv (1)
11-14: LGTM! Parameter definitions have been improved.The width parameters are now clearly defined based on source and sink configurations, making the code more maintainable.
fpga/rtl/axis_processor.sv (1)
11-19: LGTM! Parameter organization has been improved.The changes enhance clarity by:
- Importing network configuration explicitly
- Defining packet width parameters based on source and sink configurations
- Using the
width_nearest_bytemacro for proper byte alignmentfpga/network.py (1)
96-98: LGTM! Parameter names have been improved.The changes enhance clarity by using more concise and descriptive parameter names:
NET_CHARGE_WIDTH→CHARGE_WIDTHNET_NUM_INP→NUM_INPNET_NUM_OUT→NUM_OUT
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
fpga/rtl/dispatch_source.sv (1)
98-111:⚠️ Potential issueAddress potential data loss during clear operations.
The concurrency issue from the previous review still exists. When
net_enorCLRoperations occur in the same cycle as a validSPKoperation, the spike data could be lost.Consider this fix:
always_ff @(posedge clk or negedge arstn) begin: set_net_inp if (arstn == 0) begin for (int i = 0; i < NUM_INP; i++) net_inp[i] <= 0; end else begin - if (net_en || (src_valid && src_ready && op == CLR)) begin + if ((net_en && !(src_valid && src_ready && op == SPK)) || + (src_valid && src_ready && op == CLR)) begin for (int i = 0; i < NUM_INP; i++) net_inp[i] <= 0; end if (src_valid && src_ready && op == SPK) begin net_inp[inp_idx] <= inp_val; end end end
🧹 Nitpick comments (2)
fpga/rtl/dispatch_source.sv (2)
14-14: Consider explicit exports instead of wildcard.Using
export *::*can lead to name conflicts and make the code harder to maintain. Consider explicitly listing the symbols you want to export.- export *::*; + // Export only required symbols + export opcode_t; + export PFX_WIDTH; + export SPK_WIDTH;
67-85: Consider adding safety checks for sync and reset sequences.The sync and reset logic looks clean, but consider adding checks to prevent invalid operation sequences (e.g., SNC during active run).
// Add these safety checks assert property (@(posedge clk) (net_sync && src_valid && op == SNC) |-> run_counter == 0); assert property (@(posedge clk) $rose(net_arstn) |-> !net_sync);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fpga/rtl/basic_processor.sv(2 hunks)fpga/rtl/dispatch_source.sv(3 hunks)
🔇 Additional comments (7)
fpga/rtl/basic_processor.sv (4)
1-10: LGTM! Copyright and license information updated.The copyright year has been correctly updated to include 2025, and the CERN-OHL-W v2 license information is properly included.
22-28: LGTM! Module interface updated to use new configuration parameters.The changes align with the broader restructuring:
- Input signal renamed from 'instr' to 'inp' for consistency
- Width parameters now use the centralized processor_config package
31-61: Verify network synchronization with updated signal connections.The changes to network_source, network, and network_sink modules look good:
- Parameter updates align with the new configuration approach
- Signal connections provide better synchronization control
✅ Verification successful
Network synchronization confirmed with updated signal connections.
The search results show consistent usage of the handshake signals (net_sync, net_arstn, net_en) across the network modules, including in the basic_processor, which confirms that the parameter updates and wiring changes for network_source, network, and network_sink are correctly implemented.
- Signal connections in basic_processor.sv match those in other network modules.
- Parameter updates (e.g., using PKT_WIDTH) are consistently applied.
- Synchronization control between modules appears as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent signal usage across network modules # Search for net_en, net_arstn, and net_sync usage rg "net_(en|arstn|sync)" --type sv -A 2 -B 2Length of output: 11689
13-16: Verify source_config and sink_config dependencies.The parameter calculations look correct, using the
maxmacro to ensure minimum width of 1. However, let's verify the dependencies are properly included.✅ Verification successful
Source and sink configuration dependencies verified.
The
source_configandsink_configpackages are defined in the repository (in files such asfpga/rtl/stream_sink.sv,fpga/rtl/dispatch_sink.sv,fpga/rtl/stream_source.sv, andfpga/rtl/dispatch_source.sv), which confirms that the parameters inprocessor_configare referencing valid dependencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for source_config and sink_config dependencies # Search for source_config and sink_config definitions rg -l "package\s+(source|sink)_config" --type svLength of output: 154
fpga/rtl/dispatch_source.sv (3)
22-40: LGTM! Parameter and signal changes align with the new dispatch protocol.The changes maintain type safety and port width consistency while improving signal naming clarity (e.g., net_valid → net_sync).
50-65: Verify run counter synchronization.There's a potential race condition when transitioning between operations. When
run_counteris 1 and bothsrc_validandnet_readyare high, the counter could be both decremented and loaded with a new value in the same cycle.// Add this assertion to catch potential glitches assert property (@(posedge clk) (run_counter == 1 && net_ready && src_valid && op == RUN) |=> $stable(net_en));
46-48: Verify opcode bounds.The operation extraction lacks bounds checking. Consider adding assertions to verify that the extracted opcode is within valid range.
// Add these assertions assert property (@(posedge clk) src_valid |-> op inside {RUN, SPK, SNC, CLR});
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
fpga/_processor.py (4)
79-87: Consider adding parameter validation.The function has been updated to include
opc_width, but there's no validation for negative or zero values in the parameters.def dispatch_operand_widths( opc_width: int, net_num_io: int, net_charge_width: int, is_axi: bool = True ) -> tuple[int, int]: + if opc_width < 1: + raise ValueError("opc_width must be positive") + if net_num_io < 1: + raise ValueError("net_num_io must be positive") + if net_charge_width < 0: + raise ValueError("net_charge_width cannot be negative") idx_width = unsigned_width(net_num_io - 1) spk_width = idx_width + net_charge_width
197-209: Consider adding bounds checking for spike values.The
apply_spikemethod should validate spike values to ensure they're within acceptable ranges before adding them to the queue.def apply_spike(self, spike: neuro.Spike) -> None: if spike.time < 0: raise RuntimeError("Spikes cannot be scheduled in the past.") + if not isinstance(spike.value, (int, float)): + raise ValueError("Spike value must be a number") + if not -1 <= spike.value <= 1: + raise ValueError("Spike value must be between -1 and 1") self._inp.queue.append( neuro.Spike(spike.id, spike.time + self._inp.time, spike.value) )
272-298: Consider adding progress tracking for long-running operations.The
runmethod could benefit from progress tracking, especially for long-running operations.def run(self, time: int) -> None: if time < 1: raise ValueError("It's not possible to run for less than 1 timestep") target_time = self._inp.time + time + total_steps = target_time - self._inp.time + steps_completed = 0 rx_thread = Thread(target=self._hw_rx, args=(target_time,)) rx_thread.daemon = True rx_thread.start() self._last_run = self._inp.time while self._inp.time < target_time: + steps_completed = self._inp.time - (target_time - total_steps) + if steps_completed % 100 == 0: # Update every 100 steps + progress = (steps_completed / total_steps) * 100 + print(f"Progress: {progress:.1f}%")
379-380: Consider replacing magic sleep values with configuration.The TODO comment indicates that magic timing will be resolved, but for now, consider making the sleep duration configurable.
-# TODO: magic timing will be resolved by buffers PR +# TODO: Replace with buffer-based timing +SLEEP_DURATION = 100e-9 # Configure in __init__ or class variable def pause(runs: int) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fpga/_processor.py(9 hunks)
🧰 Additional context used
📓 Learnings (1)
fpga/_processor.py (1)
Learnt from: keegandent
PR: TENNLab-UTK/fpga#50
File: fpga/_processor.py:79-88
Timestamp: 2025-02-03T22:18:36.244Z
Learning: In the `dispatch_operand_widths` function, a zero width from `unsigned_width(net_num_io - 1)` is acceptable as there is logic in place to implicitly associate commands with index 0 when appropriate.
🔇 Additional comments (4)
fpga/_processor.py (4)
67-71: LGTM! Added SNC opcode for synchronization.The addition of the
SNCopcode toDispatchOpcodealigns with the PR objectives of improving communication timing logic.
74-77: LGTM! Renamed StreamOpcode to StreamFlag.The rename from
StreamOpcodetoStreamFlagbetter reflects its usage as a set of flags rather than opcodes.
90-163: LGTM! Well-structured IO configuration classes.The new configuration classes
_IoConfig,InpConfig, andOutConfigprovide a clean separation of concerns and encapsulate IO-specific logic. The inheritance hierarchy is appropriate, and the implementation follows the Single Responsibility Principle.
605-625: LGTM! Clean IO setup implementation.The
_setup_iomethod provides a clean implementation for configuring input and output types. The error messages are descriptive and helpful.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fpga/_processor.py (1)
288-291: Consider documenting the timing magic numbers.Multiple TODO comments mention that "magic timing will be resolved by buffers PR". Until then, consider documenting:
- Why 100e-9 was chosen as the sleep duration
- What conditions influence the timing
Also applies to: 382-383
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fpga/_processor.py(9 hunks)
🧰 Additional context used
📓 Learnings (1)
fpga/_processor.py (1)
Learnt from: keegandent
PR: TENNLab-UTK/fpga#50
File: fpga/_processor.py:79-88
Timestamp: 2025-02-03T22:18:36.244Z
Learning: In the `dispatch_operand_widths` function, a zero width from `unsigned_width(net_num_io - 1)` is acceptable as there is logic in place to implicitly associate commands with index 0 when appropriate.
🔇 Additional comments (7)
fpga/_processor.py (7)
67-71: LGTM! Added sync opcode for synchronization.The addition of the
SNCopcode toDispatchOpcodealigns with the synchronization functionality implementation.
74-77: LGTM! Renamed StreamOpcode to StreamFlag.The rename to
StreamFlagbetter reflects its usage as flags rather than opcodes.
79-88: LGTM! Updated dispatch_operand_widths signature.The function now accepts
opc_widthparameter to calculate operand width based on opcode size.
90-130: LGTM! Well-structured I/O configuration base class.The
_IoConfigclass effectively encapsulates common I/O configuration logic with type-specific format string generation.
147-148: Address TODOs for input/output support.The TODOs indicate missing support for fires input and charges output functionality.
Would you like me to help implement the fires input and charges output support or create issues to track these tasks?
Also applies to: 161-162
359-368: LGTM! Robust synchronization error handling.The implementation includes comprehensive error checking for synchronization flags and timing mismatches.
608-628: LGTM! Clear I/O type validation and setup.The
_setup_iomethod effectively validates and initializes I/O configurations with clear error messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fpga/scripts/packet_vis.py (2)
53-54: Consider avoiding naming-based checks
Using “flag” in the type’s name to identify flags can lead to confusion or breakage if the name changes. Prefer a clearer method (e.g., a subclass check) to ensure consistency.Here's an example diff to switch to an attribute-based check if StreamFlag is declared as a subclass:
-def is_flag(pfx_t: type): - return "flag" in pfx_t.__name__.lower() +def is_flag(pfx_t: type): + return issubclass(pfx_t, StreamFlag)
816-843: Converting sink operand
Synchronized with update_operand(...). The code reuse strongly indicates potential for factoring out shared logic, but the current approach is understandable.Here is a minor suggestion: you might create a single consolidated callback with an additional parameter specifying “source” or “sink,” thereby reducing code duplication in the long run.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fpga/scripts/packet_vis.py(10 hunks)
🔇 Additional comments (15)
fpga/scripts/packet_vis.py (15)
34-34: Import usage looks good
The new import aligns well with the updated logic referencing DispatchOpcode, IoType, StreamFlag, and dispatch_operand_widths. No immediate issues found.
42-50: Match statement correctly enumerates I/O types
The match statement appropriately returns DispatchOpcode or StreamFlag based on the IoType. However, be aware that any future IoType members (e.g., UART) will trigger a ValueError. If additional types are expected later, consider documenting or handling them.
57-58: Correct bitwidth calculation
Using len(pfx_t) - 1 for enumerations and len(pfx_t) for flags appears logically sound, assuming enumerations are contiguous and start at zero. Just ensure any future enum additions remain within these assumptions.
61-62: Potential AttributeError on invalid prefixes
If a user passes a pfx_str that doesn’t map to an enum name, getattr(...) will raise an AttributeError. Consider validation or error handling for unknown strings.
322-357: Dynamic table generation is well implemented
This UI layout logic effectively distinguishes between flag-based (multiple toggles) and opcode-based (single select) prefixes. The approach is readable and looks maintainable.
367-381: Toggle handling for opcode
Switching between updating via bit toggles or a select control is a neat approach. Be mindful of out-of-range toggles if the enum doesn’t include all possible bit values.
383-592: Consolidated operand table generation
Splitting behavior by DispatchOpcode vs. Stream logic is clear. The subcases (SNC, CLR, SPK, RUN) demonstrate good separation of concerns. Future expansions to DispatchOpcode will require corresponding case branches here.
594-696: Operand<->bit synchronization
This callback adeptly translates UI bit switches into numeric operands and vice versa. Ensure the underlying enumerations remain contiguous so that bools_to_unsigned(...) and signed_to_bools(...) remain valid.
698-704: Source prefix table callback
Straightforward delegate to update_prefix_table. No issues spotted.
706-715: Linking source opcode
This callback properly calls update_opcode(...) to keep the UI consistent. Implementation looks clean.
717-739: Source operand table generation
By deferring to update_operand_table, the logic remains DRY. The approach ensures consistent UI updates whenever the source type or bit switches change.
741-771: Converting source operand
The function calls update_operand(...) to handle numeric vs. bit-based states. The separation of concerns is well done.
773-779: Sink prefix table callback
Analogous to the source prefix table logic. Keeps the codebase uniform.
782-790: Linking sink opcode
Mirrors the source-oriented structure. Straightforward usage of update_opcode(...) ensures consistency.
792-813: Sink operand table generation
Again parallels the source approach while focusing on the sink’s numeric outputs. The code is consistent and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
fpga/rtl/stream_source.sv (1)
44-70: Add descriptive comments for signal purposes.While the logic is correct, consider adding comments to explain:
- The purpose and timing requirements of the
net_syncsignal- The conditions and impact of the
rst_passertion- assign net_sync = src_valid ? src[PKT_WIDTH - SNC - 1] : 0; + // Extract sync bit from source packet when valid, used for network synchronization + assign net_sync = src_valid ? src[PKT_WIDTH - SNC - 1] : 0; - // rst_p asserted for one clock at positive edge when CLR + // rst_p asserted for one clock cycle when CLR bit is set in the source packet + // Used to initiate network reset sequence assign rst_p = src_valid && net_ready && src[PKT_WIDTH - CLR - 1];fpga/rtl/stream_sink.sv (1)
46-58: Consider adding timeout protection for reset handling.While the reset logic is well-structured, it might benefit from a timeout mechanism to prevent potential deadlock if snk_valid or snk_ready get stuck.
always_ff @(negedge clk or negedge arstn) begin: set_rst if (arstn == 0) begin rst_n <= 0; end else begin if (net_arstn == 0) rst_n <= 1; else if (snk_valid && snk_ready) // clear reported rst_n <= 0; + else if (rst_n) begin + // Add timeout counter to prevent deadlock + if (timeout_counter == TIMEOUT_MAX) + rst_n <= 0; + else + timeout_counter <= timeout_counter + 1; + end end endfpga/rtl/dispatch_source.sv (1)
98-111: Consider adding assertion for input validation.While the input handling is correct, adding assertions would help catch potential issues during simulation.
always_ff @(posedge clk or negedge arstn) begin: set_net_inp + // Assert valid input range + `ifndef SYNTHESIS + assert property (@(posedge clk) disable iff (!arstn) + (src_valid && src_ready && op == SPK) |-> (inp_idx < NUM_INP)); + `endif if (arstn == 0) beginfpga/rtl/dispatch_sink.sv (2)
147-195: Consider adding default state transition.While the state machine is well-designed, adding a default state transition would improve robustness.
always_comb begin: calc_next_state next_state = curr_state; case (curr_state) // ... existing states ... + default: begin + next_state = IDLE; + end endcase end
82-93: Consider adding coverage for fire transitions.While the fire handling logic is correct, adding coverage points would help verify all transitions are tested.
always_ff @(posedge clk or negedge arstn) begin: set_fires + // Add coverage for fire transitions + `ifndef SYNTHESIS + cover property (@(posedge clk) disable iff (!arstn) + (net_en && |net_out)); + cover property (@(posedge clk) disable iff (!arstn) + (curr_state == SPKS && next_state != SPKS)); + `endif if (arstn == 0) begin
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fpga/rtl/dispatch_sink.sv(2 hunks)fpga/rtl/dispatch_source.sv(3 hunks)fpga/rtl/io_configs.sv(1 hunks)fpga/rtl/stream_sink.sv(2 hunks)fpga/rtl/stream_source.sv(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fpga/rtl/io_configs.sv
🧰 Additional context used
📓 Learnings (2)
fpga/rtl/stream_source.sv (1)
Learnt from: keegandent
PR: TENNLab-UTK/fpga#50
File: fpga/rtl/stream_source.sv:45-45
Timestamp: 2025-02-03T22:21:24.741Z
Learning: In the FPGA project's RTL modules, parameter validation (like ensuring bit extraction indices are valid) is handled at higher abstraction layers rather than in low-level modules. The hardware synthesizer catches invalid bit selections automatically, making additional checks redundant at the lower levels.
fpga/rtl/dispatch_source.sv (1)
Learnt from: keegandent
PR: TENNLab-UTK/fpga#50
File: fpga/rtl/dispatch_source.sv:0-0
Timestamp: 2025-02-03T22:16:54.610Z
Learning: In SystemVerilog network processors, when both network execution (`net_en`) and spike loading (`SPK`) could occur in the same cycle, the execution takes precedence and clears all inputs. New spikes are only loaded when `net_en` is false, preventing race conditions.
🔇 Additional comments (6)
fpga/rtl/stream_source.sv (2)
1-18: LGTM! Package changes enhance modularity.The updated package configuration with imported stream configurations and derived parameters improves code organization.
20-39: LGTM! Interface changes improve consistency.The module interface changes standardize signal naming and leverage centralized configuration parameters effectively.
fpga/rtl/stream_sink.sv (2)
16-17: LGTM! Well-documented parameter definition.The comment clearly explains the importance of NUM_FLG for PFX_WIDTH calculation.
73-79: LGTM! Clean output calculation logic.The output calculation is well-structured with proper zero initialization and clear bit field assignments.
fpga/rtl/dispatch_source.sv (1)
51-53: LGTM! Proper handling of execution precedence.The implementation correctly prioritizes network execution over new dispatches, aligning with the expected behavior where execution takes precedence and clears all inputs.
fpga/rtl/dispatch_sink.sv (1)
43-44: LGTM! Clear state machine definition.The state enumeration is well-defined with logical state names.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fpga/_processor.py (1)
147-148: Address TODOs for input/output support.The TODO comments indicate missing support for:
- Fires input
- Charges output
Would you like me to help implement these features or create tracking issues?
Also applies to: 161-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fpga/_processor.py(9 hunks)
🧰 Additional context used
📓 Learnings (1)
fpga/_processor.py (1)
Learnt from: keegandent
PR: TENNLab-UTK/fpga#50
File: fpga/_processor.py:79-88
Timestamp: 2025-02-03T22:18:36.244Z
Learning: In the `dispatch_operand_widths` function, a zero width from `unsigned_width(net_num_io - 1)` is acceptable as there is logic in place to implicitly associate commands with index 0 when appropriate.
🔇 Additional comments (5)
fpga/_processor.py (5)
67-77: LGTM! Improved naming and added synchronization support.The rename from
StreamOpcodetoStreamFlagbetter reflects its usage, and the addition ofSNCopcode supports improved communication timing logic.
79-87: LGTM! Function signature updated correctly.The addition of
opc_widthparameter aligns with the changes in opcode handling, and the implementation correctly handles edge cases.
90-163: LGTM! Well-structured class hierarchy.The design follows SOLID principles with clear separation of concerns between base and derived classes.
385-386: Address magic timing technical debt.Multiple TODO comments indicate that timing logic needs to be resolved in a future buffers PR. Consider prioritizing this technical debt as it affects multiple critical sections.
Would you like me to help create a tracking issue for the timing logic improvements?
Also applies to: 292-294, 312-313, 317-318
414-422: LGTM! Robust synchronization implementation.The synchronization logic is well-implemented for both dispatch and stream modes, with proper handling of edge cases.
Also applies to: 436-446
keegandent
force-pushed
the
enhancement-dispatch-sink
branch
from
9fde9d1 to
46dd184
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fpga/_processor.py (1)
147-148: TODO comments indicate missing features.The TODO comments suggest that support for fires input and charges output needs to be implemented.
Would you like me to help implement these features or create issues to track them?
Also applies to: 161-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fpga/_processor.py(9 hunks)
🧰 Additional context used
📓 Learnings (1)
fpga/_processor.py (1)
Learnt from: keegandent
PR: TENNLab-UTK/fpga#50
File: fpga/_processor.py:79-88
Timestamp: 2025-02-03T22:18:36.244Z
Learning: In the `dispatch_operand_widths` function, a zero width from `unsigned_width(net_num_io - 1)` is acceptable as there is logic in place to implicitly associate commands with index 0 when appropriate.
🔇 Additional comments (7)
fpga/_processor.py (7)
67-77: LGTM! Improved naming and added synchronization support.The rename from
StreamOpcodetoStreamFlagbetter reflects its usage, and the addition ofSNCopcode enhances communication timing control.
79-88: LGTM! Function signature improved.The addition of
opc_widthparameter makes the function more flexible and reusable.
90-163: LGTM! Well-structured I/O configuration classes.Good use of OOP principles to encapsulate I/O configuration and state management.
275-301: LGTM! Enhanced run method with improved timing control.Added input validation and better synchronization support.
303-372: LGTM! Robust error handling in _hw_rx.Enhanced error handling and added support for synchronization opcode.
611-631: LGTM! Well-structured I/O configuration setup.The _setup_io method provides clear error messages and handles all I/O configurations cleanly.
293-294: TODO comments about timing need attention.Multiple TODO comments indicate that timing issues will be resolved in a future buffers PR.
Also applies to: 385-386, 317-318
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if there's an open PR addressing the timing issues gh pr list --search "buffers in:title,body" --state openLength of output: 147
I'll run a new script to search for all occurrences of "buffers PR" (which should capture the TODO comments at lines 293-294, 317-318, and 385-386) so we can verify the full scope of these TODO comments. This will give us better context on whether additional follow‐up is needed regarding the timing issues.
🏁 Script executed:
#!/bin/bash # Search for all references to "buffers PR" in the repository rg -n "buffers PR" .Length of output: 390
Review Comment Validated: Multiple TODO Comments for Timing Remain Unresolved
The search confirms that the file fpga/_processor.py still contains several instances of the TODO comment referencing a future buffers PR for "magic timing" (found at lines 293, 312, 317, and 385). No open PR was detected to address these timing issues. These comments should be tracked to ensure they are removed or updated once the buffers PR is merged.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
fpga/rtl/network_arstn.sv (1)
20-26: Consider adding a synthesis attribute for clock domain crossing.The negedge triggered flip-flop might be intended for clock domain crossing. If so, consider adding a synthesis attribute to prevent optimization.
+ (* preserve *) // Prevent synthesis optimization logic clear_mask;fpga/rtl/stream_source.sv (1)
42-45: Consider adding comments explaining the timing relationships.While the signal assignments are correct, documenting the timing relationships between src_valid, net_run, net_sync, and net_clear would improve maintainability.
+ // net_run follows src_valid directly assign net_run = src_valid; + // net_sync and net_clear are gated by src_valid to prevent spurious triggers assign net_sync = src_valid ? src[PKT_WIDTH - SNC - 1] : 0; assign net_clear = src_valid ? src[PKT_WIDTH - CLR - 1] : 0;fpga/rtl/stream_sink.sv (1)
45-67: Consider adding assertions for state machine invariants.While the sync and clear state machines are correct, adding assertions would help catch potential timing issues.
+ // Assertions to verify state machine behavior + `ifndef SYNTHESIS + property p_sync_clear_mutex; + @(posedge clk) disable iff (!arstn) + not (sync && clear); + endproperty + assert property(p_sync_clear_mutex); + `endiffpga/rtl/basic_processor.sv (1)
31-42: Consider adding a comment about the constant src_valid.While setting src_valid to 1 is correct for this basic processor, documenting this design choice would help maintainability.
+ // Basic processor assumes continuous valid input .src_valid(1),fpga/rtl/axis_processor.sv (1)
53-58: Good addition of dedicated reset handling.The introduction of the
network_arstnmodule improves the reset logic by properly synchronizing the network reset with the clear signal.fpga/_processor.py (1)
90-163: Well-structured I/O configuration classes with clear inheritance.The object-oriented design effectively encapsulates I/O configuration logic.
Consider implementing the TODO items for charge input/output support.
The implementation notes missing functionality for charge input/output support.
Would you like me to help implement the charge input/output support or create an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
fpga/_processor.py(9 hunks)fpga/rtl/axis_processor.sv(3 hunks)fpga/rtl/basic_processor.sv(2 hunks)fpga/rtl/dispatch_sink.sv(2 hunks)fpga/rtl/dispatch_source.sv(3 hunks)fpga/rtl/network_arstn.sv(1 hunks)fpga/rtl/stream_sink.sv(2 hunks)fpga/rtl/stream_source.sv(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
fpga/rtl/dispatch_source.sv (1)
Learnt from: keegandent
PR: TENNLab-UTK/fpga#50
File: fpga/rtl/dispatch_source.sv:0-0
Timestamp: 2025-02-03T22:16:54.610Z
Learning: In SystemVerilog network processors, when both network execution (`net_en`) and spike loading (`SPK`) could occur in the same cycle, the execution takes precedence and clears all inputs. New spikes are only loaded when `net_en` is false, preventing race conditions.
fpga/rtl/stream_source.sv (1)
Learnt from: keegandent
PR: TENNLab-UTK/fpga#50
File: fpga/rtl/stream_source.sv:45-45
Timestamp: 2025-02-03T22:21:24.741Z
Learning: In the FPGA project's RTL modules, parameter validation (like ensuring bit extraction indices are valid) is handled at higher abstraction layers rather than in low-level modules. The hardware synthesizer catches invalid bit selections automatically, making additional checks redundant at the lower levels.
fpga/_processor.py (1)
Learnt from: keegandent
PR: TENNLab-UTK/fpga#50
File: fpga/_processor.py:79-88
Timestamp: 2025-02-03T22:18:36.244Z
Learning: In the `dispatch_operand_widths` function, a zero width from `unsigned_width(net_num_io - 1)` is acceptable as there is logic in place to implicitly associate commands with index 0 when appropriate.
🔇 Additional comments (22)
fpga/rtl/network_arstn.sv (2)
11-16: LGTM! Clean and well-defined module interface.The module interface clearly defines the reset and clear signals with appropriate directions.
30-30: LGTM! Robust reset implementation.The output logic effectively combines asynchronous reset with masked clear signal, preventing glitches.
fpga/rtl/stream_source.sv (3)
13-18: LGTM! Well-structured package configuration.The package properly imports dependencies and defines width parameters based on network configuration.
20-37: LGTM! Clear module interface with proper parameterization.The module interface effectively uses PKT_WIDTH parameter and clearly defines all network signals.
47-50: LGTM! Efficient input calculation.The always_comb block correctly calculates net_inp using bit slicing with proper width calculations.
fpga/rtl/stream_sink.sv (3)
13-18: LGTM! Consistent package configuration.The package configuration mirrors the source configuration, maintaining symmetry.
20-37: LGTM! Well-defined module interface.The module interface properly parameterizes the packet width and includes all necessary network signals.
69-75: LGTM! Clean output formation logic.The output calculation properly combines sync, clear, and data bits into the packet.
fpga/rtl/basic_processor.sv (5)
13-16: LGTM! Robust width calculations.The package properly calculates input and output widths using max function to handle edge cases.
22-27: LGTM! Clean module interface.The module interface uses processor_config parameters consistently.
44-49: LGTM! Proper reset handling.The network_arstn module is correctly instantiated to handle reset synchronization.
51-56: LGTM! Clean network instantiation.The network module is properly connected with reset and enable signals.
58-70: LGTM! Well-structured sink instantiation.The network_sink module is correctly parameterized and connected.
fpga/rtl/axis_processor.sv (2)
11-19: LGTM! Well-structured package configuration.The package configuration is well-organized with proper width calculations using the
width_nearest_bytemacro.
21-30: LGTM! Standard AXI-Stream interface.The module interface follows AXI-Stream protocol with properly parameterized signal widths.
fpga/rtl/dispatch_source.sv (3)
13-19: LGTM! Well-documented width calculations.The package configuration properly handles width calculations with good documentation about edge cases.
21-47: LGTM! Clear signal naming and interface structure.The module interface is well-organized with descriptive signal names and proper packet width parameterization.
97-110: Confirmed: Network execution takes precedence over spike loading.Based on the previous discussion and learnings, the implementation correctly handles the case when both network execution (
net_run) and spike loading (SPK) could occur in the same cycle, with execution taking precedence.fpga/rtl/dispatch_sink.sv (2)
11-18: LGTM! Consistent width definitions.The package configuration maintains consistency with the source module and includes good documentation about edge cases.
20-37: LGTM! Well-structured interface.The module interface properly handles packet width parameterization with clear signal naming.
fpga/_processor.py (2)
62-88: LGTM! Zero width handling is confirmed correct.The implementation properly handles zero width cases in
dispatch_operand_widths, as confirmed by previous discussion.
278-305: Address timing magic numbers in future buffer PR.The implementation contains several magic timing values and TODO comments indicating that timing issues will be resolved in a future buffer PR.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for timing-related magic numbers and TODOs # Test: Look for magic timing values and related TODOs rg "TODO.*magic timing" fpga/_processor.py rg "sleep\([0-9]+e-[0-9]+\)" fpga/_processor.pyLength of output: 426
Timing Magic Values in _processor.py Confirmed
- Verified that the magic timing value (
sleep(100e-9)) and accompanying TODO comments are present in the file as expected.- The review comment accurately reflects the current state of the code, which includes these magic timing elements intended for resolution in a future buffers PR.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fpga/rtl/basic_processor.sv (1)
13-16: LGTM! Consider adding documentation for the width calculations.The new processor_config package with INP_WIDTH and OUT_WIDTH parameters is well-structured. The width calculations using
maxensure valid widths even when SPK_WIDTH is 0.Consider adding inline documentation explaining the width calculations and their relationship to the dispatch sink sparsity improvements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fpga/rtl/basic_processor.sv(2 hunks)tb/test_simple_processor.py(4 hunks)
🔇 Additional comments (10)
tb/test_simple_processor.py (5)
62-62: LGTM! Signal name standardization.Good cleanup standardizing the signal name from
instrtoinp. This aligns with the hardware interface changes.Also applies to: 66-66, 69-69
92-98: LGTM! Improved test configuration.Good improvement explicitly listing the components under test. This provides better control over test scope and makes dependencies clear.
17-17: LGTM! Verify opcode compatibility.Good move centralizing the opcode definitions in
fpga._processor. This ensures consistent opcode handling across the codebase.Let's verify the opcode compatibility:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that all opcode usages are compatible with the new DispatchOpcode # Check for any remaining local opcode definitions rg -l "class.*opcode.*Enum" --type py # Check all usages of opcode to ensure they match DispatchOpcode ast-grep --pattern 'opcode.$_'Length of output: 258
Opcode usage verified and compatible.
The changes reviewed in
tb/test_simple_processor.pycorrectly reference the centralized dispatch opcode definitions fromfpga._processor. Our search confirms thatopcode.SPKandopcode.RUNare used as intended, with no remaining local opcode definitions.
72-72: Verify the output validation logic change.The switch to modulo operations for output validation suggests a change in the output encoding scheme.
Let's verify the output validation logic:
Also applies to: 74-74
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for similar output validation patterns # Search for similar output validation patterns in test files rg "assert.*out.*value.*%" --type py # Search for corresponding hardware output encoding rg "output.*" --type svLength of output: 4981
Output Validation Logic Verification
The modulo-based assertions in the test file (both for odd and even outcomes) are consistently applied and align with the expected changes in the output encoding scheme. No conflicting evidence was found in the hardware description files.
- tb/test_simple_processor.py contains both
assert dut.out.value % 2 == 1andassert dut.out.value % 2 == 0, indicating clear intended scenarios.- The hardware modules' declarations do not contradict this new encoding, suggesting that the modulo checks are the intentional validation method for the output’s parity.
43-44: Verify the RUN instruction encoding change.The addition of
+ 1to the RUN instruction encoding could affect timing or synchronization. Please ensure this change aligns with the hardware implementation.Let's check for similar patterns in the codebase:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check for similar RUN instruction encoding patterns # Search for RUN opcode usage in Python files rg "RUN.*<<.*\+" --type py # Search for corresponding hardware implementation rg "RUN.*" --type svLength of output: 1048
Verification of the RUN Instruction Encoding Change
The search results indicate that the new encoding pattern for the RUN instruction appears only in the test file (
tb/test_simple_processor.py). The hardware files reference aRUNsignal or state (e.g., infpga/rtl/dispatch_sink.svandfpga/rtl/io_configs.sv), but there is no evidence of a corresponding arithmetic adjustment (i.e. adding+ 1) in those implementations.Please verify that the hardware implementation indeed expects the encoded RUN command to be
(opcode.RUN << operand_width) + 1rather than just(opcode.RUN << operand_width).
- Check if the hardware decoders or state machines have been updated to accommodate the added offset.
- Confirm that the software and hardware remain aligned given this encoding change.
fpga/rtl/basic_processor.sv (5)
22-27: LGTM! Interface changes improve clarity and maintainability.The module interface changes are well-structured:
- Signal names are more descriptive (inp vs instr)
- Width parameters are centralized in processor_config
31-42: LGTM! Improved network source configuration.The network_source changes align well with the new width parameterization and signal naming scheme.
44-49: LGTM! Enhanced reset handling.The addition of network_arstn module improves the reset logic organization.
58-70: LGTM! Network sink parameterization matches source.The network_sink changes maintain symmetry with network_source changes, using consistent PKT_WIDTH parameterization.
51-56: Verify the enable condition change impact.The enable condition has changed from using net_valid to net_run. While this aligns with the PR's goal of improving communication timing, please ensure this change doesn't affect existing network timing assumptions.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other modules that might depend on the previous enable condition rg "net_valid\s*&&\s*net_ready" -A 5Length of output: 38
I'll run an additional search to check for any remaining references to
net_validand to compare with usage ofnet_runacross SystemVerilog files. This should help ensure the change is isolated and confirm that no part of the design is still expecting the previous enable condition.
🏁 Script executed:
#!/bin/bash echo "Searching for occurrences of net_valid in SystemVerilog files:" rg "\bnet_valid\b" -n --glob "*.sv" . echo "----" echo "Searching for occurrences of net_run in SystemVerilog files:" rg "\bnet_run\b" -n --glob "*.sv" .Length of output: 1890
I'll run another search to locate the definition of the
networkmodule and check if its enable input usage might be sensitive to the change fromnet_validtonet_run. This will help ensure that the change doesn't inadvertently affect network timing.
🏁 Script executed:
#!/bin/bash echo "Searching for the network module definition:" rg "module\s\+network\b" -n --glob "*.sv" .Length of output: 144
Enable Condition Verification – No Issues Detected
After investigating the codebase:
- No occurrences of the old enable condition (
net_valid && net_ready) were found.- All modules consistently use
net_run && net_ready(e.g., infpga/rtl/basic_processor.sv,axis_processor.sv,dispatch_source.sv, etc.).- The change appears intentional and aligns with the design aim of improved communication timing.
These findings indicate that the enable condition change should not adversely affect the network timing assumptions. It is, however, recommended that simulation or timing analysis be performed as a final check.
|
@BGull00 Ok, it has been a whirlwind, but I think I resolved the issue of sending spikes along with a CLR flag for stream input, which my changes had somehow broken. I've changed the architecture so that only normal, synchronous signals are passed between source and sink. These signals each represent a form of "valid" depending on sink.
Clear and sync now behave the same way in terms of setting and capturing, but we rely on a new module to convert the synchronous clear signal into an asynchronous reset event upon initial assertion. This prevents a host of issues with "throwing and catching" an event that has different behavior for Dispatch and Stream Sources. This method also allows sinks to "choose" their valid conditions. Dispatch Sink will respond to any of them being asserted and pull |
keegandent
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fpga/_processor.py (2)
616-636: Add input validation forio_typeparameter.While the I/O setup is well-structured, consider adding upfront validation of the
io_typeparameter to fail fast.def _setup_io(self): + if len(self._io_type) != 4: + raise ValueError(f"Invalid io_type length: {self._io_type}\nExpected format: (D|S)I(D|S)O") match self._io_type[:2]:
378-378: Track the magic timing TODO.The TODO comment about magic timing should be tracked to ensure it's addressed in the buffers PR.
Would you like me to create an issue to track this TODO?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fpga/_processor.py(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
fpga/_processor.py (1)
Learnt from: keegandent
PR: TENNLab-UTK/fpga#50
File: fpga/_processor.py:79-88
Timestamp: 2025-02-03T22:18:36.244Z
Learning: In the `dispatch_operand_widths` function, a zero width from `unsigned_width(net_num_io - 1)` is acceptable as there is logic in place to implicitly associate commands with index 0 when appropriate.
🔇 Additional comments (6)
fpga/_processor.py (6)
74-77: LGTM! Improved naming clarity.Renaming
StreamOpcodetoStreamFlagbetter reflects its usage as flags rather than opcodes.
67-71: LGTM! Added synchronization support.The addition of the
SNCopcode aligns with the PR objectives to improve synchronization capabilities.
90-163: LGTM! Well-structured I/O configuration classes.The new configuration classes (
_IoConfig,InpConfig,OutConfig) provide good encapsulation of I/O settings and clear separation of concerns.
197-209: LGTM! Improved spike handling with synchronization support.The updated spike handling logic correctly manages timing and includes synchronization support for dispatch mode.
298-365: LGTM! Robust synchronization handling in receive logic.The receive logic now properly handles synchronization events and includes clear error messages for timing mismatches.
366-451: LGTM! Well-structured transmit logic with sync support.The transmit logic cleanly handles both dispatch and stream modes with proper synchronization support.
🧰 Tools
🪛 Ruff (0.8.2)
375-375: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Closes #49.