Add an python example to change speeds #502

clalancette · 2018-07-12T17:25:01Z

This is the first part of #460 which does a few things:

It adds py::keep_alive to the constructors for the agents
It adds in a simple velocity controller, which takes as input a commanded velocity and a feedback velocity, and outputs an acceleration
It wires up the velocity controller into the rail car
It wires up a ConstantVector into the velocity controller
It exposes the APIs necessary to modify the ConstantVector from python, so that python code can change velocities.
It adds a new demo which shows changing the velocity after a certain amount of time

There are a lot of small commits here; it may be easier to review commit-by-commit, rather than all at once, but I'll leave that to the reviewers. A couple more notes:

I'd like a thorough review of the py::keep_alive stuff; I think this is what people were talking about, but I want to make sure
I know that VelocityController is really simple and isn't great. It's not clear to me that it is worth making it a lot smarter than it currently is, so opinions welcome.

Finally, thanks to @hidmic for suggestions on how to get this all working!

hidmic

@clalancette I left a couple comments! Nice work.

hidmic · 2018-07-13T13:27:20Z

python/delphyne/agents.cc


  py::class_<delphyne::TrajectoryAgent, delphyne::Agent>(m, "TrajectoryAgent")
      .def(py::init<const std::string&, const std::vector<double>&,
                    const std::vector<double>&,
                    const std::vector<std::vector<double>>&>(),
           "Construct and configure a trajectory agent", py::arg("name"),
-           py::arg("times"), py::arg("headings"), py::arg("translations"));
+           py::arg("times"), py::arg("headings"), py::arg("translations"),
+           py::keep_alive<1, 2>());


@clalancette I'm not entirely sure these py::keep_alive guards are needed here. The constructed object will be managed by the Python interpreter, and I don't see an argument we should tie agent's lifetime to. Do you?

This is why I wanted review on this particular part :). I think you are right about this, and removing them doesn't seem to make a difference, so I'll do that. Thanks.

hidmic · 2018-07-13T13:31:48Z

python/delphyne/simulation.cc

+           py::return_value_policy::reference_internal)
+      .def("get_mutable_context",
+           &AutomotiveSimulator<double>::GetMutableContext,
+           py::return_value_policy::reference_internal);


@clalancette FYI here you'd need py::keep_alive guards, to ensure that your AutomotiveSimulator instance isn't thrown away until you drop the returned reference. But since you're using reference_internal as return value policy, you get that for free.

Right, makes sense.

hidmic · 2018-07-13T14:06:15Z

python/delphyne/utilities.py

        road_geometry=road_geometry)     # maliput road geometry
    simulator.add_agent(agent)
+    return agent


