RFC SPI: Reference Implementation. #8445

ithinuel · 2018-10-16T14:19:50Z

Description

This PR contains reference implementations of the SPI HAL RFC changes for the FRDM-K66F and NUCLEO_F429ZI boards.

Known issues :

The implementation for the K66F does not support least significant bit first in slave mode. This is due to a hardware limitation that needs to be workedaround in software.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[x] Breaking change

cmonr · 2018-10-16T15:34:16Z

sigh

I think the feature branch needs to be updated.

cmonr · 2018-10-16T15:35:28Z

Ooor maybe this PR needs work? I dunno.

The specific failure in cloud-client-test:

[Build K66F ARM] [Error] PN512SPITransportDriver.cpp@40,0: #308: more than one instance of overloaded function "mbed::SPI::frequency" matches the argument list:
[Build K66F ARM] [ERROR] "./mbed-os/features/nfc/source/controllers/PN512SPITransportDriver.cpp", line 40: Error: #308: more than one instance of overloaded function "mbed::SPI::frequency" matches the argument list:
[Build K66F ARM] function "mbed::SPI::frequency(int)"
[Build K66F ARM] function "mbed::SPI::frequency(std::uint32_t)"
[Build K66F ARM] argument types are: (unsigned long)
[Build K66F ARM] object type is: mbed::SPI
[Build K66F ARM] ./mbed-os/features/nfc/source/controllers/PN512SPITransportDriver.cpp: 0 warnings, 1 error

ithinuel · 2018-10-16T16:49:15Z

/builds/ws/mbed-os-ci_unittests/unitTest-385/mbed-os/features/netsocket/SocketAddress.cpp:22:23: fatal error: ip4string.h: No such file or directory

we might need to rerebase the feature branch indeed.

The command "python tools/make.py -t GCC_ARM -m K82F --source=. --build=BUILD/K82F/GCC_ARM -j0" exited with 1.

and

The command "python tools/make.py -t GCC_ARM -m K64F --source=. --build=BUILD/K64F/GCC_ARM -j0" exited with 1.

These are expected failures because the port of the driver is only valid for the FRDM_K66F and the NUCLEO_F429ZI.

c1728p9 · 2018-10-16T22:29:20Z

drivers/SPI.cpp

@@ -16,97 +16,146 @@
 #include "drivers/SPI.h"
 #include "platform/mbed_critical.h"

-#if DEVICE_SPI_ASYNCH
+#if 0


Why is this code disabled?

This is because the async API in the driver is not very clear on :

Locks :

Shall lock match the lock of the peripheral (thread safety of the peripheral)

Shall lock match the lock of the driver instance to race (thread safety of the instance)

chip select behaviours

How should that behave when chip select is handled in hardware ?

What is the relation between the chip select and the locks ?

c1728p9 · 2018-10-16T22:31:09Z

drivers/SPI.cpp

+    struct SPI::spi_peripheral_s *result = NULL;
+    core_util_critical_section_enter();
+    for (uint32_t idx = 0; idx < SPI_COUNT; idx++) {
+        printf("SPI::lookup(%08x) found at %lu", name, idx);


printf should be removed

c1728p9 · 2018-10-16T22:34:41Z

drivers/SPI.cpp

 }

 int SPI::write(int value)
 {
    lock();
    _acquire();
-    int ret = spi_master_write(&_spi, value);
+    uint32_t ret = 0;
+    spi_transfer(&_self->spi, &value, _bits/8, &ret, _bits/8, NULL);


You'll probably want to round up here.

Indeed ! Fixed :)

c1728p9 · 2018-10-16T22:38:09Z

drivers/SPISlave.cpp

