break Node into several separate interfaces #277

wjwwood · 2016-11-17T02:18:58Z

This is a work in progress, I've only converted some of the most basic functionality to what I'm calling the "NodeBaseInterface". This is a preview of what I'm doing for the other parts, for which I've sketched out "NodeTopicInterface", "NodeServiceInterface", "NodeGraphInterface", and "NodeParameterInterface".

I've run into a few issues, but specifically I have some work to do to resolve the ownership issues. This is demonstrated by this:

https://github.com/ros2/rclcpp/compare/node_interfaces?expand=1#diff-14f85fa16e78532d255620f1d3529debR52

Where the comment reads "These are used to keep the scope on the containing items" and I had to comment out the Node pointer. Had we gone with inheritance, the NodeBase pointer this class would have access to would have been enough, but given the composition nature, I cannot just hold that part of the interface and maintain the same guarantee. That's ok though, I never liked how we have so much "ownership spaghetti" (for lack of a better phrase) anyways. I just have to resolve how to handle this now. I've got the options and arguments for one way versus another partially written out, I'll try to post those soon. @Karsten1987 recently ran into this in a related situation and thinks (if I can speak for him a bit) that moving away from scoped objects would help a little bit because it could ensure the lifetime of the entities created by a node do not outlive it (using a proxy or handle pattern instead).

gbiggs · 2016-11-29T03:29:12Z

I think I could do with a diagram.

wjwwood · 2016-12-01T01:00:44Z

I justed pushed the publisher and subscription parts of the refactor, rclcpp is building for me locally, but the examples are broken because I'll have to add some convenience functions for things like rclcpp::spin() which take Node * and extract the NodeBaseInterface * from it.

@gbiggs I did some design work upfront and as part of it made this:

I'm not sure it will end up looking exactly like that, but it will be similar.

wjwwood · 2016-12-01T02:08:57Z

Things to point out in the pub/sub refactor:

Node still has code in the create_publisher<>() like methods, but it's mostly variable checking.
As much of the code that depends on the Message type or the Allocator type was put into a non-templated factory class, i.e. PublisherFactory and SubscriptionFactory.
The non-template argument dependent code is in the NodeTopics::create_* functions which are also not templated and take a factory class as an argument.

The Node::create_*<MessageT, ...>() methods basically do this:

Check arguments are valid.
Create a *Factory class by calling the matching create_*_factory<MessageT, ...>() function.
Pass the factory class to the NodeTopics::create_*(..., factory, ...) method, and get a PublisherBase or SubscriptionBase shared pointer back.
Pass the *Base class to the corresponding NodeTopics::add_*() method.

We could consider putting the code that lives in NodeTopics::create_*() back into the templated Node::create_*<MessageT, ...>() methods, but by pushing the code into the NodeTopics class we minimize the code that is in the Node::create_*<MessageT, ...>() methods and by doing so gain:

shorter compile times
reduced amount of code that has to be duplicated if someone want to copy-paste-modify the Node class

Similarly, we could put the code that now lives in the factory classes (mostly code that depends on template variables in one way or another) back into the Node::create_*<MessageT, ...>() methods, but by putting it into a separate factory class it again improves the reusability of this code and would help whenever someone might want to copy-paste-modify the Node class. It especially reduces the amount of non-trivial, or non-boilerplate, code that needs to be copied when creating a new Node class.

It was not possible to have template functions on either the NodeTopicsInterface class or the NodeTopics class without also templating the Node class, which I wanted to avoid. That is why I didn't just move the templated code from the Node class into the NodeTopics class.

wjwwood · 2016-12-01T02:33:38Z

@gbiggs just some follow up points to go with that diagram:

We wanted to accomplish a few things:

Make it easier to mock up parts of the Node interface to more easily test the components that use them without instantiating a full Node implementation to do so.
More clearly separate the various functionality that is aggregated by the Node class for the convenience of the user.
Enable the creation of various other kinds of Node classes, e.g. LifecylceNode or NodeWithoutParameters.

We landed on a few things:

