spi: group into read-only, write-only and read-write traits. #323

Dirbaio · 2021-11-03T15:34:01Z

This PR depends on #331

The problem

Currently, embedded-hal has independent traits for every single method. The rationale is to allow HALs to only implement what they support. For example, a HAL could allow setting up an SPI with a MOSI pin but no MISO pin. This would be a "write-only" SPI, so the resulting struct would implement write and write_iter, but not other methods like read or transfer. This is awesome, because it enforces at compile time that you can't pass a write-only SPI to a driver that needs a read-write SPI.

However, splitting one trait per method is way too granular. It leaves too much room for variability in HAL implementations. I've done a quick survey of which HAL implements which traits, here's the result:

	Transfer	Write	WriteIter	Transactional
stm32f1xx-hal	x	x
stm32f3xx-hal	x	x
stm32f4xx-hal	x	x	x	x
stm32f7xx-hal	x	x	x
stm32h7xx-hal	x	x
stm32l0xx-hal	x	x
stm32l4xx-hal	x	x
stm32g4xx-hal	x	x
David-OConnor/stm32-hal	x	x
nrf-hal SPI	x	x	x
nrf-hal SPIM	x	x
esp32-hal	x	x	x
esp-idf-hal	x	x	x
linux-embedded-hal	x	x		x
atsamd-hal	x	x	x
lpc55s6x-hal	x	x	x
imxrt-hal	x	x	x
e310x-hal	x	x	x

As you can see, all HALs implement write and transfer, but only about half implement write_iter and almost none implements transactional.

The result is drivers can't reliably rely on Transactional or WriteIter being present, so they stick to Transfer and Write to ensure maximum HAL compatibility. This in turn means HALs have little incentive to implement Transactional and WriteIter since no drivers use them. This causes a chicken-and-egg situation, which in the end makes Transactional and WriteIter useless for drivers in practice.

