Allow to specify per-subsystem signal channel size #29
Conversation
Closes: #28
vstakhov
force-pushed
the
vstakhov-variable-signals-size
branch
from
9835307 to
75bc689
Compare
| /// Capacity of a bounded message channel between orchestra and subsystem | ||
| /// but also for bounded channels between two subsystems if not overriden. | ||
| const CHANNEL_CAPACITY: usize = #message_channel_capacity; | ||
|
|
||
| /// Capacity of a signal channel between a subsystem and the orchestra. | ||
| const SIGNAL_CHANNEL_CAPACITY: usize = #signal_channel_capacity; |
There was a problem hiding this comment.
Is the const SIGNAL_CHANNEL_CAPACITY still required?
There was a problem hiding this comment.
Yes, it is used in the connector from what I see.
There was a problem hiding this comment.
But shouldn't it be symmetrical at this point with the CHANNEL_CAPACITY/message_channel_capacity.
There was a problem hiding this comment.
So originally I used the constants as intermediary to be able to reason better about the code and be able to see where the values come from in the generated code. We should strive for consistency.
I have to dig into the connector part again, it might be we don't know the signal capacity early enough for overriding in that case, idqr.
There was a problem hiding this comment.
Connector can be specified as an external argument, so it cannot really reason about the content of the builder/orchestra structures.
There was a problem hiding this comment.
The remaining uses of the signal channel capacity are a hack, because we need to construct the the sender before we start creating the builder. And we need that since we need to pass the sender around to some substrate based thing. I'd have to check polkadot again to see if the connector / builder split is actually used.
We could create a custom sender like API and internally mutate the thing with a buffer. It's not pretty and quite a bit of code for a questionable gain and also debatable if that's not a misc addition.
There was a problem hiding this comment.
Let's defer the connector part and focus on getting this PR merged
|
@vstakhov let's rebase your PR and then get the commits into main |
| @@ -63,7 +63,7 @@ impl<Context> Fortified { | |||
|
|
|||
| #[orchestra(signal=SigSigSig, event=EvX, error=Yikes, gen=AllMessages)] | |||
| struct Duo<T, U, V, W> { | |||
| #[subsystem(consumes: MsgStrukt, sends: [Plinko], message_capacity: 32768)] | |||
| #[subsystem(consumes: MsgStrukt, sends: [Plinko], message_capacity: 32768, signal_capacity: 128)] | |||
There was a problem hiding this comment.
I would add test for the overrides(ensure we block when channel full) to ensure it doesn't break over time.
Merged, sorry for a delay. |
This issue also addresses #28 and allows to use both
message_capacityandsignal_capacityin per-subsystem overrides.