Separate the current state into these discrete parts:
- Base: core functionality and the rcl_node_t handle.
- Topics: publisher and subscription related functionality
- Services: service client and server related functionality
- Parameters: parameter related functionality
- Graph: ROS graph related functionality, i.e. wait_for_service, list_topics, etc.
Use pure virtual "interface" classes for each of the parts, in order to make mocking them easier without worrying about side effects of the constructor.
Use composition, rather than inheritance, to aggregate the functionality of each of parts into the Node class, i.e. Node has pointers to NodeBaseInterface, NodeTopicsInterface, etc. and provides forwarding methods (similar to the PIMPL pattern) rather than inheriting from them.

We discussed several of these points a lot internally and I have notes, but they're not organized at all, but I plan to put them somewhere as memoranda when I get a chance.

dirk-thomas · 2016-12-02T17:30:35Z

rclcpp/include/rclcpp/any_executable.hpp

 #include "rclcpp/macros.hpp"
-#include "rclcpp/node.hpp"
+// #include "rclcpp/node.hpp"


Can this be removed?

dirk-thomas · 2016-12-02T17:30:47Z

rclcpp/include/rclcpp/any_executable.hpp

@@ -44,7 +49,7 @@ struct AnyExecutable
  rclcpp::client::ClientBase::SharedPtr client;
  // These are used to keep the scope on the containing items
  rclcpp::callback_group::CallbackGroup::SharedPtr callback_group;
-  rclcpp::node::Node::SharedPtr node;
+  // rclcpp::node::Node::SharedPtr node;


Can this be removed?

dirk-thomas · 2016-12-02T17:32:59Z

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

+  RCLCPP_PUBLIC
+  virtual
+  rclcpp::context::Context::SharedPtr
+  get_context() const;


If the method is const shouldn't it return a const shared pointer? Otherwise the caller can modify the returned value. Maybe we need both: one const method with const return value and one non-const method with a non-const return value?

I don't know. I didn't make this const originally (this method was just moved not modified or added) because the shared ptr that is returned is a copy. From what I understand it is not required and not appropriate to return a const of a copy, e.g. bool get_x() const and not const bool get_x() const. Instead you return a const when you return a reference, e.g. const MyClass & get_my_class() const rather than MyClass & get_my_class() const.

So if you consider the shared pointer to be a copy, then it doesn't need to be const, i.e. if someone changes the shared ptr that was returned, or calls reset of on it, it would not change the class. However, since it is a pointer, you could modify the thing to which it points and so maybe it should be a ::ConstSharedPtr as we call it (aka std::shared_ptr<const Context>). Getting rid of the shared pointer and just using a raw pointer, it is currently Context * get_context() const and maybe it should be Context const * get_context() const (reads "the const method get_context returns a pointer to a const Context").

I'm not sure there is a needs to be a const and non const version. I think a non const version is fine, and if the method could be const then that would be better, but not necessary.

So the question is whether or not we should leave it as is, or if we should have it return a SharedPtr but not be a const method, or if we should have it return a ConstSharedPtrand be a const method.

From my point of view if you get a const Node then you are not supposed to do anything non-const on the node. But if the class has const methods which expose non-const internals you can call that method and then manipulate the internal state. Therefore const methods should always expose internal state as const.

dirk-thomas · 2016-12-02T17:35:19Z

rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp

@@ -52,7 +52,7 @@ class AllocatorMemoryStrategy : public memory_strategy::MemoryStrategy
  using VoidAllocTraits = typename allocator::AllocRebind<void *, Alloc>;
  using VoidAlloc = typename VoidAllocTraits::allocator_type;

-  using WeakNodeVector = std::vector<std::weak_ptr<node::Node>>;
+  // using WeakNodeVector = std::vector<std::weak_ptr<node::Node>>;


Can this be removed?

dirk-thomas · 2016-12-02T17:35:26Z

rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp

@@ -266,7 +266,7 @@ class AllocatorMemoryStrategy : public memory_strategy::MemoryStrategy
          any_exec->subscription = subscription;
        }
        any_exec->callback_group = group;
-        any_exec->node = get_node_by_group(group, weak_nodes);
+        // any_exec->node = get_node_by_group(group, weak_nodes);


Can this be removed?

dirk-thomas · 2016-12-02T17:35:58Z

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

