cpu/sam0: update I2C driver to new API #9198

dylad · 2018-05-25T11:27:00Z

Contribution description

This PR updates the I2C driver from sam0 families CPU and also update their periph_conf.h file to fit the new driver.
tested with #9193 #9195 and ADXL345

Issues/PRs references

#6577

aabadie

Only minor doxygen things and style. Not tested

aabadie · 2018-05-25T12:49:53Z

cpu/sam0_common/include/periph_cpu_common.h

@@ -41,6 +42,17 @@ extern "C" {
 #define PERIPH_SPI_NEEDS_TRANSFER_REGS
 /** @} */

+/**
+ * @brief   Use shared I2C functions


Should be @name

aabadie · 2018-05-25T12:50:04Z

cpu/sam0_common/include/periph_cpu_common.h

+#define PERIPH_I2C_NEED_READ_REGS
+#define PERIPH_I2C_NEED_WRITE_REG
+#define PERIPH_I2C_NEED_WRITE_REGS
+


blank line could be removed

aabadie · 2018-05-25T12:50:16Z

cpu/sam0_common/include/periph_cpu_common.h

+
+/**
+ * @brief   Override SPI clock speed values
+ * @{


aabadie · 2018-05-25T12:50:22Z

cpu/sam0_common/include/periph_cpu_common.h

+    I2C_SPEED_FAST_PLUS = 1000000U,    /**< fast plus mode:    ~1Mbit/s */
+    I2C_SPEED_HIGH      = 3400000U,    /**< high speed mode:   ~3.4Mbit/s */
+} i2c_speed_t;
+/** @} */


aabadie · 2018-05-25T12:50:28Z

cpu/sam0_common/include/periph_cpu_common.h

+    uint8_t gclk_src;       /**< GCLK source which supplys SERCOM */
+    uint8_t flags;          /**< allow SERCOM to run in standby mode */
+} i2c_conf_t;
+/** @} */


aabadie · 2018-05-25T12:51:16Z

cpu/sam0_common/periph/i2c.c

-    /* start transmission and send slave address */
-    if (_start(i2c, address, I2C_FLAG_READ) < 0) {
-        return 0;
+    if(!(flags & I2C_NOSTART))


a space after if could be added

aabadie · 2018-05-25T12:51:27Z

cpu/sam0_common/periph/i2c.c

    }
    /* read data to register */