@clalancette this is the part that is somewhat shady (and the reason why these utility functions exist). When you add an agent, simulator takes ownership (as it takes an std::unique_ptr) and you're left with a reference. I haven't been able to find the exact part of the code that does it in the pybind11 fork we're using, but that's what you can verify by running this code. The problem is that AFAIK (and if Eric hasn't changed anything since) you're left with a dangling reference. If simulator were to be garbage collected, accessing your agent would result in a segfault.

One way we could get away with it safely here would be to have AutomotiveSimulator::AddAgent return a raw pointer to the now owned agent and set its return value policy to reference_internal. But, we lose IDs, which I'm using as a handle to query agents on collision. This is the part we need to agree on and to standardize a mechanism to move agents around.

Aye, I was envisaging AutomotiveSimulator::AddAgent returning a raw pointer as well.

Aye, I was envisaging AutomotiveSimulator::AddAgent returning a raw pointer as well.

I did first try having AddAgent return a pointer, but there were several problems. The first one is that IDs are indeed used in collisions for lookup. The second one is that IDs are (currently) used in the tests for verification, though this PR removes those references. The big problem, though, was that returning a pointer here makes the code decidedly un-Pythonic. That is, your code would look something like this:

agent = agents.SimpleCar(...) agent_ptr = simulator.add_agent(agent)

As a Python programmer that is surprising, since I always expect things to take references, not "give away" ownership. While stuffing things in utilities helps here, it is just not what the programmer is expecting (and the fact that I ended up making the mistake seems to say that others will too). If we really want to go this direction, I can change the code, but I think it will lead to pain further down the road.

If simulator were to be garbage collected, accessing your agent would result in a segfault.

This is the bit I'm still struggling with. Isn't the whole point of py::keep_alive to ensure that that does not happen?

So, I exercise this idea in #508, changing the way IDs are being tracked too. It makes for a neat C++ API, but it does get awkward when you move to Python. The thing, I believe, is that this issue is bigger than both PRs. The AutomotiveSimulator::AddAgent() method, same as many others pydrake bindings do, forces object ownership into a language that has no support nor a defined semantic for it.

Using py::keep_alive attributes alleviates the problem somewhat, but it's not a complete solution. AFAIK when you pass ownership like this, no automatic lifetime dependency is established. That's the point of doing something like:

agent = agents.SimpleCar(...) agent = simulator.add_agent(agent)

Or even agent = simulator.add_agent(agents.SimpleCar(...)). A py::keep_alive can then be used for the return value.

As to how we can solve the problem altogether, I don't have an optimal solution. We could see if it's possible to inject a keep alive whenever ownership is transferred (assuming that if B takes A, then B also keeps A). At some point, I also thought about wrapping bindings with pure Python code to obscure as many quirks as possible (e.g. if we could reduce ownership transfer occurrences to match this instantiate-then-pass pattern, maybe we could use the fact that classes in Python are objects themselves to pass them instead and preclude the issue, like simulator.add_agent(agents.SimpleCar, *args, **kwargs) or something over those lines).

In any case, whatever we do here, it should go into Drake as well (or start at Drake?). If not, quirks will come back as soon as you import one of their modules.

Worst case, I was thinking we might have add_<xyz>_agent(simulator) python api which encapsulate the quirks so scenario authors never have to be surprised (merely the agent developer).

def add_simple_agent(simulator): agent = agents.SimpleCar(...) agent = simulator.add_agent(agent) return agent

and elsewhere

agent_john_smith = add_simple_agent(simulator)

That could be better still if we completely hide simulator.add_agent and create add_xyz_agent methods in C++ and expose them. I think @hidmic was implying this?

Alternatively. perhaps we should give up the drama of strict ownership for a language that has no support for ownership and simply use shared pointers instead? Eric has often mused that python bindings were considered too late to influence the C++ design of Drake systems.

That's the C++ side alternative, yes (i.e. having custom add_..._agent methods). It could be done on the Python side too, by passing around classes.

Personally, I'd vote for keeping the unique_ptr-based interface, making heavy use of py::keep_alive guards to cope with them in Python. In #508 I test one solution (probably not the very best) for safely returning owned pointers in arbitrary data structures, so no need to get agents by ID or name anymore. And to deal with this add_agent problem, I'd say we go ahead with something like agent = simulator.add_agent(agent). We can later add a thin Python layer on top that wraps these un-pythonic statements in something like simulation.add_agent("bob", type=SimpleCar) (where SimpleCar is a class object), and even fill this layer up with comments and warnings for the unaware developer.

What do you guys think? @clalancette @stonier

Yeah, I think I'll stick with the unique_ptr interface for now. If we want to revisit that decision in the future, it should be easy enough to do so. I'll start working on that now so we can unblock this PR.

hidmic · 2018-07-13T14:07:44Z

src/agents/rail_car.cc

+          initial_parameters_.speed);
+  velocity_input_ = velocity_input.get();
+
+  auto velocity_input_system = builder.AddSystem(std::move(velocity_input));


@clalancette why not just do ... = builder.AddSystem<drake::systems::ConstantVectorSource<double>>(...);?

Ah, I can't do that here, because I need to save a reference to the velocity_input_ pointer for later on in SetVelocity.

Hmm, I don't follow why velocity_input_ = builder.AddSystem<drake::systems::ConstantVectorSource<double>>(...); wouldn't do.

Oh, you are totally right, I had forgotten that AddSystem returns the raw pointer. I'll change this, thanks.

hidmic · 2018-07-13T14:11:29Z

src/agents/rail_car.cc

+
+  Eigen::VectorXd newval(1);
+  newval[0] = new_vel_mps;
+  sourcevel.set_value(newval);


@clalancette why not sourcevel[0] = new_vel_mps;?

I didn't know you could do that :). Thanks, fixed now.

hidmic · 2018-07-13T14:30:08Z

src/systems/velocity_controller.cc