(Too much freedom for HALs causing traits to not be useful for drivers is a common theme in e-h. For example #312 and #201 are the same. IMO e-h should be slightly more opinionated in general. Just enough opinionatedness to force HAL impls to be consistent to be useful for drivers.)

The solution

The solution is to split the traits by hardware capability, not by method.

If an SPI is capable of write, it's capable of write_iter. It can write bytes, after all. There's no reason to allow a HAL to implement write but not write_iter.
If an SPI is capable of transfer, there's no reason it can't transfer_inplace, transactional, read, write, etc. If it can read and write, it should be able to do all the kinds of reads/writes we have.

So, I propose we split the traits like this:

Read: read-only SPI.
Write: write-only SPI. It supports all the forms of writing: write and write_iter.
ReadWrite: read-write SPI. It requires Read and Write, and supports all the forms of bidirectional transfers: transfer, transfer_inplace, transactional.

This way HALs keep the "good" freedom to declare their SPI is readonly/writeonly, but they no longer have the "bad" freedom of only implementing the basic traits making the advanced traits useless.

Other traits

I believe this should be also applied to the other traits. I propose discussing this in the context of SPI here, and if the consensus is it's good I'll send PRs doing the same for the other traits.

rust-highfive · 2021-11-03T15:34:04Z

r? @ryankurte

(rust-highfive has picked a reviewer for you, use r? to override)

ryankurte · 2021-11-04T21:30:44Z

huh, definitely an interesting concept. i like the idea of grouping, it's a neat solution to offer default impls for approaches like transactional, and it seems like combining these could substantially decrease the complexity for drivers using the trait types?

for example, SPI bindings like:

where
  T: Read<u8, Error=E> + Write<u8, Error=E> + Transactional<u8, Error=E>

would become:

where
  T: WriteRead<u8, Error=E>

it also raises the question for other traits, obviously most of these are orthogonal (and we've already done this for things like stateful pins) but for example, is there any reason to have separate i2c write and write_read traits when you can't usually interact with an i2c device without both?

Dirbaio · 2021-11-04T21:34:10Z

is there any reason to have separate i2c write and write_read traits when you can't usually interact with an i2c device without both?

Yeah I think i2c should be a single trait. I don't think "write-only i2c" exists, right?

Dirbaio · 2021-11-04T21:37:00Z

The bound would be even simpler, just

where T: WriteRead

The Error=E was needed to enforce all traits to have the same error type. Now the traits already enforce that, so the E generic param in the driver struct can be removed, using T::Error instead.

ryankurte · 2021-11-04T22:13:10Z

Yeah I think i2c should be a single trait. I don't think "write-only i2c" exists, right?

not afaik

The Error=E was needed to enforce all traits to have the same error type. Now the traits already enforce that, so the E generic param in the driver struct can be removed, using T::Error instead.

i think you're still going to need the error bound at a driver level to effectively wrap bus / driver errors anyway (for driver methods, like pub fn do_something(&mut self) -> DeviceError<BusError, PinError>)

this seems like an improvement to me, any other votes @rust-embedded/hal ?

Dirbaio · 2021-11-04T22:24:42Z

You still need it in error enums, but you don't need it in the driver struct itself:

enum MyDriverError<SpiError, PinError> {
    Spi(SpiError),
    Pin(PinError),
}

struct MyDriver<S, P> { spi: S, pin: P }

impl<S: spi::ReadWrite, P: OutputPin> MyDriver<S, P> {
   fn do_something(&mut self) -> Result<(), MyDriverError<S::Error, P::Error> {
     ...
   }
}

eldruin

I disagree with a few statements in the problem description.
For example, a good driver which needs transactional would encourage HAL impls to support it.
I see the ecosystem development to be much more of a hand-in-hand thing.
I should note that we used to have 20-line-ish blanket impls for Transactional, so supporting it given Write and Transfer is pretty trivial.

Nevertheless, I think the solution is a good idea, including the default method impls and forcing the error types to match.

The default impls here would clash with ManagedCS, though.
Also, being such a big change and touching traits that Just Work ™️, I think some HALs should be adapted as proof, just in case we are missing something.

eldruin · 2021-11-09T20:45:26Z

src/spi/blocking.rs

+    /// typically `0x00`, `0xFF`, or configurable.
+    fn read_batch(&mut self, words: &mut [&mut [W]]) -> Result<(), Self::Error> {
+        for buf in words {
+            self.read(buf)?;


This default impl would not work for a Read + ManagedCS impl as read() would assert/deassert CS for each slice.

eldruin · 2021-11-09T20:46:02Z

src/spi/blocking.rs

+    /// Writes all slices in `words` to the slave as part of a single SPI transaction, ignoring all the incoming words
+    fn write_batch(&mut self, words: &[&[W]]) -> Result<(), Self::Error> {
+        for buf in words {
+            self.write(buf)?;


Same thing with Write+ManagedCS

eldruin · 2021-11-09T20:46:46Z

src/spi/blocking.rs

+        WI: IntoIterator<Item = W>,
+    {
+        for word in words {
+            self.write(&[word])?;


eldruin · 2021-11-09T20:47:16Z

src/spi/blocking.rs

+                Operation::Read(words) => self.read(words)?,
+                Operation::Write(words) => self.write(words)?,
+                Operation::Transfer(read, write) => self.transfer(read, write)?,
+                Operation::TransferInplace(words) => self.transfer_inplace(words)?,


For completeness: same

Dirbaio · 2021-11-09T21:15:22Z

For example, a good driver which needs transactional would encourage HAL impls to support it.

A driver author writes a driver because they need it for a project they're working on now. The author has two options:

Fork their HAL to add support for Transactional, PR it, then use Transactional in the driver.
Use Write/Transfer as needed, do the for loop themselves in their driver.

Option 2 is way way easier. The author wants their driver to work now. Why would they choose option 1?

Dirbaio · 2021-11-09T21:19:29Z

The default impls here would clash with ManagedCS, though.

ManagedCs would specify "for each method call of the SPI traits, CS has to be driven low before the entire method call, then driven high after". This would apply to all methods in the trait, including the ones with a defualt impl.

This means ManagedCs impls MUST override all the spi methods, not using any default impl. If they don't, they're wrong. It's not a conflict.

eldruin · 2021-11-10T09:14:44Z

Option 2 is way way easier. The author wants their driver to work now. Why would they choose option 1?

I agree option 2 can be easier but I, for one, did implement transactional for linux-embedded-hal including doing it in rust-i2cdev while doing driver contract work. And that was way more work than taking a blanket impl.
Not everybody that is writing a driver is in the same situation and I would not portray it as such.

re. ManagedCS: I agree all HALs have to do is implement the methods themselves again. I just wanted to highlight that there is an use-case where the default impls here do the wrong thing. We should be aware of this and document it clearly albeit in #245.

Dirbaio · 2021-11-10T18:06:20Z

re. ManagedCS: I agree all HALs have to do is implement the methods themselves again. I just wanted to highlight that there is an use-case where the default impls here do the wrong thing. We should be aware of this and document it clearly albeit in #245.

Agreed, it's kind of a footgun. Maybe we could not have default method impls? Adding a trait method with a default impl is backwards-compatible, but arguably not in this case because it would cause ManagedCs impls that were previously correct to become incorrect?

Maybe a different ManagedCs design could avoid these advantages? A closure-based one? Not sure if it's worth it, the existing design is super nice and simple, anything else wlil be more complex.

Sympatron · 2021-11-10T22:04:11Z

Yeah I think i2c should be a single trait. I don't think "write-only i2c" exists, right?

There are devices where a write-only I²C would be enough to use them, because they can't be read anyway.
But from a HAL's perspective I don't think there is any MCU that has a write-only I²C mode. So any sane HAL implementation could implicitly read, too. Which would make restricting a driver to Write kind of pointless even if it only used write().

GrantM11235 · 2021-11-14T20:57:20Z

I think the best solution is to remove the default method implementations for now.

If we can find a solution to the chipselect problem (and I am hopeful that we can), we should be able to add the default method implementations back as a non-breaking change.

Dirbaio · 2021-11-23T20:10:05Z

Removed the default method impls.

eldruin

Looking good, thanks!
Left to do would be the adaption of a HAL (ideally 2) to make sure we are not missing anything, since this touches traits that currently work fine, like I said earlier.

burrbull · 2021-11-24T03:00:49Z

Removed the default method impls.

I'm fine with this. But instead of deleted code, I'd want to see implementation examples in docs.

GrantM11235 · 2021-12-02T05:10:24Z

I don't think that it is necessary to include implementation examples in the docs, it would just be extra clutter for users of the traits to sift through. I think that HAL authors will have no trouble implementing the traits using the existing documentation.

Sh3Rm4n · 2021-12-02T08:57:01Z

Good point, that this documentation is not really relevant for the user. The documentation of the trait should gear towards the user of the trait just as well as the implementor.

We could either add a # Implementation Example section to the docs or link to another block or to examples/ for implementation examples. With -Z rustdoc-scrape-examples=examples in sight, this might be the best place for the documentation.

Dirbaio · 2021-12-19T08:35:35Z

I've split the "unify error types" to #331, also including unifying across nb and blocking.

Dirbaio · 2021-12-20T23:07:24Z

Without read_batch/write_batch it's impossible to read/write multiple buffers in a single transaction for impls that impl only Write or only Read. This will be a problem when combined with ManagedCs: it would force users to fall back to manual CS management.

Arguably this is only a problem for Read, since Write has write_iter though, which you can use to chain multiple buffers. I don't see an easy way to make a read_iter though, it seems it requires GATs.

I have no strong opinion, I'm fine with removing them, I just thought they'd be handy.

ryankurte · 2021-12-21T20:30:58Z

Without read_batch/write_batch it's impossible to read/write multiple buffers in a single transaction for impls that impl only Write or only Read. This will be a problem when combined with ManagedCs: it would force users to fall back to manual CS management.

i suspect for these cases you could use Iterator::Chain with the write_iter or read_iter methods?

i have a weak preference for eliding them to minimise the API surface but, would be okay either way.

Dirbaio · 2022-01-02T09:20:11Z

@ryankurte I suspect for these cases you could use Iterator::Chain with the write_iter or read_iter methods?

There's no read_iter (it'd require GATs, nightly-only)
iter-based APIs work really badly with DMA so in the Async #334 PR I've proposed only having read_batch/write_batch which can be DMA'd efficiently. Having only iter methods in blocking and only batch methods in async would be an inconsistency.

331: spi: enforce all traits have the same Error type. r=ryankurte a=Dirbaio Previously discussed in #323, split off to its own PR now. This PR enforces all SPI trait impls for a given type use the same Error associated type, by moving it to its own ErrorType trait. ## How breaking is this? I believe this is not very breaking in practice (it won't make upgrading existing code to 1.0 harder), because: - HALs already use the same error type for all methods (I haven't seen one that doesn't) - Drivers already often add bounds to enforce the errors are the same. [Example](https://github.com/rust-iot/rust-radio-sx127x/blob/8188c20c89603dbda9592c40a8e3fc5b37769a00/src/lib.rs#L122). Without these bounds, propagating errors up would be more annoying, drivers would need even more generic types `SpiWriteError, SpiReadError, SpiTransferError, SpiTransactionalError`... ## Why is this good? Traits being able to have different Error types is IMO a case of "bad freedom" for the HALs. Today's traits don't stop HALs from implementing them with different error types, but this would cause them to be incompatible with drivers with these bounds. If traits force error types to be the same, the problem is gone. ## What about other traits? I believe this should be also applied to the other traits. I propose discussing this in the context of SPI here, and if the consensus is it's good I'll send PRs doing the same for the other traits. Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>

Dirbaio · 2022-01-11T22:12:29Z

Rebased.

Also, renamed read_batch, write_batch, batch to read_transaction, write_transaction, transaction. It highlights the "single transaction" part which is quite important and the entire reason the methods exist.

ping @eldruin @therealprof @ryankurte thoughts, now that #331 is merged?

Dirbaio · 2022-01-13T16:19:19Z

Rebased for W->Word rename.

What's the final decision regarding read_transaction, write_transaction? Do we keep them or not?

ryankurte

Without read_batch/write_batch it's impossible to read/write multiple buffers in a single transaction for impls that impl only Write or only Read. This will be a problem when combined with ManagedCs: it would force users to fall back to manual CS management.

is this a thing in practice? while many peripherals might be unidirectional i can't think of an SPI controller i have used that only supports write or read.

i can't find the latest in my mailbox but iirc the latest approach to ManagedCS looks like some kind of closure, so you'll be able to call .with_cs( |s| { s.read(..)?; s.read(..)? }) to chain operations without using transactional.
(@GrantM11235 do you have a branch / PR for the work you were doing on this?)

What's the final decision regarding read_transaction, write_transaction? Do we keep them or not?

i really don't know, gut feeling is they're not needed / demonstrated, but i can see they'd be more annoying to add later. @eldruin any opinions?

src/spi/blocking.rs

ryankurte · 2022-01-13T20:30:06Z

src/spi/blocking.rs

-    }
-}
+    /// Writes all slices in `words` to the slave as part of a single SPI transaction, ignoring all the incoming words
+    fn write_transaction(&mut self, words: &[&[Word]]) -> Result<(), Self::Error>;


in transaction we call them operations rather than words, here they're not exactly operations but also not really words. non critical, but, are we happy with this / is there any clearer name?

Dirbaio · 2022-01-13T20:55:10Z

is this a thing in practice? while many peripherals might be unidirectional i can't think of an SPI controller i have used that only supports write or read.

All hardware supports read+write, but the user might want a write-only SPI for example, with MOSI but no MISO. HALs might want to have a separate type for this that impls Write but not Read, to avoid misuse. Drivers will then want to require only spi::Write to be compatible with these HALs. This is super common for SPI displays, where you just blast data at it.

i can't find the latest in my mailbox but iirc the latest approach to ManagedCS looks like some kind of closure

Hmm, then maybe they aren't as necessary, yeah... Doesn't the same argument apply to the (read-write) transaction method too though? It seems to me we should either have all three, or none.

Sympatron · 2022-01-14T11:37:28Z

I like the transaction methods and think they would be useful for driver authors.

eldruin · 2022-01-14T15:21:54Z

After further discussion with @ryankurte, we would favor removing the read_transaction/ write_transaction methods.
We appreciate the handiness of the methods but the boilerplate increase to use the transactional interface instead is rather minimal and we would prefer to keep the API surface smaller.

let a = [0; 2];
let b = [0; 3];
write_transaction(&[&a, &b]);
transaction(&[Operation::Write(&a), Operation::Write(&b)]);

As pointed out above, any SPI controller supports read and write in practice, so using the transactional interface would be possible.
As for restricting use to only read or write with the Read/Write traits, I do not see a real chance of making a mistake and using the wrong method or any other real "misuse" protection.

Dirbaio · 2022-01-14T15:42:53Z

Do we want to support readonly, writeonly, and "readwrite but in readonly/writeonly mode because it's missing pins" SPI peripherals?

If the answer is yes, then we should have the Read, Write traits be "first class citizens", complete with with their read_transaction, write_transaction methods.

If the answer is no (it seems you're arguing for no), then we should merge all SPI traits in one, like #339. (and then not have read_transaction, write_transaction, of course).

Having separate traits but without separate read_transaction, write_transaction is a weird "yes but no but yes" answer to the above question.

Shall I merge all 3 traits? :D

eldruin · 2022-01-14T16:44:03Z

I think merging all traits into one (1.) makes sense except there is a general problem with implementing Transactional, then I would prefer (2.).
Again the options:

Spi trait with Read, Write and Transactional
ReadWrite, Transactional
Read, Write, Transactional (status quo)

Opinions @rust-embedded/hal?

Dirbaio · 2022-01-14T18:24:36Z

What's the problem with Transactional? AFAICT all hardware that can impl read/write/transfer should be able to impl transactional.

burrbull · 2022-01-14T18:52:10Z

Saying about transfer and transactional.
If all methods will be in 1 trait, we can restore default implementations removed in #289 as now they will not conflict with external implementations.

Dirbaio · 2022-01-14T19:00:41Z

I don't think making it a single trait fixes the original issue raised in #289. Also, the arguments against the Default I raised in #289 (comment) still apply IMO

eldruin · 2022-01-14T20:02:37Z

AFAIK there is no problem with Transactional, I was just wondering.
If we merge all into one trait, what would be the problem with some default implementations like the following?

// I just wrote this down, please ignore trivial mistakes
pub trait Spi {
  fn write(&mut self, data: &[Word]) -> Result<(), Self::Error> {
    self.transaction(&[Operation::Write(data)])
  }
  fn read(&mut self, data: &mut [Word]) -> Result<(), Self::Error> {
    self.transaction(&[Operation::Read(data)])
  }
  fn transaction(&mut self, ops: &[Operation]) -> Result<(), Self::Error>;
}

ryankurte · 2022-01-14T20:29:20Z

Doesn't the same argument apply to the (read-write) transaction method too though?

yep, though it is also useful of itself for DMA-ing a set of transactions on platforms that support this.

If we merge all into one trait, what would be the problem with some default implementations like the following?

one of the side effects of the newer option for ManagedCS is you have to be very careful with default implementations because you can fall through to the wrong ones, i think it's probably better to leave them off for now.

Having separate traits but without separate read_transaction, write_transaction is a weird "yes but no but yes" answer to the above question.

i still prefer this, though in practice i have never only used Write or Read (and tend to just use Transactional). i think it is less "yah nah yeah" and more that there are still changes to come ^_^

Dirbaio · 2022-01-14T23:20:46Z

There's another use case for readonly/writeonly SPI: limited DMA channels.

STM32 needs one DMA channel for read, another for write. If the user only wants to write it's very rude to force them to waste one DMA channel for reading. Channels are very scarce on low-end chips (smallest G0, L0 only have 5!). Also, in chips without DMAMUX, channels have no/limited remappability, so the wasted channel could cause a conflict even if there are other free channels.

embassy-stm32 SPI driver is generic on the channels, and only impls the traits if the channels in the needed directions are set: https://github.com/embassy-rs/embassy/blob/ddf8c99a938f7d2c4bb6e3ea9476c487c5b1c047/embassy-stm32/src/spi/mod.rs#L608-L651

This applies more to async, rather than blocking, but I'd like async+blocking to be consistent on this.

Given this, I no longer think unifying SPI into a single trait is a good idea...

IMO the answer to "Do we want to support readonly, writeonly SPI peripherals?" is Yes.

On read_transaction, write_transaction: I still don't see why transaction is useful but read/write_transaction is not. They're both equally useful.

If HALs are going to have write-only/read-only SPI, you can't use transaction on them, so read/write_transaction is the solution for writing multiple buffers.

burrbull · 2022-01-15T11:00:35Z

In any case, I believe that it is better to solve the problem of missing implementations in organizational way.
Revisit all known repositories with hals and open PRs or/and issues with list of traits that are not implemented.

eldruin · 2022-01-17T07:49:29Z

Thanks for that important argument @Dirbaio. Let's keep Read, Write and Transactional separate.
As for read_transaction/write_transaction, @ryankurte had some additional concerns regarding managed CS compatibility. He also made a new proposal at #350

339: I2c unify r=eldruin a=Dirbaio Depends on #336 Equivalent of #323 but for I2c. I think for i2c unifying everything in a single trait makes the most sense. The i2c bus is specified to be bidirectional, I believe no hardware out there can "only write" or "only read" (and writing requires *reading* ACK bits anyway!). Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>

Dirbaio · 2022-02-16T02:11:22Z

Closing in favor of #351 which incorporates all the ideas from here.

Dirbaio requested a review from a team as a code owner November 3, 2021 15:34

rust-highfive assigned ryankurte Nov 3, 2021

rust-highfive added S-waiting-on-review Review is incomplete T-hal labels Nov 3, 2021

Dirbaio force-pushed the spi-unify branch from ad6f1d8 to 48b4c9b Compare November 4, 2021 22:03

eldruin reviewed Nov 9, 2021

View reviewed changes

Dirbaio force-pushed the spi-unify branch 2 times, most recently from 3b4b548 to 039bd36 Compare November 23, 2021 20:09

eldruin reviewed Nov 23, 2021

View reviewed changes

Dirbaio force-pushed the spi-unify branch from e2b35b7 to df19c44 Compare November 23, 2021 20:15

eldruin added this to the v1.0.0 milestone Dec 14, 2021

Dirbaio force-pushed the spi-unify branch from df19c44 to 11cfe62 Compare December 19, 2021 08:21

Dirbaio mentioned this pull request Dec 19, 2021

spi: enforce all traits have the same Error type. #331

Merged

Dirbaio force-pushed the spi-unify branch from 02f1371 to fa8f1fb Compare January 11, 2022 21:53

Dirbaio force-pushed the spi-unify branch from fa8f1fb to 8725bc6 Compare January 11, 2022 22:09

Dirbaio mentioned this pull request Jan 12, 2022

async: add SPI #347

Merged

Dirbaio force-pushed the spi-unify branch from 8725bc6 to df935ac Compare January 13, 2022 16:17

ryankurte reviewed Jan 13, 2022

View reviewed changes

spi/blocking: group into read-only, write-only and read-write traits

0aad3d4

Dirbaio force-pushed the spi-unify branch from df935ac to 0aad3d4 Compare January 19, 2022 21:46

eldruin mentioned this pull request Feb 14, 2022

Ship a 1.0 release #177

Closed

Dirbaio closed this Feb 16, 2022

spi: group into read-only, write-only and read-write traits. #323

spi: group into read-only, write-only and read-write traits. #323

Conversation

Dirbaio commented Nov 3, 2021 • edited Loading

The problem

The solution

Other traits

rust-highfive commented Nov 3, 2021

ryankurte commented Nov 4, 2021

Dirbaio commented Nov 4, 2021

Dirbaio commented Nov 4, 2021 • edited Loading

ryankurte commented Nov 4, 2021

Dirbaio commented Nov 4, 2021

eldruin left a comment • edited Loading

Choose a reason for hiding this comment

eldruin Nov 9, 2021

Choose a reason for hiding this comment

eldruin Nov 9, 2021

Choose a reason for hiding this comment

eldruin Nov 9, 2021

Choose a reason for hiding this comment

eldruin Nov 9, 2021

Choose a reason for hiding this comment

Dirbaio commented Nov 9, 2021

Dirbaio commented Nov 9, 2021

eldruin commented Nov 10, 2021

Dirbaio commented Nov 10, 2021

Sympatron commented Nov 10, 2021

GrantM11235 commented Nov 14, 2021

Dirbaio commented Nov 23, 2021

eldruin left a comment • edited Loading

Choose a reason for hiding this comment

burrbull commented Nov 24, 2021

GrantM11235 commented Dec 2, 2021

Sh3Rm4n commented Dec 2, 2021

Dirbaio commented Dec 19, 2021

Dirbaio commented Dec 20, 2021

ryankurte commented Dec 21, 2021

Dirbaio commented Jan 2, 2022

Dirbaio commented Jan 11, 2022

Dirbaio commented Jan 13, 2022

ryankurte left a comment

Choose a reason for hiding this comment

ryankurte Jan 13, 2022

Choose a reason for hiding this comment

Dirbaio Jan 13, 2022

Choose a reason for hiding this comment

Dirbaio commented Jan 13, 2022

Sympatron commented Jan 14, 2022

eldruin commented Jan 14, 2022

Dirbaio commented Jan 14, 2022 • edited Loading

eldruin commented Jan 14, 2022

Dirbaio commented Jan 14, 2022

burrbull commented Jan 14, 2022

Dirbaio commented Jan 14, 2022

eldruin commented Jan 14, 2022

ryankurte commented Jan 14, 2022

Dirbaio commented Jan 14, 2022 • edited Loading

burrbull commented Jan 15, 2022

eldruin commented Jan 17, 2022

Dirbaio commented Feb 16, 2022

Dirbaio commented Nov 3, 2021 •

edited

Loading

Dirbaio commented Nov 4, 2021 •

edited

Loading

eldruin left a comment •

edited

Loading

eldruin left a comment •

edited

Loading

Dirbaio commented Jan 14, 2022 •

edited

Loading

Dirbaio commented Jan 14, 2022 •

edited

Loading