@@ -45,17 +45,17 @@ void SPISlave::frequency(int hz)

 int SPISlave::receive(void)
 {
-    return (spi_slave_receive(&_spi));
+    return 0;//(spi_slave_receive(&_spi));


Why is function commented out? Was the intent to remove it?

It looks like I forgot the commit SPISlave driver changes. This should be ok now.

c1728p9 · 2018-10-16T22:38:54Z

targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_K66F/TARGET_FRDM/PeripheralPins.c

@@ -185,7 +185,7 @@ const PinMap PinMap_SPI_SCLK[] = {
    {NC   , NC   , 0}
 };

-const PinMap PinMap_SPI_MOSI[] = {
+const PinMap PinMap_SPI_SOUT[] = {


Why was this changed from MOSI to SOUT? Same for MISO to SIN below.

This is because on Freescale's targets the pins aren't MISO & MOSI. If you switch from master to slave, the direction of the pin will not be changed to match it's new role.
On Freescale's targets, the SIN is always the input what ever the mode (master or slave) of the peripheral.

deepikabhavnani · 2018-10-22T21:35:25Z

drivers/SPI.cpp

    } else {
        _acquire();
    }
    unlock();
 }

-void SPI::frequency(int hz)
+uint32_t SPI::frequency(uint32_t hz)


Variable/Parameter types should be consistent in driver. Existing driver uses int, char.. etc, changed here for few API's to uint32_t uint8_t and not all. I would expect API's to be consistent in a driver.

Also if we are changing the return type, should we deprecate the existing one? In case of format for addition of new parameter we are adding new API, but old is not deprecated and for frequency we are changing the return type and parameter type of API without any deprecation.

The changes to this driver are provisional and meant to be minimal.
Any change to bring consistency may break existing API.

We cannot deprecate the void returning version of SPI::frequency because return types are not "overloadable". Also, going from void to uint32_t do not present risk on ignoring this return value.

kjbracey

The bus-separating logic here will be pretty vital - we're currently hitting problems with SD card traffic interfering with 802.15.4 radios on a different bus, and this will solve it.

(I had knocked up a quick hack to confirm this, and came back to check what the state was with this PR).

kjbracey · 2018-11-05T12:30:40Z

drivers/SPI.h

+    struct spi_peripheral_s {
+        SPIName name;
+        spi_t spi;
+        PlatformMutex *mutex;


Would it not be easier to make this a SingletonPtr<PlatformMutex>?

That's probably pushing too much stuff into the static allocation though. After more thought, this seems like a bad hybrid. There is a benefit to being fully-static, but if you're going to start doing some dynamic allocation, it may as well be fully dynamic to minimise overhead. I think rather than this static array, it would be better just to have the head of a linked list of peripherals, each of which with an embedded PlatformMutex and spi_t, so one heap block per peripheral.

Ideally I would want the PlatformMutex to be statically allocated but AFAIU this is causing issues with the constructor not being invoked when instantiated from a static array..

kjbracey · 2018-11-05T12:30:45Z

drivers/SPI.h

+    // Drawback: it costs ram size even if the device is not used.
+    static spi_peripheral_s _peripherals[SPI_COUNT];
+
+    spi_peripheral_s *_self;


_self as a name is far too vague - it should surely be _peripheral.

This is a little bikeshedding but I picked _self because :

_peripheral do not reflect enough that this member is the peripheral associated to this SPI instance.

_this would have been ambiguous with this.

_peripheral do not reflect enough that this member is the peripheral associated to this SPI instance.

I'm not sure how it could be any clearer. It tells us it's a/the peripheral, and obviously it's associated with this SPI instance, because our SPI object pointing to it. It's this->_peripheral.

SPI is the object representing an SPI slave we're talking to, and then we have a pointer to the peripheral controlling it.

this->_self reads to me just the same as saying this->_this.

kjbracey · 2018-11-05T12:31:54Z

drivers/SPI.h

@@ -74,9 +77,14 @@ namespace mbed {
 * @ingroup drivers
 */
 class SPI : private NonCopyable<SPI> {
-
+protected:


Why protected? Is this intentionally exposed as API?

Because the member variables using this type are protected too.

_self? Does that need to be protected rather than private? You want to permit derived classes to mess with it? I'd be inclined to keep it private until there's a definite need to reveal it.

(Watching all the "protected exclusion" markers being applied for Doxygen recently has brought home to me how much excess protected there is. protected is often unintentionally exposed API. Either it shouldn't be protected, or it should be documented (as @internal if necessary.)

kjbracey · 2018-11-05T12:37:53Z

drivers/SPI.cpp

+    // unless not assigned to a peripheral, this mutex will never be deleted.
+    PlatformMutex *mutex = new PlatformMutex();
+
+    core_util_critical_section_enter();


Critical section seems heavy, and I'm wondering if that's tying your hands from using SingletonPtr for the PlatformMutex.

You don't need your own mutex, you could just borrow singleton_lock from SingletonPtr, which is used by that and all other static initialisation.

drivers/SPI.cpp

kjbracey · 2018-11-05T12:59:08Z

drivers/SPI.cpp

+    }
+    if (_self->name == 0) {
+        _self->name = name;
+        spi_init(&_self->spi, false, mosi, miso, sclk, NC);


ssel is now being totally ignored. I still think that's a retrograde step - it is notionally within the bounds of existing platform-specific behaviour, but it would surely be better to have lock manipulate ssel as a GPIO.

AFAIK, we're already in the happy state where all portable code already passes NC as ssel and does it manually because of the variable behaviour of platforms, so we now have the room to start making it work.

On further reflection, probably best to not put it in actual lock, as we do have a common pattern where lock is just a "peripheral lock", and it's virtual so that people can inherit and override, in particular for bypassing the mutex for use from interrupt, if they know they're the only user of the peripheral.

There are some UnlockedSPI out there that inherit from SPI for use from IRQ, overriding lock+unlock. Shouldn't break that pattern.

Instead, having select and deselect methods that are lock; acquire; cs=1 (reference counted) would be the way to go - that would still work when someone overrides lock to eliminate the peripheral mutex.

My bad, this NC should be ssel.

Any work regarding locks/selections is out of scope from this PR though.

It has to be NC - can't be anything else or the entire "peripheral" concept won't work. That inherently prevents any use of the HAL ssel, because you're sharing spi_t for the same bus.

Well, I guess you /could/ pass ssel to the HAL, but only if you did it on every acquire. But that means a full init every time you change owner of a peripheral, I guess?

Any work regarding locks/selections is out of scope from this PR though.

From my fairly prolonged personal experience of using the SPI API, the two biggest problems are the system-wide transfer mutex, and the total uselessness of the SSEL parameter. (Lack of trust of the async API implementations would be a runner-up)

Great that you're fixing the first one - which is also a purely C++ layer and non-HAL thing - but I don't see how you can do any sort of SPI rework that doesn't address the SSEL problem. If it's not going to be addressed when during an SPI API rework, when is it going to be addressed?

The peripheral change as you've written it here is effectively is going to force some SSEL change. You're going to have to do something. Any of these are possible:

render SSEL totally non-functional for all platforms, as the current PR, everyone does SSEL manually via GPIO, as portable code already does anyway.

rework the acquire mechanism to do spi_init to change SSEL

give up the shared spi_t

put the GPIO for SSEL inside SPI - doesn't break anyone with non-portable code relying on the SPI SSEL, and the portable code can be cleaned up to take out the manual handling.

I think the last is by far the best combination of easy+beneficial.

kjbracey · 2018-11-05T13:04:03Z

drivers/SPI.cpp

 }

 int SPI::write(int value)
 {
    lock();
    _acquire();
-    int ret = spi_master_write(&_spi, value);
+    uint32_t ret = 0;
+    spi_transfer(&_self->spi, &value, (_bits+7)/8, &ret, (_bits+7)/8, NULL);


This only works little-endian. If attempting portability you need a 3-way switch using uint8_t, 1;uint16_t, 2; or uint32_t, 4.

Although, do we ever care? Not sure mbed OS supports big-endian at all, or have any plans to do so. It's just that the HAL api said something about platform endianness, suggesting coping with either.

IMHO this method should be deprecated as it encourages reading symbols per symbol which is really bad for performances.
This fallback is provided for compatibility.

I assume this will be second step - another PR (deprecation note for this byte oriented methods) ?

Wouldn't disagree with this - it's a convenient short-hand, but other APIs like Socket and FileHandle only give you the block form. If people really need byte-wise stuff, they can probably wrap a helper up themselves.

kjbracey · 2018-11-05T13:05:05Z

drivers/SPI.cpp

@@ -115,29 +162,29 @@ int SPI::write(const char *tx_buffer, int tx_length, char *rx_buffer, int rx_len
 {
    lock();
    _acquire();
-    int ret = spi_master_block_write(&_spi, tx_buffer, tx_length, rx_buffer, rx_length, _write_fill);
+    int ret = spi_transfer(&_self->spi, tx_buffer, tx_length, rx_buffer, rx_length, &_write_fill);


Again, this use of _write_fill assumes little endian. Possibly simplest to have a 3-way uint8_t/uint16_t/uint32_t union to store it.

kjbracey · 2018-11-05T13:37:10Z

drivers/SPISlave.cpp

+        spi_transfer_async_abort(&_spi);
+        _is_pending = false;
+    }
+    _is_pending = spi_transfer_async(&_spi, &value, 1, (void *)&_buffer, 1, &_dummy, &SPISlave::irq_handler, this, (DMAUsage)0);


More illegal-if-big-endian stuff.

kjbracey · 2018-11-05T13:38:29Z

drivers/SPI.h


 protected:
-    spi_t _spi;
+    // holds spi_peripheral_s per peripheral on the device.
+    // Drawback: it costs ram size even if the device is not used.


This shouldn't be costing space if SPI isn't used at all, I hope. I guess we can live with using more than necessary for buses that aren't used.

kjbracey

Okay, all my issues on this are to do with the state of C++ SPI class - I've got no issue with the HAL specification or implementation. As far as I can see all the tools are in place to get SPI working the way I think it should be. (And that would include not actually using the HAL SSEL, to have maximum flexibility for multiple devices on one bus). I believe it should also be possible to resurrect async with proper peripheral ownership management with current HAL API.

Therefore I'll approve this to allow HAL development to proceed, but more work on SPI and its APIs will be required before going to master. I will try to find time to put forward my own SPI-refining PR for this feature branch.

0xc0170 · 2018-11-09T13:46:29Z

Thanks @kjbracey-arm ! Looking forward for SPI-refining

0xc0170 · 2018-11-30T12:19:25Z

@ithinuel The branch was recently rebased, can you update this one (there's one conflict as well now)

Co-authored-by: Wilfried Chauveau <[email protected]> Co-authored-by: Przemyslaw Stekiel <[email protected]>

…PI within the SoC

mbed-ci · 2018-12-28T17:01:04Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 27
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

deepikabhavnani · 2018-12-31T19:05:25Z

targets/TARGET_Silicon_Labs/TARGET_EFM32/TARGET_EFM32PG/PeripheralPins.c

@@ -198,6 +198,7 @@ MBED_WEAK const PinMap PinMap_PWM[] = {
 };

 /*************SPI**************/
+#if SPI_DEVICE


Should it be DEVICE_SPI?

Good catch, thanks @deepikabhavnani!

@donatieng - All the #if changes are relevant for master branch as well, it will be good to move common changes to master branch instead feature branch to have minimal conflicts in future, just a suggestion not blocker.

mprse · 2019-01-03T11:29:20Z

Analysis of the failing tests:
https://mbed-os.mbedcloudtesting.com/blue/organizations/jenkins/mbed-os-ci_greentea-test/detail/mbed-os-ci_greentea-test/658/tests/

List of the failed tests below:

Test	Board	Compiler
features-storage-tests-blockdevice-general_block_device	K82F, K64F	ARM, GCC_ARM, IAR
features-device_key-tests-device_key-functionality	K82F, K64F	ARM, GCC_ARM, IAR
features-storage-tests-kvstore-static_tests	K82F, K64F	ARM, GCC_ARM, IAR
features-storage-tests-kvstore-general_tests_phase_1	K82F, K64F	ARM, GCC_ARM, IAR
features-storage-tests-kvstore-securestore_whitebox	K82F, K64F	ARM, GCC_ARM, IAR
tests-psa-prot_internal_storage	K64F	ARM, GCC_ARM, IAR

I tried to run the tests on master branch (K64F/ARM) and compare the results with and without SD component which is now disabled for K64F by this PR. I got the results consistent with above failures:

Test	SD	FLASHIAP	Result
features-storage-tests-blockdevice-general_block_device	yes	yes	failed*
features-device_key-tests-device_key-functionality	yes	yes	passed
features-storage-tests-kvstore-static_tests	yes	yes	passed
features-storage-tests-kvstore-general_tests_phase_1	yes	yes	passed
features-storage-tests-kvstore-securestore_whitebox	yes	yes	passed
tests-psa-prot_internal_storage	yes	yes	passed

features-storage-tests-blockdevice-general_block_device	no	yes	failed
features-device_key-tests-device_key-functionality	no	yes	failed
features-storage-tests-kvstore-static_tests	no	yes	failed
features-storage-tests-kvstore-general_tests_phase_1	no	yes	failed
features-storage-tests-kvstore-securestore_whitebox	no	yes	failed
tests-psa-prot_internal_storage	no	yes	failed

features-storage-tests-blockdevice-general_block_device	no	no	passed
features-device_key-tests-device_key-functionality	no	no	skipped
features-storage-tests-kvstore-static_tests	no	no	passed
features-storage-tests-kvstore-general_tests_phase_1	no	no	passed
features-storage-tests-kvstore-securestore_whitebox	no	no	skipped
tests-psa-prot_internal_storage	no	no	passed**

* - features-storage-tests-blockdevice-general_block_device test has failed on master, but I checked that it regularly passes the CI test runs. Maybe there is some SD card issue on my side.

** - also PSA support needs to be disabled since it requires storage.

Conclusion:
It looks like there is a general problem on K64F (and K82F) with storage tests when SD Card support is disabled and Flash is used. I will create issue for this problem, but these failures are not related to spi feature branch, so I suggest do disable also FLASHIAP for now. Above results proves that this should solve the problem. @donatieng Fix can be found here: PR https://github.com/ithinuel/mbed-os/pull/8.

When "SD" component is unavailable the following tests try to use Flash as storage and fail: features-storage-tests-blockdevice-general_block_device features-device_key-tests-device_key-functionality features-storage-tests-kvstore-static_tests features-storage-tests-kvstore-general_tests_phase_1 features-storage-tests-kvstore-securestore_whitebox tests-psa-prot_internal_storage Additional investigation is required to find the reason of the failures.

donatieng · 2019-01-03T12:00:02Z

Thanks @mprse for the detailed investigation, we'll have to track the storage issue.

@ARMmbed/mbed-os-maintainers this is now ready for CI (again ;))

0xc0170 · 2019-01-03T12:45:29Z

CI started

mbed-ci · 2019-01-03T17:31:51Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 28
Build artifacts

donatieng · 2019-01-03T17:35:41Z

Woohoo!

ithinuel · 2019-01-03T18:04:20Z

🕺

This commit takes some of the work done on the SPI class from ARMmbed#8445, and refines it, to provide the per-peripheral mutex functionality. This also implements GPIO-based SSEL, which exposes a new select()/deselect() API for users to group transfers, and should work on every platform (unlike the HAL-based SSEL). This requires users to use a new constructor to avoid backwards compatibility issues. To activate the per-peripheral mutex, the HAL must define SPI_COUNT and provide spi_get_peripheral_name(). (In ARMmbed#8445 this is a reworked spi_get_module, but the name is changed here to avoid a collision with existing HALs - this commit is designed to work without wider HAL changes). Fixes: ARMmbed#9149

ithinuel changed the title ~~Rfc spi overhaul ref impl~~ RFC SPI: Reference Implementation. Oct 16, 2018

cmonr added the needs: work label Oct 16, 2018

c1728p9 suggested changes Oct 16, 2018

View reviewed changes

cmonr added the release-version: feature branch label Oct 17, 2018

deepikabhavnani reviewed Oct 22, 2018

View reviewed changes

kjbracey suggested changes Nov 5, 2018

View reviewed changes

kjbracey approved these changes Nov 9, 2018

View reviewed changes

0xc0170 force-pushed the feature-hal-spec-spi branch from 689890e to 85413ff Compare November 12, 2018 17:38

cmonr mentioned this pull request Nov 15, 2018

Add SPI basic test and test header file. #7976

Merged

c1728p9 approved these changes Nov 27, 2018

View reviewed changes

mprse mentioned this pull request Nov 29, 2018

Add Ice Tea SPI communication test. #8443

Merged

0xc0170 force-pushed the feature-hal-spec-spi branch from 85413ff to a290cfe Compare November 29, 2018 11:19

cmonr added needs: CI and removed needs: work labels Nov 29, 2018

mprse mentioned this pull request Nov 30, 2018

Add one board Green Tea SPI Communication Test #8919

Merged

0xc0170 requested a review from donatieng November 30, 2018 12:19

cmonr added needs: work and removed needs: CI labels Dec 1, 2018

cmonr force-pushed the feature-hal-spec-spi branch from a290cfe to 0892667 Compare December 4, 2018 22:23

ithinuel and others added 5 commits December 6, 2018 11:49

disable SPI for all targets but K66F & NUCLEO_F429ZI

d47558e

update HAL API

9416182

update drivers

b40f6e1

reference implementation for K66F

9519e0e

Co-authored-by: Wilfried Chauveau <[email protected]> Co-authored-by: Przemyslaw Stekiel <[email protected]>

reference implementation for NUCLEO_F429ZI

ed1505a

Co-authored-by: Wilfried Chauveau <[email protected]> Co-authored-by: Przemyslaw Stekiel <[email protected]>

donatieng approved these changes Dec 6, 2018

View reviewed changes

Remove support for 802.15.4 PHY from KW24D as it is connected using S…

fd248fb

…PI within the SoC

deepikabhavnani reviewed Dec 31, 2018

View reviewed changes

Fix SPI_DEVICE <> DEVICE_SPI

567fd31

0xc0170 added needs: CI and removed needs: work labels Jan 3, 2019

adbridge added ready for merge and removed needs: CI labels Jan 3, 2019

adbridge merged commit f3429a3 into ARMmbed:feature-hal-spec-spi Jan 3, 2019

adbridge removed the ready for merge label Jan 3, 2019

kjbracey mentioned this pull request Jan 23, 2019

SPI upgrade - per-peripheral mutex and GPIO-based SSEL #9469

Merged

kjbracey mentioned this pull request Feb 26, 2019

Add spi_get_peripheral_name() to stm_spi #9845

Merged

RFC SPI: Reference Implementation. #8445

RFC SPI: Reference Implementation. #8445

Conversation

ithinuel commented Oct 16, 2018

Description

Known issues :

Pull request type

cmonr commented Oct 16, 2018

cmonr commented Oct 16, 2018

ithinuel commented Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ithinuel Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey left a comment

Choose a reason for hiding this comment

0xc0170 commented Nov 9, 2018

0xc0170 commented Nov 30, 2018

mbed-ci commented Dec 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprse commented Jan 3, 2019 • edited Loading

donatieng commented Jan 3, 2019

0xc0170 commented Jan 3, 2019

mbed-ci commented Jan 3, 2019

donatieng commented Jan 3, 2019

ithinuel commented Jan 3, 2019

ithinuel commented Oct 16, 2018 •

edited

Loading

ithinuel Nov 6, 2018 •

edited

Loading

kjbracey Nov 6, 2018 •

edited

Loading

kjbracey Nov 6, 2018 •

edited

Loading

mprse commented Jan 3, 2019 •

edited

Loading