diff --git a/text/0073-stricter-connections.md b/text/0073-stricter-connections.md new file mode 100644 index 0000000..6d05230 --- /dev/null +++ b/text/0073-stricter-connections.md @@ -0,0 +1,101 @@ +- Start Date: 2024-09-16 +- RFC PR: [amaranth-lang/rfcs#73](https://github.com/amaranth-lang/rfcs/pull/73) +- Amaranth Issue: [amaranth-lang/amaranth#1511](https://github.com/amaranth-lang/amaranth/issues/1511) + +# Stricter connections + +## Summary +[summary]: #summary + +Make it possible for `lib.wiring.connect()` to reject connections between ports with shapes that despite having equal widths are incompatible. + +## Motivation +[motivation]: #motivation + +The primary motivating use case is to be able to prevent connecting stream interfaces with incompatible payload shapes. + +## Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +`lib.stream` defines a basic stream interface with `ready`, `valid` and `payload` members. +Additional metadata signals can be added by making the `payload` shape an aggregate. +This has the advantage that such streams can be passed through standard components like FIFOs without them having to know or care about these signals. + +Consider how we can make a byte-wide packetized stream by defining the `payload` shape as `StructLayout({"data": 8, "last": 1})`. +The `data` field contains the data byte and the `last` field is a flag indicating that the current byte is the last in the packet. + +This is not the only common way to make a packetized stream. +Instead of (or in addition to) the `last` flag, the payload could have a `first` flag, indicating that the current byte is the first in the packet. +Both are valid options with different semantics for different usecases. + +The problem is that `lib.wiring.connect()` currently only checks that port members have matching widths, so a connection between a stream interface with `last` semantics and a stream interface with `first` semantics will not be rejected, despite being incompatible. + +Currently, `lib.data.View.eq()` does no checks on the passed value and immediately forwards to `self.as_value().eq()`, making this legal: + +```python +>>> a = Signal(StructLayout({"data": 8, "last": 1})) +>>> b = Signal(StructLayout({"data": 8, "first": 1})) +>>> a.eq(b) +(eq (sig a) (sig b)) +``` + +This RFC proposes adding a check to `View.eq()` that would reject direct assignments from another aggregate data structure with a non-identical layout. +If such an assignment is desired, the other aggregate data structure can be explicitly passed through `Value.cast()` first. + +Currently `lib.wiring.connect()` passes every signal through `Value.cast()` before assigning them. +This results in a `ValueCastable`'s `.eq()` not being called, and thereby bypassing the check proposed above. + +This RFC proposes removing the `Value.cast()` so a `ValueCastable`'s `.eq()` will be called directly. + +This RFC proposes updating `lib.enum.EnumView` in the same manner, for the same reason, as `lib.data.View`. + +## Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +Modify `lib.wiring.connect()` to not pass port members through `Value.cast()`, so that a `ValueCastable`'s `.eq()` will be called, allowing it to perform compatibility checks. +- If a `ValueCastable` doesn't define `.eq()`, reject the assignment. + +Modify `lib.data.View.eq(other)` to add the following checks: +- If `other` is a `ValueCastable`, do `Layout.cast(other.shape())` + - If a valid `Layout` is returned, reject the assignment if it doesn't match `self.shape()`. + - If `Layout.cast()` raises, reject the assignment. +- Otherwise, proceed as normal. + +Modify `lib.enum.EnumView.eq(other)` to add the following checks: +- If `other` is a `ValueCastable`, reject the assignment if `other.shape()` doesn't match `self.shape()`. +- Otherwise, proceed as normal. + +Rejected assignments are a warning in Amaranth 0.6 and becomes a hard error in Amaranth 0.7. + +## Drawbacks +[drawbacks]: #drawbacks + +- Increased language complexity. + +- This will add an implied requirement for a `ValueCastable` to implement `.eq()` to be usable with `lib.wiring`. Currently a `ValueCastable` is not required to implement `.eq()` at all. + - We could fall back to `Value.cast().eq()` when `.eq()` is not defined. + +## Rationale and alternatives +[rationale-and-alternatives]: #rationale-and-alternatives + +- Signals like `first` and `last` could be added as separate ports in the stream signature, preventing incompatible connections. + - This was already discussed in [RFC 61](0061-minimal-streams.md) and decided against. +- An explicit hook for checking compatibility between interfaces could be added, instead of relying on `.eq()`. + - This RFC proposes picking the low-hanging fruits first, making use of already existing mechanisms. + If this is not enough, another RFC can propose an explicit hook later. + +## Prior art +[prior-art]: #prior-art + +[RFC 61](0061-minimal-streams.md) already discussed the need to eventually make `lib.wiring.connect()` stricter to prevent the connection of stream interfaces with incompatible payloads. + +## Unresolved questions +[unresolved-questions]: #unresolved-questions + +None. + +## Future possibilities +[future-possibilities]: #future-possibilities + +- A hook can still be added to allow a signature/interface to perform an explicit compatibility check, for cases where signatures have identical members but still have metadata indicating they are incompatible. + - This hook could also allow overriding the existing checks, allowing connections where interfaces are compatible despite differences in members.