+  } else {
+    constant_value[0] = 0.0;
+  }
+  output->set_value(constant_value);


@clalancette same as above, you can use SetAtIndex() on a BasicVector.

Great, thanks.

hidmic · 2018-07-13T14:32:02Z

src/systems/velocity_controller.cc

+
+  // Allocates and uses a BasicVector containing a zero acceleration command in
+  // case the input contains nullptr.
+  const auto default_input = drake::systems::BasicVector<T>::Make(magnitude);


@clalancette I'd rather not allocate memory if not strictly necessary, consider using automatic storage variables only (i.e. local, stack ones).

This is basically a copy of https://github.com/ToyotaResearchInstitute/delphyne/blob/master/src/systems/rail_follower.cc#L312 . That being said, I agree with you that it would be nicer to make it a stack variable, so I'll change it. Thanks.

hidmic · 2018-07-13T14:35:32Z

test/regression/cpp/automotive_simulator_test.cc

-  // Verifies that passing an unknown agent ID is an error.
-  const int agent_x_id = -1;
-  EXPECT_THROW(simulator->GetAgentById(agent_x_id), std::runtime_error);
-}


@clalancette why the test removal?

That's because I removed GetAgentById completely in the first patch :).

hidmic · 2018-07-13T14:37:07Z

src/agents/rail_car.h