-    if (_read(i2c, data, length) < 0) {
+    if (_read(bus(dev), data, len) < 0) {


a space after if could be added

aabadie · 2018-05-25T12:51:42Z

cpu/sam0_common/periph/i2c.c

-    /* start transmission and send slave address */
-    if (_start(i2c, address, I2C_FLAG_WRITE) < 0) {
-        return 0;
+    if(!(flags & I2C_NOSTOP)) {


a space after if could be added

aabadie · 2018-05-25T12:51:58Z

cpu/sam0_common/periph/i2c.c

-    /* send register address and wait for complete transfer to be finished */
-    if (_write(i2c, &reg, 1) < 0) {
-        return 0;
+    if(!(flags & I2C_NOSTART)) {


a space after if could be added

aabadie · 2018-05-25T12:52:08Z

cpu/sam0_common/periph/i2c.c

-{
-    if (sercom == NULL) {
-        return;
+    if(!(flags & I2C_NOSTOP)) {


a space after if could be added

dylad · 2018-05-26T18:53:40Z

@aabadie I just pushed the required changes.

aabadie

Another set of comments but it looks good in general.

aabadie · 2018-05-27T13:32:01Z

boards/sodaq-explorer/include/periph_conf.h

+        .gclk_src = GCLK_CLKCTRL_GEN_GCLK0,
+        .flags    = I2C_FLAG_NONE,
+     },
+         {


Minor: this opening curly brace is not aligned

aabadie · 2018-05-27T13:34:13Z

cpu/sam0_common/include/periph_cpu_common.h

+    gpio_t sda_pin;         /**< used MOSI pin */
+    gpio_mux_t mux;         /**< alternate function (mux) */
+    uint8_t gclk_src;       /**< GCLK source which supplys SERCOM */
+    uint8_t flags;          /**< allow SERCOM to run in standby mode */


The IRQ management has been removed by the refactoring. Is this intentional ? Do you think it could be set in an attribute of this struct ?

Well it was useless for SAM0 but now you bring this to the stage... I don't know if it could be useful for others arch.
I guess we could still add something later if it's needed right ?

I'm fine with keeping it like this for now.

aabadie · 2018-05-27T13:36:38Z

cpu/sam0_common/periph/i2c.c

-
-    /* HACK: fixes cppcheck issue until #7588 is merged. */
-    if (I2CSercom == NULL) {
+    if (dev >= I2C_NUMOF) {


You could use an assert instead.

assert(dev < I2C_NUMOF);

aabadie · 2018-05-27T13:38:47Z

cpu/sam0_common/periph/i2c.c

-#endif
-        default:
-            return -1;
+    if (dev >= I2C_NUMOF) {


same comment about the assert, there might be others

aabadie · 2018-05-27T13:40:19Z

cpu/sam0_common/periph/i2c.c

-    /* start transmission and send slave address */
-    if (_start(i2c, address, I2C_FLAG_READ) < 0) {
-        return 0;
+    if (!(flags & I2C_NOSTART))


Missing opening {, the if block should be surrounded with {} even if it's a one liner. It improves readability.

Oops ...
Nice catch !

dylad · 2018-05-27T19:01:13Z

@aabadie I've addressed your comments

aabadie

Found one minor remaining thing. You can squash directly when fixed.

aabadie · 2018-05-27T19:21:13Z

cpu/sam0_common/periph/i2c.c

 static inline int _write(SercomI2cm *dev, const uint8_t *data, int length);
 static inline int _read(SercomI2cm *dev, uint8_t *data, int length);
 static inline void _stop(SercomI2cm *dev);
 static inline int _wait_for_response(SercomI2cm *dev, uint32_t max_timeout_counter);
+static void i2c_poweron(i2c_t dev);


sorry, just notice this now: internal static functions should prefixed with an _

dylad · 2018-05-27T19:34:10Z

Squashed

aabadie · 2018-05-27T19:46:25Z

Travis is reporting trailing white spaces issues, can you have a look ?

MrKevinWeiss · 2018-05-28T08:56:26Z

Here are the issues I found while testing this PR with the new periph_i2c test on a samr21.

I found that if you try to acquire twice it waits forever instead of timeout or return with busy message.

It also appears that all read and write calls return I2C_ACK regardless of success. A read/write reg will continue with the second action (after the initial write to reg) regardless of success.

I haven't been able to get a true successful read from my device with this yet (but that may be my problem, I will confirm later).

It seems that the device doesn't need to be acquired to be used, I am not sure if it is intentional or not.

dylad · 2018-05-28T09:13:07Z

I found that if you try to acquire twice it waits forever instead of timeout or return with busy message.

This is because the if (xxxx) return -1; has been replace by an assertion. Maybe it was not a good idea ?

It also appears that all read and write calls return I2C_ACK regardless of success. A read/write reg will continue with the second action (after the initial write to reg) regardless of success

I guess the old driver behaves the same and I still haven't find enough time to handle the error on the bus correctly. Maybe it will be done after the refactoring if I can't fix it quickly.

It seems that the device doesn't need to be acquired to be used, I am not sure if it is intentional or not.

I think we will deal with it right now, because we must implement (in a near future ?) the acquire and release function with clock gating + pm_management. So it will be impossible to use the bus if acquire is not called.

MrKevinWeiss · 2018-05-28T11:11:37Z

cpu/sam0_common/periph/i2c.c

-    if (dev >= I2C_NUMOF) {
-        return -1;
-    }
+    assert(dev < I2C_NUMOF);
    mutex_lock(&locks[dev]);


I like the assert, I assume once everything is ported we can either remove the int return (because all failures would be asserted) or at least update the comments in the i2c.h driver.

Should the mutex_lock be used or the mutex_trylock or should there be an option?

@aabadie what's your opinion about assert vs return err_code ?

+1 for the assert. Briefly comparing with spi API, the spi_init returns void, I think it should be the same for i2c (sorry for noticing this only now).

MrKevinWeiss · 2018-05-29T10:13:54Z

So it appears that the _start fxn both sends the start, addr and automatically gets the first byte from that slave (feature or bug). Disabling the smart mode doesn't seem to effect it and on the register level I can't see why it would do that. Either way it should be fixed since read adds an extra byte (even though it isn't shown on the higher levels, the last byte seems to just remain in the dev->DATA.reg).

For i2c.c

in the fxn:
static inline int _read(SercomI2cm *dev, uint8_t *data, int length)
line 338
while (count != length) { should be while (count != length - 1) {

it seems like the last byte should only be read after the stop command to prevent it from reading another:

int i2c_read_bytes(i2c_t dev, uint16_t addr, void *data, size_t len, uint8_t flags)
line 204 add
((uint8_t*)data[len - 1] = bus(dev)->dev->DATA.reg;

maybe protect it with some
assert(len > 0);

I also don't know how this effects the flags (ie if no start then maybe len should be +1 and if no stop then reading the byte will still trigger another (unwanted) read). I am a bit unclear on the no start functionality, should it send the addr without a start bit or no addr at all?

Also it would be nice to make the return codes match the API (maybe a separate PR).

dylad · 2018-05-29T17:53:37Z

I am a bit unclear on the no start functionality, should it send the addr without a start bit or no addr at all?

If NO_START is NOT selected, then we must send start bit AND the device address.
If NO_START is selected, then we must NOT send start and the devices address.
I'll try to look at it this week but I'm back to work so I don't have much time.
Thanks for the feedback !

dylad · 2018-05-29T19:35:16Z

I'm currently working on a solution for the extra byte but I'm running out of hardware. I'll push some more commits soon !

MrKevinWeiss · 2018-05-30T14:37:37Z

Let me know if I can help. Also I noticed that the i2c_write_regs is not well defined.
Currently it goes
-start-addr-reg0-stop- -start-addr-data...

if the 16 bit flag is set it goes
-start-addr-reg0-reg1-stop -start-addr-data...

This appears to be two different write commands and shouldn't work for any sensors, the first byte of the data usually would be the register byte.
For example on the lis2dh with if I wanted to write 2,3,4 in register 5,6,7 I would go
i2c_write_regs(addr = 12, reg = 2, data = [5,6,7])
I would get:
-start-12<<1-2-stop-
which sets the pointer then
-start-12<<1-5-6-7-stop-
this would actually write 6 and 7 in register 5 not 5 in reg 2, 6 in reg 3, and 7 in reg 4

I think it should either go:
-start-addr-reg0-data... which is the convention for writing to registers in lis2dh and many other ic's
for 16 bit
-start-addr-reg0-reg1-data...

or
-start-addr-reg0-start-data...
but I have never seen this actually done.

Can anyone give me a sensor or slave device that follows either the current riot implementation or the no stop version?

MrKevinWeiss · 2018-05-31T09:00:54Z

Well according to the SAMR21 datasheet page 513 or 514 it appears impossible to support the no stop flag without having an extra unwanted read. I think the closest you can come is that it sends a NACK instead of a ACK after the last byte is read if no stop bit is requested. I would suggest for the read_bytes we just don't support that call with the stop flag.

dylad · 2018-05-31T19:08:16Z

@MrKevinWeiss I just push a fix for the extra read byte without loosing support of NOSTART, NOSTOP
Please let me know if this solve your issue. I tested using a logic analyzer and I don't see any extra read byte now.

Now I'll cleanup some parts and work on the errno return code.

MrKevinWeiss · 2018-06-01T06:37:53Z

cpu/sam0_common/periph/i2c.c

        /* Save data to buffer. */
        data[count] = dev->DATA.reg;

        /* Wait for response on bus. */
        if (_wait_for_response(dev, SAMD21_I2C_TIMEOUT) < 0)
-            return -1;
+            return I2C_ERR;


It seems to always fail here, maybe put a condition that it only checks if it is not the last byte.

MrKevinWeiss · 2018-06-01T06:39:28Z

cpu/sam0_common/periph/i2c.c

    }
-    _stop(I2CSercom);
-    return length;
+    return len;


Should this be ret not len?

MrKevinWeiss · 2018-06-01T06:39:49Z

cpu/sam0_common/periph/i2c.c

-int i2c_read_reg(i2c_t dev, uint8_t address, uint8_t reg, void *data)
-{
-    return i2c_read_regs(dev, address, reg, data, 1);
+    return len;


Should this be ret not len?

MrKevinWeiss · 2018-06-01T06:42:03Z

cpu/sam0_common/periph/i2c.c


+void i2c_init(i2c_t dev)


Why no feedback if the init was successful, I imagine it would alter the program if something failed or is there an GET_INIT_STATE somewhere?

MrKevinWeiss · 2018-06-01T06:44:57Z

@dylad I like moving the functionality to _read, much cleaner, just a few specific things that I have commented on. This fixes the extra read problem if you have a stop bit but the problem is still there if you do not have a stop bit (I2C_NOSTOP). This is an issue with the state machine of the hardware. If you try to read data without the next state being a stop or nack or repeated start it will clk the slave for another byte. The closest I have come is by disabling smart mode and withholding the ACK but it still doesn't seem correct to me.

This is why I suggest we just return error for a read with NOSTOP. The only reason to support a read NOSTOP is if we want to read a certain amount wait (with the scl line held down) then do a NOSTART read later. I don't see this functionality being useful (this also means that we wouldn't need to support a NOSTART since we would never get into the state where it could be used).

jnohlgard · 2018-06-01T07:15:36Z

I had the same situation with the Kinetis hardware. I chose to let the hardware do another read when NOSTOP is set and avoid performing a dummy read to clock the first byte when NOSTART is set. I don't know if it applies to the Sam hardware though

Edit, see https://github.com/gebart/RIOT/blob/pr/kinetis-i2c/cpu/kinetis/periph/i2c.c#L293

dylad · 2018-06-01T07:17:51Z

@MrKevinWeiss I understand what you mean but I would like to keep this level of freedom for the driver.
Of course, if we cannot find a suitable workaround to this limitation we impose this to the driver.
I'm sure we can find a solution to this :) At least, I hope...

MrKevinWeiss · 2018-06-01T08:03:07Z

@gebart Yup seems to be the same situation. If everyone agrees that we can assume anyone calling a read bytes with NOSTART will have previously called read bytes with NOSTOP or NOSTOP | NOSTART then it should be doable. We could even add some static memory or certain reg check to know that it is in an inbetween state, I noticed that without a stop byte it appears that the scl stays low. We could just say if the scl is low there is a valid previous byte in the register that we want.

@dylad Maybe I am getting too obsessy over this. I also don't think saying the sam0_common cpu doesn't support it means that the driver doesn't support it (other cpus could have it). I tend think if something doesn't work perfectly then it should be clearly not supported rather than hiding corner case errors (which end up costing the people using RIOT a lot of time). With information above I will gear my tests to your decision.

keestux · 2018-06-06T19:07:14Z

@dylad I need a little bit more time to test it...

keestux · 2018-06-06T19:11:29Z

cpu/sam0_common/periph/i2c.c

            DEBUG("STATUS_ERR_PACKET_COLLISION\n");
            return -EAGAIN;
        }

        /* Wait for hardware module to sync */
        while (dev->SYNCBUSY.reg & SERCOM_I2CM_SYNCBUSY_MASK) {}

-        DEBUG("Written byte #%i to data reg, now waiting for DR to be empty again\n", buffer_counter);
+        DEBUG("Written byte #%i to data reg, now waiting for DR \
+              to be empty again\n", buffer_counter);


This is not good. The newline is escaped alright, but then there are 14 spaces instead of one.

BTW. The way to solve it is like this

DEBUG("Written byte #%i to data reg, now waiting for DR" " to be empty again\n", buffer_counter);

keestux · 2018-06-06T19:15:11Z

cpu/sam0_common/periph/i2c.c

@@ -281,7 +283,7 @@ static int _start(SercomI2cm *dev, uint16_t addr)

    /* Wait for response on bus. */
    if (addr & I2C_READ) {
-        /* Some devices (e.g. SHT2x) can hold the bus while preparing the reply */
+        /* Some devices can hold the bus while preparing the reply */


The example of SHT2x was added by me. If we leave it out there could be a moment in the future when somebody wants to know why.

I'll change that.

Signed-off-by: dylad <dylan.laduranty@mesotic.com>

dylad · 2018-06-06T19:35:50Z

@keestux I'm really really sorry I squashed the branch before seeing your comments...
I'll fix all your pending comments and wait for your ACK before re-squash.
Again I'm sorry...

aabadie · 2018-06-06T19:38:45Z

I need a little bit more time to test it...

no worry @keestux, if we merge this branch, it will be in the new_i2c_if feature branch. You'll still be able to test this feature branch and provide fixes if you find problems.

When all is adapted (cpu + drivers) and tested, we'll open a PR of the feature branch to master. We hope this will be done in the next 2 weeks.

dylad · 2018-06-06T19:42:08Z

@aabadie We can wait for @keestux's test. I'm fine with that. I'll prepare the nRF52 during this time.

aabadie · 2018-06-06T19:44:17Z

cpu/sam0_common/periph/i2c.c

-    }
-
+    assert(dev < I2C_NUMOF);
+    /* Initiliaze mutex */


There's a typo on 'Initiliaze', should be 'Initialize'

keestux · 2018-06-06T19:52:41Z

@dylad I'm not happy yet. My BME280 driver does not work (gives -EAGAIN). And when I enable debug the error goes away. This smells like a timing problem. And this is just a simple sensor. What other drivers were tested?

Oh, and don't worry about the resquashing

aabadie · 2018-06-06T20:04:29Z

What other drivers were tested?

I tested a hts221 (temperature/humidity) and lsm6dsl (accelero/gyro) from ST mems extension directly plugged on a genuino-zero.
I modified the branch of this PR by merging #9298 and #9299 in it.
They were working well but I don't have a logic analyzer to track what is exchanged on the I2C bus.

dylad · 2018-06-06T20:06:15Z

@keestux I've got one idea. I'll push a potential fix in a few minutes.

dylad · 2018-06-06T20:26:20Z

Found a critical bug when preparing a fix. Sorry but we need to re-test this driver...

keestux · 2018-06-06T20:54:59Z

@dylad Hmm, this is making it worse. Last debug lines are:

Wait for device to be ready
Generate start condition by sending address
Wait for response.
Looping through bytes
Written byte #0 to data reg, now waiting for DR to be empty again

Anyway, for me, time is up for today.

dylad · 2018-06-07T07:47:31Z

@keestux the lastest introduced bug is solved by the last commit.
I think this should also solve your previous issue with your bme280 device.
Let me know if it's OK for you.

aabadie · 2018-06-07T10:57:05Z

I tested the latest version of this branch on arduino-zero with bmp280 (using a fixed version of #9206) and it works.

keestux · 2018-06-07T18:48:05Z

@dylad Great. It's working for me (bme280).
I'll continue to investigate, but it is good that you got this far. Thanks.

keestux · 2018-06-07T18:49:02Z

cpu/sam0_common/periph/i2c.c

-    while ((bus(dev)->STATUS.reg & SERCOM_I2CM_STATUS_BUSSTATE_Msk)
-           != BUSSTATE_IDLE) {}
+   /* while ((bus(dev)->STATUS.reg & SERCOM_I2CM_STATUS_BUSSTATE_Msk)
+           != BUSSTATE_IDLE) {}*/


This should be cleaned up.

Ooh a wild comment !
Will fix

keestux · 2018-06-07T18:52:10Z

cpu/sam0_common/periph/i2c.c

@@ -363,6 +355,10 @@ static inline int _write(SercomI2cm *dev, const uint8_t *data, int length,
            return -EIO;
        }
    }
+    if (stop) {
+        /* Prepare stop command before write last byte */


Is the comment correct? The last byte was already written.
Also, I get the feeling we should do something similar in _read. I mean, do the _stop after the while loop (if it is requested).

The comment probably belongs to the ACK/NACK, which you "prepare" it before doing the last read byte.

Will fix the comment, but we should not touch the current call orders.
Calling the _stop after the while loop in the _read will lead to an extra byte read.
I can add comment within the code to explain the situation if you want

dylad · 2018-06-07T20:32:48Z

@keestux I've updated the comments

keestux · 2018-06-07T20:42:32Z

Let's go for it
Do I need to ACK it somewhere? If not, here is my ACK.

aabadie

Now that you gave your green light @keestux, let's formally ACK this one.

Please squash!

dylad · 2018-06-07T21:03:20Z

Squashed !

aabadie · 2018-06-07T21:11:31Z

So let's go then!

cpu/sam0: update I2C driver to new API

dylad added Area: drivers Area: Device drivers TF: I2C Marks issues and PRs related to the work of the I²C rework task force labels May 25, 2018

aabadie requested changes May 25, 2018

View reviewed changes

haukepetersen mentioned this pull request May 25, 2018

periph/i2c: tracking the remodeling #6577

Closed

47 tasks

aabadie requested changes May 27, 2018

View reviewed changes

aabadie reviewed May 27, 2018

View reviewed changes

dylad force-pushed the i2c_if_sam0 branch from 8fb56d9 to c3c6830 Compare May 27, 2018 19:33

dylad force-pushed the i2c_if_sam0 branch from e981281 to 93d0a6a Compare May 28, 2018 08:32

MrKevinWeiss self-requested a review May 28, 2018 08:47

dylad added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label May 28, 2018

MrKevinWeiss mentioned this pull request May 28, 2018

tests/periph_i2c: Update to new I2C API #9168

Merged

MrKevinWeiss reviewed May 28, 2018

View reviewed changes

MrKevinWeiss mentioned this pull request May 31, 2018

sam0_common/i2c.c extra byte fix #9257

Closed

MrKevinWeiss reviewed Jun 1, 2018

View reviewed changes

keestux reviewed Jun 6, 2018

View reviewed changes

sam0/i2c: update sam0 based periph_conf

db01585

Signed-off-by: dylad <dylan.laduranty@mesotic.com>

dylad force-pushed the i2c_if_sam0 branch from 7f1ad70 to 451f83b Compare June 6, 2018 19:32

aabadie reviewed Jun 6, 2018

View reviewed changes

keestux reviewed Jun 7, 2018

View reviewed changes

aabadie approved these changes Jun 7, 2018

View reviewed changes

sam0/i2c: update driver to new API

cb6a53b

dylad force-pushed the i2c_if_sam0 branch from a949e26 to cb6a53b Compare June 7, 2018 21:02

aabadie merged commit 0d3f10d into RIOT-OS:new_i2c_if Jun 7, 2018

dylad deleted the i2c_if_sam0 branch June 8, 2018 07:12

basilfx pushed a commit to basilfx/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9198 from dylad/i2c_if_sam0

b3a0632

cpu/sam0: update I2C driver to new API

dylad pushed a commit to dylad/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9198 from dylad/i2c_if_sam0

33e2cbd

cpu/sam0: update I2C driver to new API

dylad pushed a commit that referenced this pull request Jul 11, 2018

Merge pull request #9198 from dylad/i2c_if_sam0

1e9924b

cpu/sam0: update I2C driver to new API

cpu/sam0: update I2C driver to new API #9198

cpu/sam0: update I2C driver to new API #9198

Uh oh!

Conversation

dylad commented May 25, 2018 • edited by aabadie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylad commented May 26, 2018

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylad commented May 27, 2018

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylad commented May 27, 2018

Uh oh!

aabadie commented May 27, 2018

Uh oh!

MrKevinWeiss commented May 28, 2018

Uh oh!

dylad commented May 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrKevinWeiss commented May 29, 2018

Uh oh!

dylad commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dylad commented May 25, 2018 •

edited by aabadie

Loading

aabadie May 28, 2018 •

edited

Loading

dylad commented May 29, 2018 •

edited

Loading

jnohlgard commented Jun 1, 2018 •

edited

Loading