Add ConditionVariable to mbed rtos #3648

c1728p9 · 2017-01-26T09:21:48Z

Add the ConditionVariable class to mbed rtos to provide easier and safer synchronization.

c1728p9 · 2017-01-26T09:24:47Z

CC: @kjbracey-arm, @0xc0170, @bulislaw, @RobertRostohar , @sg-

c1728p9 · 2017-02-08T17:49:21Z

Closing this for now due to lack of interest. If this becomes relevant I'll re-open.

kjbracey · 2017-02-09T07:12:33Z

Still think I'm going to want it, but it'll be for a post-5.4 feature / during 5.5.

hasnainvirk · 2017-04-07T07:36:23Z

@c1728p9 @sg- We would appreciate if this PR is opened again. This will improve the quality of our cellular work a lot ..

pan- · 2017-04-07T08:32:42Z

rtos/Mutex.cpp

 }

 bool Mutex::trylock() {
-    return (osMutexWait(_osMutexId, 0) == osOK);
+    if (osMutexWait(_osMutexId, 0) == osOK) {


This function can be factored to a call to lock(0).

pan- · 2017-04-07T08:37:27Z

rtos/Mutex.cpp

 }

 osStatus Mutex::unlock() {
+    count--;


count should be decremented if the calling context is thread mode and the calling thread already owns the mutex (when osMutexRelease will succeed).

pan- · 2017-04-07T08:51:33Z

rtos/ConditionVariable.cpp

+
+namespace rtos {
+
+#define RESUME_SIGNAL      (1 << 15)


Is there a way to avoid the reservation of a signal ID or ensure user code won't use this signal ?

CMSIS-RTOS2 gives event flags, could use those.

pan- · 2017-04-07T08:59:26Z

rtos/ConditionVariable.cpp

+}
+
+void ConditionVariable::notify() {
+    MBED_ASSERT(_mutex.is_owner());


Is it possible to return an error instead if this precondition is false ?

Could do, but would make it harder to debug. I strongly favour asserts for programming errors, rather than the two bad choices of adding run-time code to catch and report a returned error that should never happen, or not bother to check for errors.

For comparison - do you think memcpy should return an error code for bad pointers, and people should check for the error return on every memcpy?

@kjbracey-arm I completely support your point and also prefer asserts when user inputs doesn't respect the function contract. However RTOS primitives in RTX (and mbed OS) never assert when a precondition is false, they return errors instead. I believe it is important to remains consistent with the API exposed by the RTOS module.

Sadly, I do somewhat buy the consistency argument. Do the other calls have only precondition errors, or or they real runtime errors as well?

But I doubt I'd ever add code to check the return code if using this API - I'd know this one was precondition errors only, and that I got the preconditions right.

They have both: precondition and runtime error. For example, Mutex::lock will return an error if the call is made in interrupt context or the Mutex object is invalid those are preconditions error. It also return run-time error if the mutex could not be obtained in the given time.

I'm +1 for consistency. This class should be conforming to the style present in the rtos classes.

However, we can create a seperate push to add asserts through the rtos classes. Though this should be handled in a seperate pr.

pan- · 2017-04-07T09:01:44Z

rtos/ConditionVariable.cpp

+}
+
+void ConditionVariable::notify_all() {
+    MBED_ASSERT(_mutex.is_owner());


Is it possible to return an error instead if this precondition is false ?

pan- · 2017-04-07T09:26:45Z

rtos/ConditionVariable.h

+    ConditionVariable(Mutex &mutex);
+
+    /** Wait for a notification
+      @param   millisec  timeout value or 0 in case of no time-out. (default: osWaitForever)


Please indicate that mutex should be acquired before this call.

pan- · 2017-04-07T09:27:16Z

rtos/ConditionVariable.h

+
+    /** Notify one waiter on this condition variable that a condition changed.
+     */
+    void notify();


Please indicate that mutex should be acquired before this call.

Is it possible to rename the function into notify_one ?

signal would be better - it's the posix/C/C++ name.

Just a precision about names for the different environments:

pthreads: signal / broadcast

C11: signal / broadcast

C++: notify_one / notify_all

I'm agnostic on the naming but be consistent (that's why I suggested notify_one but the pair of function can be renamed to follow c11/pthreads).

Ah, thanks. Hadn't actually checked C++11, I just assumed it was aligned with C11 (they mostly are).

Needs to be consistent with one of those. I think I prefer notify_one and notify_all.

pan- · 2017-04-07T09:27:21Z

rtos/ConditionVariable.h

+      @param   millisec  timeout value or 0 in case of no time-out. (default: osWaitForever)
+      @return  true if woken from notification, false if timed out.
+     */
+    bool wait(uint32_t millisec=osWaitForever);


Please indicate that mutex should be acquired before this call.

pan- · 2017-04-07T09:27:35Z

rtos/ConditionVariable.h

+    /** Notify all waiters on this condition variable that a condition changed.
+     *
+     */
+    void notify_all();


Please indicate that mutex should be acquired before this call.

And broadcast would be the standard name for this one.

pan- · 2017-04-07T09:29:39Z

rtos/ConditionVariable.h

+    void add_wait_list(Waiter * waiter);
+    void remove_wait_list(Waiter * waiter);
+    Mutex &_mutex;
+    Waiter *wait_list;


Could you declare (but not define!) private copy constructor and private assignment operator here.
That would disallow copy of the ConditionVariable class.

kjbracey · 2017-04-07T09:55:40Z

rtos/ConditionVariable.cpp

+
+#define RESUME_SIGNAL      (1 << 15)
+
+class Waiter {


(I'd make it a struct though).

kjbracey · 2017-04-07T09:57:35Z

rtos/ConditionVariable.cpp

+
+    // Unlock mutex
+    uint32_t count = _mutex.count;
+    for (uint32_t i = 0; i < count; i++) {


Not sure this loop is needed. Condition variables are normally documented such that you they only release once, so it is required that you hold it only once. Wonder if there would be another weird side-effect of letting this be done. I'd add the count check to the assert.

kjbracey · 2017-04-07T10:00:40Z

rtos/ConditionVariable.cpp

+}
+
+void ConditionVariable::notify() {
+    MBED_ASSERT(_mutex.is_owner());


Could do, but would make it harder to debug. I strongly favour asserts for programming errors, rather than the two bad choices of adding run-time code to catch and report a returned error that should never happen, or not bother to check for errors.

For comparison - do you think memcpy should return an error code for bad pointers, and people should check for the error return on every memcpy?

kjbracey · 2017-04-07T10:12:42Z

rtos/ConditionVariable.h

+
+    /** Notify one waiter on this condition variable that a condition changed.
+     */
+    void notify();


signal would be better - it's the posix/C/C++ name.

kjbracey · 2017-04-07T10:13:18Z

rtos/ConditionVariable.cpp

+void ConditionVariable::notify() {
+    MBED_ASSERT(_mutex.is_owner());
+    if (wait_list != NULL) {
+        osSignalSet(wait_list->thread, RESUME_SIGNAL);


Possibly. Is that portably possible via CMSIS-RTOS? Better would be to implement it natively in CMSIS-RTOS/RTX, of course. It's not possible to implement a perfect condition variable on top of other primitives.

kjbracey · 2017-04-07T10:15:02Z

rtos/ConditionVariable.cpp

+    if (NULL == wait_list) {
+        // Nothing in the list so add it directly.
+        // Update prev pointer to reference self
+        wait_list = waiter;


FYI there's a rock-solid and efficient typesafe generic doubly-linked list implementation in ns_list.h, but it's hidden behind FEATURE_COMMON_PAL.

kjbracey · 2017-04-07T10:17:02Z

rtos/ConditionVariable.h

+    /** Notify all waiters on this condition variable that a condition changed.
+     *
+     */
+    void notify_all();


And broadcast would be the standard name for this one.

c1728p9 · 2017-10-26T18:53:08Z

Made the following changes:

Rebased to master
Added underscore to private variables
Renamed to match C++ (notify -> notify_one)
Added a note to indicate that calling thread must hold mutex
Changed Waiter class to struct

Still left to do:

Add tests
Add example of use in doxygen
Docs PR
Update to use new event flags class

kjbracey · 2017-10-27T07:27:34Z

rtos/ConditionVariable.h

+     *
+     * @note - The thread calling this function must be the owner of the ConditionVariable's mutex
+     */
+    bool wait(uint32_t millisec=osWaitForever);


Can we have wait_until? It's what you're more likely to need as you're doing wait in a loop, so waiting for a certain time has to be done like this:

bool wait_for_my_condition(uint32_t for_ticks) { if (my_condition) { return true; } uint32_t end_time = osKernelGetTickCount() + for_ticks; do { if (!cv.wait_until(end_time)) { return my_condition; } } while (!my_condition); return true; }

For comparison C++11 has wait, wait_until and wait_for; C11 and pthreads have wait and timedwait="wait until", with no "wait for".

Currently there aren't any C++ RTOS APIs to get the kernel tick. Because of this this couldn't be used with the C++ RTOS API alone. This would be better off as a future addition when a way to get the tick count from the C++ RTOS API is available.

Before that can be done though, the relation between OS tick and millisecond needs to get sorted out. All the existing RTOS drivers are written on the assumption that 1ms = 1 tick. If this changes in the future, waiting until an absolute time could become a lot more complex, as uint32_t ticks and uint32_t milliseconds will overflow at different points. For relative delays this is not as much of a problem since you do not need to worry about rollover. CC @pan-, @0xc0170

I'll add the wait_for and remove the parameter from wait but wait_until will need to come in a separate PR.

My main reservation is that I don't see how wait_for is really useable either without a way of getting the current time. If no C++ time API is an argument against wait_until, it's just as strong an argument as wait_for.

NSAPI already has this bug that its timed socket blocks don't actually block for the time requested, due to its pattern of repeated inner semaphore waits. But that was quite an unusual usage of semaphores.

Here, it's a requirement that users have the CV loop, and giving them only wait_for without the infrastructure to get their intended total wait correct seems like an invitation to "put bug here". As if wait_for wasn't that already - hence its total absence from pthreads and C11.

Personally I'm not a fan of RTOS APIs trying to present milliseconds - serious users will need to understand tick granularity and how to round, and specify in actual ticks, If you were to ever break the assumption 1 tick = 1 ms, then I think you'd need to have a proper tick API - you couldn't stay with just the millisecond API. (I spent a while with Seppo reworking Nanostack's timing API on that basis earlier this year - the "until" API is in native ticks only, avoiding wrapping problems).

c1728p9 · 2017-10-28T17:24:56Z

Made the following changes

Added tests
Added example of use in doxygen
Docs PR

As for the EventFlags update:

I ran into problems trying to update this class to use EventFlags. For the required properties of a condition variable to be met (see the defined behavior section in the doxygen of this PR) there needs to be a way to wake each thread individually. Using a single instance of EventFlags only allows at most 31 threads to be individually woken.

One alternative that would allow this to work is to create an EventFlags class on the stack for each thread that is waiting. This type of solution was already available before the EventFlags class as a Semaphore could be used safely for this purpose. For efficiency the built in thread flags was used instead.

In summary, the code could be changed to use Semaphore/EventFlags instead of thread flags at the expense of increased stack usage and runtime creation of a Semapore or EventFlags on each call to wait/wait_for. @pan-, @kjbracey-arm do you have any preferences for what should be done here or alternative solutions?

c1728p9 · 2017-10-28T19:22:26Z

/morph build

kjbracey · 2017-10-30T07:54:09Z

I don't worry too much about stack space - any wait structure is the last thing on the stack while waiting; not the sort of thing adds up. Only about 32 bytes tops, anyway? If construction overhead is significant, that might be a big deal I guess, but again, it's only when actually waiting.

Also, spurious wake-up is normally allowed by condition variables - you could cop out with the shared EventFlags and just say if you ever do go over 30 simultaneous waiters, the 31st onwards will all end up waking simultaneously with the 30th. Not as if that would ever happen in practice, but does mean a code path you need to test. :(

I think I'd be inclined to keep it simple and use an on-stack Semaphore/EventFlags (whichever is cheaper in CPU cost).

I am a little bit wary about you over-defining the defined behaviour. If you ever found yourself on an RTOS with native condition variables, I'd hate it to be the case that ConditionVariable couldn't actually change to use it, because the native one didn't do the non-standard stuff like "unlock multiple times".

mbed-ci · 2017-10-30T08:19:54Z

Build : SUCCESS

Build number : 365
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/3648/

Triggering tests

/morph test
/morph uvisor-test

mbed-ci · 2017-10-30T09:21:23Z

Test : FAILURE

Build number : 177
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/3648/177

c1728p9 · 2017-10-30T15:32:38Z

I am a little bit wary about you over-defining the defined behaviour. If you ever found yourself on an RTOS with native condition variables, I'd hate it to be the case that ConditionVariable couldn't actually change to use it, because the native one didn't do the non-standard stuff like "unlock multiple times".

Makes sense to me. What behavior would you like to see moved into the undefined category? There is a task for contractors to write tests for all of the defined behavior, so if any of it needs to be removed, now is the time. CC @bulislaw

Also, I'll update the implementation to use Semaphores/EventFlags on the stack.

c1728p9 · 2017-11-03T15:36:35Z

The bulk of this PR has been reviewed and is now waiting until #5419 is merged so wait_until can be added.

kjbracey · 2017-11-06T13:32:37Z

Okay, it seems that we're currently in a situation where we aren't actually stable on what timestamps for "until" look like, so I think this is going to have to proceed without wait_until.

mbed-ci · 2017-11-06T14:06:07Z

Build : SUCCESS

Build number : 442
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/3648/

Triggering tests

/morph test
/morph uvisor-test

mbed-ci · 2017-11-06T15:21:45Z

Test : FAILURE

Build number : 250
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/3648/250

0xc0170 · 2017-11-06T15:56:57Z

I fetched the binaries for the test failure, and running multiple times (using mbedhtrun) queue and stats tests (ARM toolchain failures), all OK.

I tried another job, with similar failures, same result 😖

mbed-ci · 2017-11-06T19:39:32Z

Test : SUCCESS

Build number : 254
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/3648/254

0xc0170 · 2017-11-07T10:30:45Z

/morph uvisor-test

0xc0170

Looks good to me. I left only cosmetic comments.

Mutex changes should be in a separate commit (ConditionVariable would be built on top of it).

0xc0170 · 2017-11-07T10:42:53Z

rtos/ConditionVariable.h

+    ~ConditionVariable();
+
+private:
+    void _add_wait_list(Waiter * waiter);


Should also private methods to be documented? I would say yes (of course excluded from published docs).

I figured this is fairly self explanatory, but I can add docs if you think others will find this confusing.

0xc0170 · 2017-11-07T10:47:34Z

TESTS/mbedmicro-rtos-mbed/condition_variable/main.cpp

+}
+
+Case cases[] = {
+    Case("Test notify one", test_notify_one),


didn't we agree on having tests documented (this test unit would contain a header file with documentation) ?

This is only for the HAL specification which will be transcluded into our docs. CC @bulislaw @pan-

0xc0170 · 2017-11-07T10:51:44Z

rtos/Mutex.cpp

@@ -27,7 +27,7 @@

 namespace rtos {

-Mutex::Mutex()
+Mutex::Mutex(): _count(0)


this change should be in a separate commit with own decription (friend class there to get the count - part of condition variable change, the rest would be a separate commit - Add counter to Mutex because.... I would question if getter method would not be better, fine as it is).

As I expressed in a previous comment, I think having public "get_count" is dangerous, as it's a programming trap. No correctly-written code needs it, and recursive mutex implementations don't conventionally provide it, which is why it's being manually tracked here.

The current ConditionVariable implementation doesn't really need it - it's there only to support a helpful MBED_ASSERT - detecting the programming error of calling ConditionVariable:wait with a count other than 1.

@c1728p9 countered my reservation about having get_count by noting that it was exposed only via friend. (I think a previous version had a private getter function, and still with friend?).

My personal preference would be to avoid the friend thing, and make it public, but conditional on !NDEBUG to make clear that it is a for-diagnostic-only API. I can see being able to access that information is useful for diagnostics and debugging, but really don't want any danger of any real code trying to do anything based on the count. Or maybe we could provide it but "deprecated" thing if NDEBUG?

The first draft of ConditionVariable used the count by "helpfully" unlocking and relocking the mutex N times where N was the current count, but that is not a good idea, as explained here: https://groups.google.com/forum/#!msg/comp.programming.threads/B0JE0_b50Ww/8ekMG09xewsJ

That feature was the original reason for adding the mutex count, but now it's just for that assert.

My concern was regarding having related but possibly logically separated changes in one commit without any background (your comment helps now to get better picture, thanks).

It is fine as it is as stated above, should not hold this any longer

Just wanted to push back on the getter method. Would agree on separating the commit.

I can move this to a separate commit if you would like but I don't really see the purpose of this. Without ConditionVariable _count is incremented and decremented, but never used and cannot be used as it is private. Furthermore, it isn't even helpful for git operations, as reverting it breaks condition variable. Alternatively reverting ConditionVariable leaves unused code. @0xc0170 let me know if you still want me to split this into two commits, as testing will need to be rerun.

@c1728p9 Agree. I was missing the context that you and Kevin provided today. If you think some of those details can be captured in the commit msg (like the relation between extending Mutex class by count for ConditionVariable), you can add them.

My above 2 comments were just questions to be certain we document consistently!

In any case, 👍 for this PR, LGTM

0xc0170 · 2017-11-07T11:08:42Z

/morph uvisor-test

Restarting uvisor as it is green but actually it failed (there was an issue in the CI config).

0xc0170 · 2017-11-08T07:58:21Z

/morph uvisor-test

kjbracey · 2017-11-10T10:05:36Z

Yay! Thanks, Russ.

sg- · 2017-11-10T15:26:51Z

Love that this came in with tests. Any chance there are also docs completed that can be linked to this PR for completeness?

c1728p9 · 2017-11-10T15:56:34Z

Yep, docs can be found here:
ARMmbed/mbed-os-5-docs#318

Give C++ access to the RTOS's absolute timebase, reducing the need to run private Timers and similar. Allows wait_until functionality, and makes it easier to avoid time drift. Place it in a new header and namespace in case we want more kernel functions in future. Try to cover over the breaking API change potentially upcoming in CMSIS-RTOS 2.1.1, when it reduces the tick count from 64-bit to 32-bit. (See ARM-software/CMSIS_5#277) Explicitly state that ticks are milliseconds in mbed OS, despite CMSIS RTOS 2 permitting different tick rates. See also ARMmbed#3648 (wait_until for condition variables) and ARMmbed#5378 (EventQueue should use RTOS tick count).

Give C++ access to the RTOS's absolute timebase, reducing the need to run private Timers and similar. Allows wait_until functionality, and makes it easier to avoid time drift. Place it in a new header and namespace in case we want more kernel functions in future. Try to cover over the breaking API change potentially upcoming in CMSIS-RTOS 2.1.1, when it reduces the tick count from 64-bit to 32-bit. (See ARM-software/CMSIS_5#277) Explicitly state that ticks are milliseconds in mbed OS, despite CMSIS RTOS 2 permitting different tick rates. See also #3648 (wait_until for condition variables) and #5378 (EventQueue should use RTOS tick count).

Give C++ access to the RTOS's absolute timebase, reducing the need to run private Timers and similar. Allows wait_until functionality, and makes it easier to avoid time drift. Place it in a new header and namespace in case we want more kernel functions in future. Try to cover over the breaking API change potentially upcoming in CMSIS-RTOS 2.1.1, when it reduces the tick count from 64-bit to 32-bit. (See ARM-software/CMSIS_5#277) Explicitly state that ticks are milliseconds in mbed OS, despite CMSIS RTOS 2 permitting different tick rates. See also ARMmbed#3648 (wait_until for condition variables) and ARMmbed#5378 (EventQueue should use RTOS tick count).

c1728p9 added the do not merge label Jan 26, 2017

c1728p9 closed this Feb 8, 2017

kjbracey mentioned this pull request Apr 6, 2017

Cellular feature br #4119

Merged

pan- reviewed Apr 7, 2017

View reviewed changes

kjbracey reviewed Apr 7, 2017

View reviewed changes

geky mentioned this pull request Sep 11, 2017

Condition variable #5064

Closed

c1728p9 reopened this Oct 26, 2017

c1728p9 force-pushed the condition_variable branch from f780318 to d69264d Compare October 26, 2017 18:51

c1728p9 changed the title ~~[RFC] Add ConditionVariable to mbed rtos~~ Add ConditionVariable to mbed rtos Oct 26, 2017

c1728p9 removed the do not merge label Oct 26, 2017

c1728p9 force-pushed the condition_variable branch from d69264d to 488d972 Compare October 26, 2017 19:02

kjbracey reviewed Oct 27, 2017

View reviewed changes

c1728p9 force-pushed the condition_variable branch from 488d972 to ed86aed Compare October 28, 2017 17:18

c1728p9 mentioned this pull request Oct 28, 2017

Add documentation for the ConditionVariable class ARMmbed/mbed-os-5-docs#318

Merged

adbridge added needs: review needs: work and removed needs: review labels Oct 30, 2017

c1728p9 force-pushed the condition_variable branch from ed86aed to 4186f91 Compare October 30, 2017 21:33

kjbracey approved these changes Nov 6, 2017

View reviewed changes

c1728p9 removed the needs: preceding PR label Nov 6, 2017

0xc0170 added release-version: 5.7.0-rc1 and removed needs: work labels Nov 7, 2017

0xc0170 reviewed Nov 7, 2017

View reviewed changes

0xc0170 added the ready for merge label Nov 9, 2017

0xc0170 merged commit c45dd26 into ARMmbed:master Nov 9, 2017

0xc0170 removed the ready for merge label Nov 9, 2017

kjbracey mentioned this pull request May 11, 2018

What is the uint32_t _count; variable used for in Mutex Class? #6873

Closed

c1728p9 deleted the condition_variable branch June 19, 2018 14:32

kjbracey mentioned this pull request May 15, 2020

Remove Mutex::_count #12976

Closed

Add ConditionVariable to mbed rtos #3648

Add ConditionVariable to mbed rtos #3648

Uh oh!

Conversation

c1728p9 commented Jan 26, 2017

Uh oh!

c1728p9 commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c1728p9 commented Feb 8, 2017

Uh oh!

kjbracey commented Feb 9, 2017

Uh oh!

hasnainvirk commented Apr 7, 2017

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!

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!

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!

c1728p9 commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1728p9 Oct 27, 2017 • 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!

c1728p9 commented Jan 26, 2017 •

edited

Loading

c1728p9 commented Oct 26, 2017 •

edited

Loading

c1728p9 Oct 27, 2017 •

edited

Loading

0xc0170 commented Nov 6, 2017 •

edited

Loading