{"action":"created","issue":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2967","repository_url":"https://api.github.com/repos/RobotLocomotion/drake","labels_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2967/labels{/name}","comments_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2967/comments","events_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2967/events","html_url":"https://github.com/RobotLocomotion/drake/pull/2967","id":168144130,"number":2967,"title":"Add input port to the spring-mass system","user":{"login":"amcastro-tri","id":17601461,"avatar_url":"https://avatars.githubusercontent.com/u/17601461?v=3","gravatar_id":"","url":"https://api.github.com/users/amcastro-tri","html_url":"https://github.com/amcastro-tri","followers_url":"https://api.github.com/users/amcastro-tri/followers","following_url":"https://api.github.com/users/amcastro-tri/following{/other_user}","gists_url":"https://api.github.com/users/amcastro-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/amcastro-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amcastro-tri/subscriptions","organizations_url":"https://api.github.com/users/amcastro-tri/orgs","repos_url":"https://api.github.com/users/amcastro-tri/repos","events_url":"https://api.github.com/users/amcastro-tri/events{/privacy}","received_events_url":"https://api.github.com/users/amcastro-tri/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/RobotLocomotion/drake/labels/priority:%20medium","name":"priority: medium","color":"e99695"},{"url":"https://api.github.com/repos/RobotLocomotion/drake/labels/team:%20dynamics","name":"team: dynamics","color":"0052cc"}],"state":"open","locked":false,"assignee":{"login":"david-german-tri","id":17437069,"avatar_url":"https://avatars.githubusercontent.com/u/17437069?v=3","gravatar_id":"","url":"https://api.github.com/users/david-german-tri","html_url":"https://github.com/david-german-tri","followers_url":"https://api.github.com/users/david-german-tri/followers","following_url":"https://api.github.com/users/david-german-tri/following{/other_user}","gists_url":"https://api.github.com/users/david-german-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/david-german-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/david-german-tri/subscriptions","organizations_url":"https://api.github.com/users/david-german-tri/orgs","repos_url":"https://api.github.com/users/david-german-tri/repos","events_url":"https://api.github.com/users/david-german-tri/events{/privacy}","received_events_url":"https://api.github.com/users/david-german-tri/received_events","type":"User","site_admin":false},"assignees":[{"login":"sherm1","id":4088016,"avatar_url":"https://avatars.githubusercontent.com/u/4088016?v=3","gravatar_id":"","url":"https://api.github.com/users/sherm1","html_url":"https://github.com/sherm1","followers_url":"https://api.github.com/users/sherm1/followers","following_url":"https://api.github.com/users/sherm1/following{/other_user}","gists_url":"https://api.github.com/users/sherm1/gists{/gist_id}","starred_url":"https://api.github.com/users/sherm1/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sherm1/subscriptions","organizations_url":"https://api.github.com/users/sherm1/orgs","repos_url":"https://api.github.com/users/sherm1/repos","events_url":"https://api.github.com/users/sherm1/events{/privacy}","received_events_url":"https://api.github.com/users/sherm1/received_events","type":"User","site_admin":false},{"login":"david-german-tri","id":17437069,"avatar_url":"https://avatars.githubusercontent.com/u/17437069?v=3","gravatar_id":"","url":"https://api.github.com/users/david-german-tri","html_url":"https://github.com/david-german-tri","followers_url":"https://api.github.com/users/david-german-tri/followers","following_url":"https://api.github.com/users/david-german-tri/following{/other_user}","gists_url":"https://api.github.com/users/david-german-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/david-german-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/david-german-tri/subscriptions","organizations_url":"https://api.github.com/users/david-german-tri/orgs","repos_url":"https://api.github.com/users/david-german-tri/repos","events_url":"https://api.github.com/users/david-german-tri/events{/privacy}","received_events_url":"https://api.github.com/users/david-german-tri/received_events","type":"User","site_admin":false},{"login":"jwnimmer-tri","id":17596505,"avatar_url":"https://avatars.githubusercontent.com/u/17596505?v=3","gravatar_id":"","url":"https://api.github.com/users/jwnimmer-tri","html_url":"https://github.com/jwnimmer-tri","followers_url":"https://api.github.com/users/jwnimmer-tri/followers","following_url":"https://api.github.com/users/jwnimmer-tri/following{/other_user}","gists_url":"https://api.github.com/users/jwnimmer-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/jwnimmer-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jwnimmer-tri/subscriptions","organizations_url":"https://api.github.com/users/jwnimmer-tri/orgs","repos_url":"https://api.github.com/users/jwnimmer-tri/repos","events_url":"https://api.github.com/users/jwnimmer-tri/events{/privacy}","received_events_url":"https://api.github.com/users/jwnimmer-tri/received_events","type":"User","site_admin":false}],"milestone":null,"comments":19,"created_at":"2016-07-28T16:52:43Z","updated_at":"2016-07-28T21:15:51Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/RobotLocomotion/drake/pulls/2967","html_url":"https://github.com/RobotLocomotion/drake/pull/2967","diff_url":"https://github.com/RobotLocomotion/drake/pull/2967.diff","patch_url":"https://github.com/RobotLocomotion/drake/pull/2967.patch"},"body":"Learning how to setup input/output ports in System 2.0.\r\nThe plan is to create a Diagram with a PI controller on this plant. \r\n\r\nThis is my little step towards implementing RBS (which essentially does F=m*a!).\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/robotlocomotion/drake/2967)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/comments/236027684","html_url":"https://github.com/RobotLocomotion/drake/pull/2967#issuecomment-236027684","issue_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2967","id":236027684,"user":{"login":"amcastro-tri","id":17601461,"avatar_url":"https://avatars.githubusercontent.com/u/17601461?v=3","gravatar_id":"","url":"https://api.github.com/users/amcastro-tri","html_url":"https://github.com/amcastro-tri","followers_url":"https://api.github.com/users/amcastro-tri/followers","following_url":"https://api.github.com/users/amcastro-tri/following{/other_user}","gists_url":"https://api.github.com/users/amcastro-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/amcastro-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amcastro-tri/subscriptions","organizations_url":"https://api.github.com/users/amcastro-tri/orgs","repos_url":"https://api.github.com/users/amcastro-tri/repos","events_url":"https://api.github.com/users/amcastro-tri/events{/privacy}","received_events_url":"https://api.github.com/users/amcastro-tri/received_events","type":"User","site_admin":false},"created_at":"2016-07-28T21:15:51Z","updated_at":"2016-07-28T21:15:51Z","body":"@david-german-tri: Form the very humble perspective of a user, let me give you some feedback on my feeling implementing this simple input port to the mass-spring system. \nI found the abstractions you designed in System 2.0 to be quite beautiful and very useful to split important concepts.\nI must say however that the API for the user (like me) is not quite there yet. It took me two full days of study and a f2f conversation with you to get this started. And even still I find myself uneasy in a number of points:\n- Doing quite a bit of unique_ptr shuffling in a number of places. I think we should not expect our user to do this.\n- I find it strange that in my test `DyanmnicsWithExternalForce` I have to add an input port to the context. In principle the SpringMassSystem should already have this information encoded. The context should only be used for actual data (ports and systems only hold the constant connectivity). This goes along #2955.\n- Notice that for this implementation to add an input port I need to do it in two places: AllocateDefaultContext and in the test itself.\n- I feel quite uneasy writing lengthy things like `context.get_vector_input(0)->get_value().x()` to just get a scalar value out of the context. I don't think our users will like it either. \n\nAgain, I think the System abstraction are just great. However, I do think there is work to do with the concepts of ports. It is not only a matter of API but of splitting the \"connectivity\" concept form the actual data flowing through those connections (the user should not need to add ports to the context but to the system). \n\nThese comments might seem subjective so I will try to quantify them. When reading System 3.0 I had a pretty good idea how to set a system up and connects things in less than an hour. It took me two days and a f2f conversation with @david-german-tri to get this started. \n\nI will try to come up with a simulink example of a simple PID controlled mass-spring system and how this setup could look in our System 2.0, hopefully that will help to clarify my observations.\n\nI hope this is somehow useful!\n\n---\n\nReview status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.\n\n---\n\n*[drake/examples/spring_mass/spring_mass_system.h, line 268 \\[r5\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2967#-KNnGJiVGSAEsvIL_j59:-KNnHT_KFbLhrU0GDsEN:765371266) ([raw file](https://github.com/robotlocomotion/drake/blob/569e504898f48eceff72bcc9f2b46aac273ac61d/drake/examples/spring_mass/spring_mass_system.h#L268)):*\n<details><summary><i>Previously, jwnimmer-tri (Jeremy Nimmer) wrote…</i></summary>\n> The `{}` are required by the drake style guide; the `false` is a bit weird, I agree.\n</details>\nI don't understand the comment here. You mean because it is already initialized in the constructor?\nThe `false` to me is nicer because clearly shows the default. I matter of style I guess.\n\n---\n\n*[drake/examples/spring_mass/test/spring_mass_system_test.cc, line 56 \\[r5\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2967#-KNnGjbKICiSnRg3Vpym:-KNnHq1TzMLnU_8CJVli:-1642843890) ([raw file](https://github.com/robotlocomotion/drake/blob/569e504898f48eceff72bcc9f2b46aac273ac61d/drake/examples/spring_mass/test/spring_mass_system_test.cc#L56)):*\n<details><summary><i>Previously, jwnimmer-tri (Jeremy Nimmer) wrote…</i></summary>\n> Nobody is using this default argument anymore.\n</details>\nNow yes :-)\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/robotlocomotion/drake/2967#-:-KNnIBOzBNUJrdhanViG:453670549)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"amcastro-tri"}}