Closed SeanCurtis-TRI closed 4 years ago
Just a couple of comments on the issue since it's finally receiving attention. One note is that Reviewable is failing to preserve context with the review comments (the comments are intact but the places pointed to in the code are not.)
Doxygen needs:
* Guide to authors of a new `LeafSystem`s, what type of events they would register,
I agree completely and started to add this to LeafSystem. Unfortunately, there is now zero information for LeafSystem authors so adding info about events there would now be incongruous. Issue #8616 already covers that issue (I've updated it to point this out). I also put a little info into my upcoming PR (#12501) for addressing this present issue.
* [ ] `GetSubSystemStuff()`. Probably need to revisit the _name_ of this method.
It's used for lots of, ahem, stuff, so I think this uninformative name is actually appropriate.
The new API for the event system is far-reaching and complex. The decision was made to allow it through without having wholly satisfying documentation because of its complexity. Accordingly, this issue is intended to track the known documentation issues (which may or may not be specifically called out in todos in the code):
Doxygen needs:
LeafSystem
s, what type of events they would register,DoPublish
method might be overloaded.kPerStep
in detail.Event
refers to a no-op handling. This is confusing because the architecture allows handling either in the event callback or in theDoPublish
(or corresponding method) as handling as well.LeafEventCollection::MakeForcedEventCollection()
- the role this method plays in the ecosystem needs to be clear.CompositeEventCollection::add_publish_event()
- clarify the relationship between the base class and the type ofEventCollection
it contains.Publish
,CalcDiscreteUpdate
, andCalcUnrestrictedUpdate
(and corresponding methods, discuss event processing in terms of a common vernacular (defined in the module).Publish
(https://reviewable.io/reviews/robotlocomotion/drake/6050#-Kmm-wpyNB4x_qF8Sb_R:-Kn_czQo0sWqn-g3ojiY:b-n5fyb3) and comment on line 196.DispatchPublishHandler()
.Implementation details for developers:
AllocateForced_____EventCollection()
. Document how this non-obvious C++ machinery works.GetSubSystemStuff()
. Probably need to revisit the name of this method.UpdateDiscreteVariables
rely onCalcNextUpdate
to clear the event collection.