+  RCLCPP_PUBLIC
+  virtual
+  rcl_node_t *
+  get_rcl_node_handle() const;


Same thing applies here, despite the fact that it isn't a smart pointer.

dirk-thomas · 2016-12-02T17:36:00Z

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

+  RCLCPP_PUBLIC
+  virtual
+  std::shared_ptr<rcl_node_t>
+  get_shared_rcl_node_handle() const;


dirk-thomas · 2016-12-02T17:36:47Z

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

+  RCLCPP_PUBLIC
+  virtual
+  std::unique_lock<std::recursive_mutex>
+  acquire_notify_guard_condition_lock() const;


With the ownership being changed this shouldn't be const?

The returned lock is a temporary and the mutex is mutable, so I'd say no.

I did't' know it is a temporary. That makes sense then.

dirk-thomas · 2016-12-02T17:37:33Z

rclcpp/src/rclcpp/any_executable.cpp

-  callback_group(nullptr),
-  node(nullptr)
+  callback_group(nullptr)
+  // node(nullptr)


Can this be removed?

dirk-thomas · 2016-12-02T17:39:14Z

rclcpp/src/rclcpp/node_interfaces/node_base.cpp

+      // Finalize the interrupt guard condition.
+      finalize_notify_guard_condition();
+
+      throw std::runtime_error("failed to interpret ROS_DOMAIN_ID as integral number");


While this code block has only been moved this line leaks the memory of ros_domain_id on Windows.

wjwwood · 2016-12-02T20:17:46Z

I just pushed the graph API refactor and rebased since @Karsten1987's pr was merged.

I'll address the commented out things and other comments once it is in review.

wjwwood · 2016-12-03T02:49:27Z

I pushed the NodeServices API portion of the refactor and fixed some things added by @Karsten1987's pr which broke due to my changes. Only the NodeParameters part is left to go and to decide where to put the "create_timer" functions, either in a NodeTimers API part, or in the NodeBase part. I'm leaning towards giving timers their own part, mostly because it doesn't seem to fit in the NodeBase API very well (most things that use NodeBaseInterface don't need timers).

Karsten1987 · 2016-12-03T18:53:16Z

+1 for giving timers their own part

gbiggs · 2016-12-04T06:43:58Z

I wonder if creating timers, etc. should be done via factory functions that are friends of the base node interface (or node implementation? I forget which is correct for friends). I think that this should allow any object that provides the necessary friend-accessible interface to allow the creation of timers, and would also keep the size of the node object APIs minimal. One of the popular C++ book authors wrote several articles and book chapters about keeping class APIs minimal by making good use of friend and non-friend functions when possible.

I haven't analysed this idea in any way, but I thought it's worth consideration.

gbiggs · 2016-12-04T06:56:13Z

Thanks for the diagram and explanations, @wjwwood. It's all stuff I've been hoping for, so I'm really happy to see it happening.

wjwwood · 2016-12-06T23:57:40Z

I just pushed Parameters and Timers API's. Now I just need to do some testing and address some existing comments.

wjwwood · 2016-12-07T10:43:59Z

Ok, I've addressed or continued discussion on the existing comments. I've also started some CI which will run tonight and I'll check on tomorrow. One thing I'm thinking of doing still is to remove the publisher and subscription factories I created since they don't make as much sense anymore since I've made the templated free functions create_publisher and create_subscription. It's not strictly necessary, but would be easier to read I think, as the factory breaks the flow of the code up considerably.

I put this in review.

wjwwood · 2016-12-07T22:45:16Z

Ok, I think I've addressed the constness issues, now there is a tlsf test failing which may be something I broke in the allocator support that I need to look into. Still looking for more reviews in the meantime.

wjwwood · 2016-12-09T05:27:20Z

Passing CI:

Linux:
OS X:
Windows:

Karsten1987 · 2016-12-09T01:15:36Z

rclcpp/include/rclcpp/create_publisher.hpp