@@ -65,6 +66,10 @@ class RailCar : public delphyne::Agent {

  std::unique_ptr<DiagramBundle> BuildDiagram() const;

+  void SetVelocity(drake::systems::Context<double>* sim_context,


@clalancette missing documentation.

Will add, thanks.

hidmic · 2018-07-13T14:41:46Z

src/agents/rail_car.cc

@@ -137,4 +158,18 @@ std::unique_ptr<Agent::DiagramBundle> RailCar::BuildDiagram() const {
  return std::move(builder.Build());
 }

+void RailCar::SetVelocity(drake::systems::Context<double>* sim_context,
+                          const drake::systems::Diagram<double>* diagram,
+                          double new_vel_mps) {


@clalancette so, I understand why we're passing all this arguments, but I wonder if this is how the API should look like from now on, as opposed to:

Have the Agent get a hold to its own diagram context during build time.

Have the Agent delegate the subcontext query to the simulator.

...

I'm generally fine with passing contexts around, but doing the same with the diagram is kinda weird. Note that this is purely subjective and you're free to ignore it and/or iterate on this on a future PR.

I am wondering whether it's better to heave around context like this, or just asynchronously drop in the value in the same way as the LcmSubscriberSystem - lcm_subscriber_system.h, lcm_subscriber_system.cc. The key difference (I think) being you have a system that acts as a source which feeds the rest of the diagram rather than directly modifying an input port.

If we're serious about keeping the simulator out of the agent, then this is the way I anticipated that you might do it and you'd end up with an exceedingly simple SetVelocity(double new_vel_mps) api. Mind you, we don't need half of the complexity of that system, but it might be worth considering the creation of a default (templated) class that can act as a 'setter' from the agents in this way.

I'm generally fine with passing contexts around, but doing the same with the diagram is kinda weird. Note that this is purely subjective and you're free to ignore it and/or iterate on this on a future PR.

I actually totally agree; moving the diagram around like this felt weird to me too, so I'm glad you brought it up. I think your proposal 1) is off the table if we want to keep agents completely divorced from the simulator. I like proposal 2), I think it cleans things up a bit, so I'll explore doing that quickly.

The key difference (I think) being you have a system that acts as a source which feeds the rest of the diagram rather than directly modifying an input port.

OK, I'll take a look at doing this. We are also doing something similar with the IgnSubscriberSystem.

I like proposal 2), I think it cleans things up a bit, so I'll explore doing that quickly.

Of course, the problem with doing that is that it requires us to #include <delphyne/automotive_simulator.h> in the agents, which I think we were trying to avoid. I'm going to give up on that for now and see what I can do with a system.

OK, in fa0c3fd I ended up implementing a speed system, rather than the speed controller with the ConstantVector input. And @stonier was correct, I think it is much nicer from the python perspective. There is still one problem with it in that there is a sort of "jump" when the acceleration changes; I still need to look into that. But a quick glance at the approach would be appreciated.

stonier · 2018-07-17T00:52:40Z

python/delphyne/demos/change_speed_time.py

@@ -0,0 +1,125 @@
+#!/usr/bin/env python2.7


Can this example be folded into delphyne-scriptlets (as indicated in #460) so we don't need an additional example to maintain?

I ended up making it a separate demo because it doesn't really fit into the framework of https://github.com/ToyotaResearchInstitute/delphyne/blob/master/python/delphyne/demos/scriptlets.py very nicely. In particular, that scriptlets file is very simple so doesn't have a road network or any other railcars. I wasn't sure if that was meant to be the simple, introductory example. I'm fine going either way, just let me know if you want to make scriptlets more complex.

I was thinking of beefing it up to include road networks and such. No compelling reason to 'keep it simple'.

stonier · 2018-07-17T02:01:26Z

src/systems/velocity_controller.cc

+ *****************************************************************************/
+
+template <typename T>
+VelocityController<T>::VelocityController() {


Should probably be a speed, not velocity controller?

I mostly named it "Velocity" since it is hooking up to rail_follower_system->velocity_output(), but SpeedController is more accurate. I'll rename it.

stonier

Left some comments inline.

clalancette · 2018-07-19T15:44:45Z

I've rebased, fixed conflicts, and addressed the "easy" comments. I think that leaves us with two large open items here:

I've left comments in both of them, so hopefully we can come to some sort of consensus on these.

clalancette · 2018-07-26T19:25:18Z

All right, I've switched to returning an AgentBase * from AddAgent. @hidmic , make sure to look at the changes in 6f2b7f8 in particular, as they change how the collisions are being reported. Let me know what you think.

clalancette · 2018-07-26T19:25:58Z

Oh, and I should mention that I think all of the major pieces are in place here, so this is probably ready for another review.

hidmic

@clalancette Second pass ready. Sorry for the long roundtrip.

hidmic · 2018-07-31T13:50:48Z

src/agents/rail_car.h

+  ///
+  /// @param sim_context[in] The simulator Context to use for this change
+  /// @param diagram[in] The diagram from which to look the sub-Context up
+  /// @param new_speed_mps[in] The new speed for the agent in meters/second


@clalancette outdated documentation maybe?

Yep, absolutely. Fixed.

hidmic · 2018-07-31T13:52:23Z

src/backend/automotive_simulator.cc

  // save a handle to it
  agents_[id] = std::move(agent);
-  return id;
+
+  return retptr;


@clalancette would something like the following be more clear?

agents_[id] = std::move(agent); return agents_[id].get();

Sure, that's fine too. Fixed.

hidmic · 2018-07-31T13:54:32Z

src/systems/speed_system.h

+
+namespace delphyne {
+
+class SpeedSystem final : public drake::systems::LeafSystem<double> {


@clalancette missing class documentation.

hidmic · 2018-07-31T13:55:02Z

src/systems/speed_system.h

+  ///
+  /// @param[in] topic_name The name of the ignition topic this system will
+  /// be subscribed to.
+  SpeedSystem() {


@clalancette outdated documentation?

Yep, fixed.

hidmic · 2018-07-31T13:56:03Z

src/systems/speed_system.h

+            .get_index();
+
+    this->DeclareAbstractState(
+        drake::systems::AbstractValue::Make<double>(double{}));


@clalancette why an abstract state instead of a continuous one?

Moreover, why is speed a state variable?

@clalancette why an abstract state instead of a continuous one?

I'm honestly not sure of the difference. Do you think is this is better modeled as continuous, and if so, why is that?

Moreover, why is speed a state variable?

I'm not entirely sure I understand the question. To give a little better context, I used https://github.com/ToyotaResearchInstitute/delphyne/blob/master/src/backend/ign_subscriber_system.h as an example, and modified that to have a speed instead of subscribing to a topic. In that context, it makes sense to me that speed is a state variable, but I have to admit that I still do not really understand Drake's systems. Do you have a suggestion on what else it might be?

I'm honestly not sure of the difference. Do you think is this is better modeled as continuous, and if so, why is that?

Much of the type casting machinery is already handled by Drake if you use continuous state (which it is). Truth to be told, everything is abstract at the bottom of it.

Do you have a suggestion on what else it might be?

It does make more sense to me (that I think of Drake as a powerful Simulink) to have the speed setpoint being an input to the controller instead of its state. I do understand though why you did so, trying to synchronize speed changes, but then consider my comment below.

hidmic · 2018-07-31T14:03:05Z

src/systems/speed_system.h

+    double input_speed = GetSpeed(context);
+
+    if (input_speed > magnitude) {
+      output->SetAtIndex(0, 10.0);


@clalancette consider using something like kAccelerationSetpoint instead of plain 10.

Good point, fixed.

hidmic · 2018-07-31T14:07:38Z

test/regression/cpp/automotive_simulator_test.cc

@@ -353,25 +333,14 @@ TEST_F(AutomotiveSimulatorTest, TestMobilControlledSimpleCar) {

  // Expect the SimpleCar to start steering to the left; y value increases.
  const double mobil_y =
-      draw_message.models(id_mobil).link(0).pose().position().y();
+      draw_message.models(0).link(0).pose().position().y();


@clalancette Here and below, we're making assumptions about agent IDs. If we cannot remove them altogether (e.g. because LCM and/or ignition model messages carry these IDs), why not have the agent keeping it? See #508 for an example of this.

I thought about that, but it really seems like IDs are an AutomotiveSimulator level concern, and the agents shouldn't know that about themselves (this goes along with "keep the agents completely divorced from the simulator"). Your solution in #508 is a bit nicer, but it still seems to store data where it doesn't belong. I'm not sure what to do here, honestly.

Hmm, if IDs, just like names, ought to be unique identifiers, so why are we delegating ID generation to a particular simulator instance? In any case, why do we even need IDs if we have unique names already?

Honestly, the only place we use these IDs outside of the AutomotiveSimulator class is in the tests (and there only to look at the offsets into the protobuf message). I think I mostly agree with you that we may not need IDs at all, but I think I'd like to punt on that to another issue (so we can keep this more focused). Since we are only hard coding numbers for the tests, this seems like an acceptable path to me. Do you agree?

Yeap, it's OK (at least for the time being).

hidmic · 2018-07-31T14:11:16Z

src/systems/speed_system.h

+    accel_output_port_index_ =
+        this->DeclareVectorOutputPort(drake::systems::BasicVector<double>(1),
+                                      &SpeedSystem::CalcOutputAcceleration)
+            .get_index();


@clalancette Hmm, why isn't there an speed setpoint input port (e.g. connected to a constant vector source system)?

I'll reply to this one in the comment below.

hidmic · 2018-07-31T14:48:32Z

src/systems/speed_system.h

+
+      uu_events.add_event(
+          std::make_unique<drake::systems::UnrestrictedUpdateEvent<double>>(
+              drake::systems::Event<double>::TriggerType::kTimed));


@clalancette Let me see if I understand this correctly. We're buffering speed setting inputs, in both system state and a locked access internal member-field . Values in the latter are pushed to the former synchronously through events, so that asynchronous accesses from another thread (e.g. Python interpreter's thread) do not corrupt the simulation.

If that's the case, then following the same train of thought we'll have to repeat this pattern across all systems that expose a similar API and have Python bindings (and what do we do about Drake systems that fulfill these conditions already but do not provide such synchronization mechanisms?).

Two ideas pop up:

Would it make sense to isolate this mechanism in single system type, a ConstantVectorSource<T> variant maybe, that can then be connected through regular ports?

Should we even allow this? I think about Python callbacks as tools for simulation scripting (as in, high level manipulation), but not as the cornerstone of a simulation (that's what systems are for :)). In that context, can't we just ban modifications on a running simulation? All accesses ultimately have to impact the simulation context, maybe we can somehow put a guard at that level.

@clalancette Let me see if I understand this correctly. We're buffering speed setting inputs, in both system state and a locked access internal member-field . Values in the latter are pushed to the former synchronously through events, so that asynchronous accesses from another thread (e.g. Python interpreter's thread) do not corrupt the simulation.

Yep, that's exactly how I interpret how this works (again, I took this design from ign_subscriber_system.h).

If that's the case, then following the same train of thought we'll have to repeat this pattern across all systems that expose a similar API and have Python bindings

Right, that's my expectation as well.

(and what do we do about Drake systems that fulfill these conditions already but do not provide such synchronization mechanisms?).

I'm not sure, but I think for most of these we would need to make changes anyway to expose the APIs we want to see.

Two ideas pop up:

Would it make sense to isolate this mechanism in single system type, a ConstantVectorSource<T> variant maybe, that can then be connected through regular ports?

Yeah, I guess we could do this. We would have a templated ConstantVectorSettable, something like:

template <typename T> class ConstantVectorSettable final: public SingleOutputVectorSource<T> { public: ConstantVectorSettable() { ... this->DeclareAbstractState(drake::systems::AbstractValue::Make<T>(T{})); } T get(Context<T>& context) { return context.get_abstract_state().get_value<kStateIndexVar).GetValue<T>(); } void set(T new_val) { std::lock_guard<std::mutex> lock(mutex_); val_ = new_val; } private: <do update synchronization here> mutable std::mutex mutex_; T val_; };

Does that match what you are thinking of?

Should we even allow this? I think about Python callbacks as tools for simulation scripting (as in, high level manipulation), but not as the cornerstone of a simulation (that's what systems are for :)). In that context, can't we just ban modifications on a running simulation? All accesses ultimately have to impact the simulation context, maybe we can somehow put a guard at that level.

I guess I don't understand this bit, as my understanding of this task is that it explicitly wants to do modification on a running simulation. There is another design we could think about, where we load up the changes before we start running the simulation and just have them fire at the correct time, but I consider that more-or-less an optimization of this.

Does that match what you are thinking of?

Yes! That would free us from having to repeat this pattern and instead have as many of these blocks connected through regular ports as we need.

I guess I don't understand this bit, as my understanding of this task is that it explicitly wants to do modification on a running simulation.

Yes, it was more of an afterthought, probably not expressed in the best possible way. My point is, one thing is reacting to events and another thing is adding functionality to the simulation. If we only need the former (and IMHO, we'll likely get much better performance if we refrain from having asynchronous python code doing what a wired system should be doing), then why not ensure that the simulation is paused while the the event handler manipulates it? That's what's happening already, as callbacks are served outside the simulation step, BUT nothing prevents someone from doing funky things outside of a callback. That's why I talk about guards and stuff.

Anyways, let's move forward with the first idea. It's a much more well-rounded approach

Yes! That would free us from having to repeat this pattern and instead have as many of these blocks connected through regular ports as we need.

I just realized that this will require us to revisit the discussion about passing around Context and Diagram (ala #502 (comment)). Looking at it again, what I currently have is explicitly what that comment is asking for. I now realize that I didn't completely do what @stonier was asking, in that I didn't make it templated and easy to reuse. If I make the current code templated (and rename it), that is sort of halfway between what you were asking and what Daniel was asking. @hidmic what do you think?

@clalancette IMHO having this synchronizing ConstantVectorSource variant gets really close to what @stonier was suggesting (do correct me if I'm wrong or missing something). There's no need to explicitly pass sub-contexts anymore if we do this, which is great!

One thing that crossed my mind the other day. If we issue changes to the simulation through these synchronized sources, we're implicitly giving away determinism and repeatability. Not a show stopper, but something to clearly state somewhere in the documentation. It is a thing for some.

hidmic · 2018-07-31T15:25:32Z

src/backend/automotive_simulator.cc

-const std::vector<std::pair<int, int>> AutomotiveSimulator<T>::GetCollisions()
-    const {
+const std::vector<std::pair<delphyne::AgentBase<T>*, delphyne::AgentBase<T>*>>
+AutomotiveSimulator<T>::GetCollisions() {


@clalancette I had to test the delphyne-crash demo several times because I couldn't understand how it was not breaking. I would have thought that a returning a vector by-value and sticking a reference_internal return value policy to the binding would've left us with a dangling reference to a value in the stack (as intermediary containers are not internal to the simulator, only the leaf pointers are) but, alas, it doesn't. It seems I failed to see that pybind11 code that deals with STL containers always returns a Python primitive (list, tuple, etc.) regardless of the specified return value policy.

So kudos, no need for the solution in #508 for this specific issue.

@clalancette I had to test the delphyne-crash demo several times because I couldn't understand how it was not breaking. I would have thought that a returning a vector by-value and sticking a reference_internal return value policy to the binding would've left us with a dangling reference to a value in the stack (as intermediary containers are not internal to the simulator, only the leaf pointers are) but, alas, it doesn't. It seems I failed to see that pybind11 code that deals with STL containers always returns a Python primitive (list, tuple, etc.) regardless of the specified return value policy.

But wouldn't the existing code have the same problem? It is also returning a vector by value, just templated on different types. Or am I misunderstanding your point?

The implementation on master was not using a reference_internal return value policy (check here). Yours is and to my surprise it works just fine (unless I've misunderstood a part of pybind11).

clalancette · 2018-08-08T17:37:44Z

OK, the speed system has been replaced by a ConstantVectorSettable system, which should be mostly re-usable (I guess we won't know for sure until we have a second user). I think this is ready for another round of review.

clalancette · 2018-08-10T17:36:44Z

My latest commits (c3526f8 and 6f875de) implements the two system notion that @hidmic and I discussed a couple of days ago. This seems to work fine in my testing, and is a little more generic. I will point out that the ConstantVectorSettable class isn't really generic; the template type to it is for the "double" type, not the output type. My personal feeling is that we should go with this limitation for now, and we can refactor it later, but I'll be interested to here more opinions here.

hidmic

@clalancette just a few details pending but otherwise ok, so I'll approve to get this going. Great work!

hidmic · 2018-08-13T12:02:07Z

python/delphyne/demos/change_speed_time.py

+#
+"""
+The demo to change railcar speed after a certain amount of time.
+```


@clalancette extra ``` bits?

hidmic · 2018-08-13T12:03:18Z

src/systems/constant_vector_settable.h

+/// the user asks for a new one.  This differs from drake's ConstantVectorSource
+/// in that it is thread-safe with respect to changes to the ConstantVector.
+///
+/// @tparam T must be a valid Eigin ScalarType


@clalancette Eigin -> Eigen

hidmic · 2018-08-13T12:04:51Z

src/systems/constant_vector_settable.h

+                                      &ConstantVectorSettable::CalcOutputValue)
+            .get_index();
+    this->DeclareAbstractState(
+        drake::systems::AbstractValue::Make<T>(T{defaultval}));


@clalancette FYI as pointed out above, somewhere, this could be continuous state instead, but this will work too.

hidmic · 2018-08-13T12:05:33Z

src/systems/speed_system.h

+/// The SpeedSystem implements a very simple speed controller, taking as an
+/// input the current frame velocity (from an InputPort) and the desired speed
+/// (set as an abstract state value of this class), and producing an
+/// acceleration on an OutputPort to reach that speed.


@clalancette outdated documentation?

hidmic · 2018-08-13T12:05:59Z

test/regression/cpp/automotive_simulator_test.cc

@@ -353,25 +333,14 @@ TEST_F(AutomotiveSimulatorTest, TestMobilControlledSimpleCar) {

  // Expect the SimpleCar to start steering to the left; y value increases.
  const double mobil_y =
-      draw_message.models(id_mobil).link(0).pose().position().y();
+      draw_message.models(0).link(0).pose().position().y();


Yeap, it's OK (at least for the time being).

stonier · 2018-08-14T18:32:49Z

Added a comment to combine it into the scriptlet case - most people will want to know how to create a scriptlet for their scenario - which will almost always include roads and such.
ConstantVectorSettable is a contradictory name! Does it end up equalling 'nothing'? Maybe VectorSource? That is also odd that it only deals with dimension one.
Should be easy to extend that system to handle N dimensions - supply 'names' at construction time, generate an index-name map internally and then set with name and value

Discussed the above face to face. Chris will leave follow-up details here.

Otherwise, LGTM.

clalancette · 2018-08-14T20:04:36Z

Added a comment to combine it into the scriptlet case - most people will want to know how to create a scriptlet for their scenario - which will almost always include roads and such.

Got rid of delphyne-change-speeds, merged this functionality into delphyne-scriptlets

ConstantVectorSettable is a contradictory name! Does it end up equalling 'nothing'? Maybe VectorSource? That is also odd that it only deals with dimension one.

Renamed it to VectorSource now.

Should be easy to extend that system to handle N dimensions - supply 'names' at construction time, generate an index-name map internally and then set with name and value

I opened up #523 to track this improvement.

I also addressed @hidmic's small outstanding comments.

Unless I hear otherwise, I'm going to merge this tomorrow morning. Thanks for the reviews and the patience here.

This isn't really used anywhere, and is really an internal implementation detail. Just remove its use and tests. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>