+
+  auto pub = node_topics->create_publisher(
+    topic_name,
+    rclcpp::create_publisher_factory<MessageT, AllocatorT, PublisherT>(allocator),


why do we need this factory when we pass the allocator as a template argument?

The factory captures all template code into non template functions so they can be interleaved with non template code in the cpp file. As I mentioned before, I think we could remove the factories now in favor of more linear code that lives in the template functions. The trade off being that there is more code in template functions which increase compile time.

I'd rather do that refactor later though so that we can merge this and let other work build on top in the mean time.

Karsten1987 · 2016-12-09T01:16:45Z

rclcpp/include/rclcpp/create_publisher.hpp

+  node_topics->add_publisher(
+    pub,
+    // this is the callback group, not currently used or exposed to the user for publishers
+    nullptr);


could that become a default argument for create_publisher function?

I don't think it needs to because there's no reason to imply that passing it to create publisher has any effect. If anything I'd remove it from the add publisher function.

I removed the argument in c18b6ab, I'll run CI on Linux only (due to the nature of the change that should be enough).

Karsten1987 · 2016-12-09T01:19:18Z

rclcpp/include/rclcpp/create_subscription.hpp

+  const rmw_qos_profile_t & qos_profile,
+  rclcpp::callback_group::CallbackGroup::SharedPtr group,
+  bool ignore_local_publications,
+  bool use_intra_process_comms,


can the two booleans go into a dedicated struct. Similar to the qos_profile_t maybe something like intra_process_profile_t? Also to lower the number of arguments here. Or expose the subscription_options ?

We need an options structure in c++ for these classes, but I didn't want to tackle that in this pr.

I'm creating an issue for this right now.

Karsten1987 · 2016-12-09T01:50:51Z

rclcpp/include/rclcpp/node_interfaces/node_services.hpp

+  RCLCPP_PUBLIC
+  virtual
+  void
+  add_service(


why not separating clients and services?

What do you mean?

I mean why is add_client in NodeServices and not in NodeServiceClients ? Or, if mixed, where is the constructor for ServiceClient ?

Clients and Services (Service servers) are two halves of the same part: "services". It is not separate for the same reason that publisher and subscription related functions are together on the NodeTopics interface: because they are related. I don't see any need to break up the NodeServices interface into NodeServiceClients and NodeServiceServers, but if you do, please make an argument for that case.

where is the constructor for ServiceClient?

I don't understand the question, there is no class called ServiceClient.

Karsten1987 · 2016-12-09T01:52:14Z

rclcpp/include/rclcpp/node_interfaces/node_timers.hpp

+class NodeTimers : public NodeTimersInterface
+{
+public:
+  RCLCPP_SMART_PTR_ALIASES_ONLY(NodeTimers)


Do we need this macro? I do believe that I ran into problems when inheriting from a class with these macros.

We discussed this before, and while I agree that the thing you ran into was a bad user experience, I don't think it should be addressed in this pr and should perhaps only be changed for classes from which people commonly inherit. Please make a new issue to discuss this.

Can you make an issue for this, so it is tracked?

Karsten1987 · 2016-12-09T15:31:56Z

rclcpp/include/rclcpp/node.hpp

-  EventNotRegisteredError()
-  : std::runtime_error("event already registered") {}
-};
-
 /// Node is the single point of entry for creating publishers and subscribers.
 class Node : public std::enable_shared_from_this<Node>


Does the new architecture allow to create Services inside the node constructor?

Yes, but I haven't tested it.

I'll add a test for this real quick.

I made a test in ros2/system_tests#182 and fixed it in f0afcab 😛, good thing you got me to test it.

… func

Karsten1987 · 2016-12-09T21:12:55Z

rclcpp/include/rclcpp/intra_process_manager_impl.hpp

-  using IDTopicMap = std::map<std::string, AllocSet,
-      std::less<std::string>, RebindAlloc<std::pair<const std::string, AllocSet>>>;
+
+  struct strcmp_wrapper : public std::binary_function<const char *, const char *, bool>


can we exclude this struct into an external header file? MIght be helpful for other pieces of code.

I don't feel it needs it's own file, it's a pretty trivial class, I would have made it a lambda if I could have. I'd prefer not to separate it, but you can push here with that change if you like.

Karsten1987 · 2016-12-09T21:13:39Z

rclcpp/include/rclcpp/node.hpp

@@ -117,6 +99,14 @@ class Node : public std::enable_shared_from_this<Node>
  rclcpp::callback_group::CallbackGroup::SharedPtr
  create_callback_group(rclcpp::callback_group::CallbackGroupType group_type);

+  using CallbackGroup = rclcpp::callback_group::CallbackGroup;


can we shift that line before the first usage?

I just removed it and the following line, not sure why I added that there. Might have been a copy-paste issue, thanks for pointing it out. 28a2cfc

Karsten1987 · 2016-12-09T21:15:45Z

rclcpp/include/rclcpp/node_impl.hpp

 }

-template<typename MessageT, typename CallbackT, typename Alloc>
+template<typename MessageT, typename CallbackT, typename Alloc, typename SubscriptionT>
 typename rclcpp::subscription::Subscription<MessageT, Alloc>::SharedPtr
 Node::create_subscription(


out of consistency, I would also bring that forward function into the create_subscription.cpp file.

This is (a) a template function which cannot be put in a cpp file and (b) is a method of the Node class, so it would go in the node.cpp file if it could go in a cpp file.

Karsten1987 · 2016-12-09T21:16:21Z

rclcpp/include/rclcpp/node_impl.hpp

-template<typename CallbackType>
-typename rclcpp::timer::WallTimer<CallbackType>::SharedPtr
+template<typename DurationT, typename CallbackT>
+typename rclcpp::timer::WallTimer<CallbackT>::SharedPtr
 Node::create_wall_timer(


again, just to be consistent, I would also put the create_wall_timer into its own file.

Same as above.

Karsten1987 · 2016-12-09T21:18:38Z

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

+private:
+  RCLCPP_DISABLE_COPY(NodeBase)
+
+  std::string name_;


why do we need this? Does it not make sense to hit directly to rcl for getting the name?

No, I updated it in 83b610e. Thanks!

Karsten1987 · 2016-12-09T21:22:58Z

rclcpp/include/rclcpp/node_interfaces/node_services.hpp

+  RCLCPP_PUBLIC
+  virtual
+  void
+  add_service(


I mean why is add_client in NodeServices and not in NodeServiceClients ? Or, if mixed, where is the constructor for ServiceClient ?

Karsten1987 · 2016-12-09T21:29:45Z

rclcpp/src/rclcpp/node_interfaces/node_timers.cpp

+
+#include <string>
+
+#include "rclcpp/node_interfaces/node_timers.hpp"


shouldn't that include be on first?

I tried that, but I think cpplint was not happy about it for some reason. I'll try again.

I fixed it here and in other places in 23a7728.

Karsten1987 · 2016-12-09T21:31:12Z

rclcpp/src/rclcpp/node_interfaces/node_topics.cpp

+}
+
+rclcpp::subscription::SubscriptionBase::SharedPtr
+NodeTopics::create_subscription(


why is this not in create_subscription.cpp ?

Karsten1987 · 2016-12-09T21:31:36Z

rclcpp/src/rclcpp/node_interfaces/node_topics.cpp

+{}
+
+rclcpp::publisher::PublisherBase::SharedPtr
+NodeTopics::create_publisher(


why is this not in create_publisher.cpp ?

Because it is a member of NodeTopics?

Karsten1987 · 2016-12-09T21:33:51Z

This PR looks good to me. I had a few comments/questions about file ordering and consistency. But no show-stoppers.

wjwwood · 2016-12-09T23:10:51Z

Ok, @Karsten1987 I think I addressed your comments, can you check to make sure I did? Also, anyone else that wants to review, please do so now.

I'm starting new CI since I made some significant changes.

wjwwood · 2016-12-10T01:09:28Z

The warnings are being addressed by @codebot and I cannot reproduce the OS X test failures on my machine, and I am debugging the Windows test timeouts and I will fix that up after merging this if this pr turns out to have caused it.

* initialize memory before sending a message * add todo referencing ticket * wrap todo

wjwwood added the in progress Actively being worked on (Kanban column) label Nov 17, 2016

wjwwood mentioned this pull request Nov 17, 2016

triggering a guard condition is not const ros2/rcl#89

Merged

codebot assigned wjwwood Nov 22, 2016

wjwwood force-pushed the node_interfaces branch from 42bc080 to 58a2cca Compare December 1, 2016 00:56

This was referenced Dec 1, 2016

made a fix for how rclcpp::spin was being used with std::thread ros/geometry2#193

Merged

(dev) construct service from existing rcl_service_t #279

Merged

dirk-thomas reviewed Dec 2, 2016

View reviewed changes

wjwwood force-pushed the node_interfaces branch from 4a101f1 to 4dcab50 Compare December 2, 2016 20:16

wjwwood force-pushed the node_interfaces branch from ad610c0 to 0dd7020 Compare December 6, 2016 22:31

wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 7, 2016

wjwwood added 7 commits December 8, 2016 14:41

add the NodeBaseInterface and impl NodeBase

5133756

refactor rclcpp to use NodeBaseInterface

e1e9709

triggering a guard condition is not const

a9947d5

remove unnecessary pure virtual destructor

4fd3ef2

remove unused private member, style

f205de7

create NodeTopics interface, refactor pub/sub

242f459

add convenience functions to fix API breaks

68ccfa6

wjwwood added 4 commits December 8, 2016 14:42

address constness issues

9fd2ad9

do not store the topic name as a std::string in subscription

9ad8342

fixes to support const char * topic name

6252e72

fix incomplete type specification, which fails on Windows

7150837

wjwwood force-pushed the node_interfaces branch from f483cea to 7150837 Compare December 8, 2016 22:43

wjwwood and others added 4 commits December 8, 2016 16:07

refactor after rebase from type support changes

1122e25

fixup Windows build

365705a

fix template issues on Windows

dfa0cf7

uncrustify

053cd5b

Karsten1987 reviewed Dec 9, 2016

View reviewed changes

wjwwood mentioned this pull request Dec 9, 2016

Deduce MessageT from the first argument of the callback #286

Closed

remove the unnecessary callback group argument from the add_publisher…

c18b6ab

… func

Karsten1987 reviewed Dec 9, 2016

View reviewed changes

Karsten1987 approved these changes Dec 9, 2016

View reviewed changes

wjwwood added 2 commits December 9, 2016 14:46

remove unnecessary using = directive

28a2cfc

do not store node name in C++

83b610e

wjwwood mentioned this pull request Dec 9, 2016

add test for creating clients and services in a Node constructor ros2/system_tests#182

Merged

wjwwood added 2 commits December 9, 2016 15:04

fix client and service creation in Node constructor

f0afcab

fix include orders

23a7728

wjwwood merged commit 734ac27 into master Dec 10, 2016

wjwwood deleted the node_interfaces branch December 10, 2016 01:09

wjwwood removed the in review Waiting for review (Kanban column) label Dec 10, 2016

wjwwood mentioned this pull request Dec 10, 2016

fix the return type of create_subscription #290

Merged

dhood mentioned this pull request Nov 27, 2017

Can't create ParameterService from within node constructor #409

Closed

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

initialize memory before sending a message (ros2#277)

1e5bf4a

* initialize memory before sending a message * add todo referencing ticket * wrap todo


		#include <string>

		#include "rclcpp/node_interfaces/node_timers.hpp"

break Node into several separate interfaces #277

break Node into several separate interfaces #277

Conversation

wjwwood commented Nov 17, 2016 • edited by codebot Loading

gbiggs commented Nov 29, 2016

wjwwood commented Dec 1, 2016

wjwwood commented Dec 1, 2016

wjwwood commented Dec 1, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood commented Dec 2, 2016

wjwwood commented Dec 3, 2016

Karsten1987 commented Dec 3, 2016

gbiggs commented Dec 4, 2016

gbiggs commented Dec 4, 2016

wjwwood commented Dec 6, 2016

wjwwood commented Dec 7, 2016

wjwwood commented Dec 7, 2016

wjwwood commented Dec 9, 2016

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karsten1987 commented Dec 9, 2016

wjwwood commented Dec 9, 2016

wjwwood commented Dec 10, 2016

wjwwood commented Nov 17, 2016 •

edited by codebot

Loading