Style Tips - Patterns, Anti-Patterns & Suggestions

Table of Contents

• Introduction
• Naming TopicStates
• Too Big vs. Too Small TopicStates
  • Type is data
  • Trivial/Empty Topics are sometimes fine.
  • Variables/bits/data that change together atomically belong together.
  • Mutually exclusive states also belong together.
  • Topics are APIs
• Detectors and Topics Dependencies
• Caching/Saving TopicStates is the right solution!
• Settings are also Topics
• Flat Evaluates vs. Indirect and/or nested member methods
• Concentrators & Aggregations
• Resuming State
• Explicit calls to Evaluate()?
• Publishing Initial States
• Usage of Namespaces

Introduction

This page contains a set of guidelines & rules of thumb that we accumulated after 3+ years of using DetectorGraph. These are aimed at keeping software design constrained in a way that best takes advantage of the DetectorGraph framework, its expressibility and modeling power.

Naming TopicStates

Topics carry TopicStates. TopicStates can express states, events/transitions, operations/requests & responses. Examples are:

• FooState
• BarSample when Bar is a sensor.
• FooBarState, produced by a FooBarDetector. Note that FooBarDetectorState is not encouraged as it ties the producer with it's product - but it can be ok in some very specific cases.
• SomethingRequest & SomethingResponse when implementing a RPC Request/Response pattern.
• FooBarSettings for settings that determine how FooBarDetector (and by extension FooBarState) behave. See also Settings.

Too Big vs. Too Small TopicStates

Hypothetically, the entirety of a system's state could be contained on a single TopicState that is Subscribed to & published to by every detector. Conversely, one could split every individual bit (pardon the pun) of data into separate TopicStates and have detectors only subscribe to the bits necessary to form the data it depends on. Both are clearly poor separations-of-concern choices and are clear misuses of the DetectorGraph framework - obviously the sweet spot lies in between, but where? Below are a set of guidelines for partitioning Topics:

Type is data

A rose by any other name would smell as sweet

— Juliet

Topics can be seen as named buses where the name is the TopicState's type - and since Subscriptions are to a type, the type itself has 'meaning'. Thus a TopicState's type carries both a name and the data structure/fields inside the TopicState. Often it is appropriate to have different names for otherwise equivalent data structures - that is fine and encouraged.

This can be the case when an application has different topics for filtered vs. unfiltered data for a sensor; or some data through different layers of verification/processing.

There are two ways of accomplishing this; composition or inheritance. Composition is often preferable over inheritance as it gives more flexibility but both should be considered and are possible.

Composition example (from Robot Localization):

struct KalmanState
{
    KalmanState() : pose(Vector3d::Zero()), error(Matrix3d::Zero()) {}
    KalmanState(Vector3d aPose, Matrix3d aError) : pose(aPose), error(aError) {}
    Vector3d pose;
    Matrix3d error;
};

//! [Mutually Atomic Variables]
struct LocalizationBelief : public TopicState
{
    LocalizationBelief() : timestampMs(), state() {}
    LocalizationBelief(uint64_t ts, const KalmanState& aState) : timestampMs(ts), state(aState) {}
    LocalizationBelief(uint64_t ts, const Vector3d& aPose, const Matrix3d& aError) : timestampMs(ts), state(aPose, aError) {}
    uint64_t timestampMs;
    KalmanState state;
};
//! [Mutually Atomic Variables]

struct GPSPosition : public TopicState
{
    GPSPosition() : timestampMs(), state() {}
    GPSPosition(uint64_t ts, const KalmanState& aState) : timestampMs(ts), state(aState) {}
    uint64_t timestampMs;
    KalmanState state;
};

Inheritance examples From Fancy Vending Machine:

struct RefillChange : public DetectorGraph::TopicState, public ChangeAlgo::CoinStock
{
    RefillChange() {}
    RefillChange(const ChangeAlgo::CoinStock& stock) : ChangeAlgo::CoinStock(stock) {}
};

struct ReleaseCoins : public DetectorGraph::TopicState, public ChangeAlgo::Draw
{
    ReleaseCoins() {}
    ReleaseCoins(const ChangeAlgo::Draw& draw) : ChangeAlgo::Draw(draw) {}
};

From Beat Machine:

struct SongLine
{
    std::string line;
    SongLine() : line() {}
    SongLine(const std::string& aLine) : line(aLine) {}
    friend std::ostream& operator<<(std::ostream& os, SongLine s) { return os << s.line; }
};

struct SongThemeState : public DetectorGraph::TopicState, public SongLine
{
    SongThemeState() {}
    SongThemeState(const std::string& aLine) : SongLine(aLine) {}
};

struct DontThemeState : public DetectorGraph::TopicState, public SongLine
{
    DontThemeState() {}
    DontThemeState(const std::string& aLine) : SongLine(aLine) {}
};

struct RememberState : public DetectorGraph::TopicState, public SongLine
{
    RememberState() {}
    RememberState(const std::string& aLine) : SongLine(aLine) {}
};

struct ThenYouState : public DetectorGraph::TopicState, public SongLine
{
    ThenYouState() {}
    ThenYouState(const std::string& aLine) : SongLine(aLine) {}
};

struct PlaybackState : public DetectorGraph::TopicState
{
    bool autoplay;
    PlaybackState(bool aAutoplay = false) : autoplay(aAutoplay) {}
};

Inheritance can save you some typing and be OK when literally all you're changing is the type 'name' and when you know the data structure will never change and when the 'is a' relationship holds strongly. Inheritance is the lazy approach but that you'll likely have to ditch (at great expense possibly) at some point in the future.

Trivial/Empty Topics are sometimes fine.

Sometimes all a topic needs to express is the fact that it updated, that it happened or any other purely unary signals:

From Trivial Vending Machine:

struct CoinInserted : public DetectorGraph::TopicState
{
};

From Fancy Vending Machine:

struct MoneyBackButton : public DetectorGraph::TopicState
{
};

Variables/bits/data that change together atomically belong together.

Examples:

A product and its new price:

struct PriceUpdate : public DetectorGraph::TopicState
{
    ProductIdType productId;
    int priceCents;
    PriceUpdate(ProductIdType aProduct = kProductIdTypeNone, int aPrice = 0) : productId(aProduct), priceCents(aPrice) {}
};

A robot's pose and a timestamp:

struct LocalizationBelief : public TopicState
{
    LocalizationBelief() : timestampMs(), state() {}
    LocalizationBelief(uint64_t ts, const KalmanState& aState) : timestampMs(ts), state(aState) {}
    LocalizationBelief(uint64_t ts, const Vector3d& aPose, const Matrix3d& aError) : timestampMs(ts), state(aPose, aError) {}
    uint64_t timestampMs;
    KalmanState state;
};

Mutually exclusive states also belong together.

struct DoorState : public TopicState
{
    enum OpenCloseState {
        kOpen,
        kClosed,
    };

    OpenCloseState state;
};

This also includes Topics used to drive (or driven by) a finite state machine.

Topics are APIs

Another rule of thumb is to think of Topics as APIs themselves; they should be as general as possible. More general TopicStates tend to better isolate concerns and be more reusable in the future. In general, when creating a topic don't name it with respect to what it'll be used; instead try to name it according to what information it carries.

Detectors and Topics Dependencies

One of the main advantages of using DetectorGraph is a very clear isolation of dependencies & concerns. To maintain those advantages DetectorGraph code should comply with the following rules:

• Detectors should not have any dependencies on any other Detectors: no data-structure dependencies nor runtime/callgraph dependencies.
• Detectors can depend on any number of Topics; ideally only the Topics it Subscribes or Publishes.
• Topics can depend on other Topics for data-structures but should never inherit from other Topics.
• Detectors should do no I/O nor should they talk to other dynamic software modules; debug/text logging is probably the one notable exception. It's totally fine to use a math library or an algorithm-specific library but it's not OK to communicate across threads accessing a singleton outside the DetectorGraph.

Caching/Saving TopicStates is the right solution!

Sometimes detectors only need a the information in TopicStateA when evaluating TopicStateB. It may seem like Subscribing to TopicStateA is overkill - but it's not. Remember that Evaluate(TopicStateA)s is only called when TopicStateA changes and thus there's no runtime penalty on having multiple Evaluate()s that are called rarely.

The recommended pattern here is:

class FooBarDetector : public SubscriberInterface<Foo>, public SubscriberInterface<Bar>, public Publisher<FooBarState>
{
//...
    void Evaluate(const Foo& aNewFoo)
    {
        mCurrentFoo = aNewFoo;
    }

    void Evaluate(const Bar& aNewBar)
    {
        Publish(FooBar(mCurrentFoo, aNewBar));
    }

private:
    Foo mCurrentFoo;
};

Note that most TopicStates are small bits of data and thus copying by value should always be efficient. In cases where a TopicState contains a lot of data, such that copy-by-value would be a problem, it is the responsibility of that TopicStates implementation to implement an appropriate shallow copies scheme. For an example of that see Sharing Memory across TopicStates.

Settings are also Topics

Topics should be used to convey settings as well, and caching them is the way to go (see Caching/Saving TopicStates is the right solution!)

class TooHotDetector : public SubscriberInterface<TooHotThreshold>, public SubscriberInterface<Temperature>, public Publisher<TooHot>
{
    void Evaluate(const TooHotThreshold& aThreshold)
    {
        mCurrentThreshold = aThreshold;
    }

    void Evaluate(const Temperature& aTemperature)
    {
        if (aTemperature.value > mCurrentThreshold.value)
        {
            Publish(TooHot());
        }
    }

private:
    TooHotThreshold mCurrentThreshold;
};

Flat Evaluates vs. Indirect and/or nested member methods

Flat is better than nested

— Zen of Python

When reading a Detector's code some of the first concepts a reader tries to grasp is the causality between input TopicStates and output ones - simplifying the answer to this question is one of the design goals of DetectorGraph and one of the main reasons one would use the framework. To simplify this further one should keep in mind that the path between Evaluate() methods and Publish() calls should be as straightforward as possible. One way to achieve that is to reduce call stack between the two to a minimal. For example:

class TooHotDetector : public SubscriberInterface<Temperature>, public Publisher<TooHot>
{
    //...
    void Evaluate(const Temperature& temp)
    {
        PublishTooHotIfTooHot(temp);
    }

    void PublishTooHotIfTooHot(const Temperature& temp)
    {
        // do a bunch of stuff with Temperature
        if (...)
        {
            Publish(TooHot(true));
        }
        else
        {
            Publish(TooHot(false));
        }
    }
};

versus:

class TooHotDetector : public SubscriberInterface<Temperature>, public Publisher<TooHot>
{
    //...
    void Evaluate(const Temperature& temp)
    {
        bool tooHot = IsTooHot(temp);
        Publish(TooHot(tooHot));
    }

    bool IsTooHot(const Temperature& temp)
    {
        // do a bunch of stuff with Temperature
        return isTooHot;
    }
};

In the first example a reader must follow & comprehend what PublishTooHotIfTooHot does before they can conclude that TooHotDetector will always publish 1 (and only one) TooHot state per input sample. In the second example that relationship is trivial. The intent is to have direct paths from an Evaluate() (or BeginEvaluation(), CompleteEvaluation()) to Publish (or FuturePublish(), PublishOnTimeout()).

Concentrators & Aggregations

In many cases detectors provide an accumulated/aggregated view of a set of TopicStates. The simplest & more readable way we found to do this was based on variations of the pattern below:

class AggregatorDetector : public Detector,
    public SubscriberInterface<BoosterState>,
    public SubscriberInterface<RetrofireState>,
    public SubscriberInterface<FlightDynamicsState>,
    public Publisher<LiftOff>
{
    enum CheckList
    {
        kBooster = 0,
        kRetrofireState,
        kFlightDynamicsState,

        // last
        kCheckListCount
    };
    std::bitset<kCheckListCount> mCheckList;

    // ...
    void Evaluate(const BoosterState& aBoosterState) { mCheckList[kBooster] = (aBoosterState.Go()) }

    void Evaluate(const RetrofireState& aRetroState) { mCheckList[kRetrofireState] = (aRetroState.Go()) }

    void Evaluate(const FlightDynamicsState& aFidoState) { mCheckList[kFlightDynamicsState] = (aFidoState.Go()) }

    // ..

    void CompleteEvaluation() {
        if (mCheckList.all()) {
            Publish(LiftOff());
        }
    }
};

Note the use of CompleteEvaluation to make the general conditional check regardless what inputs have changed.

Resuming State

The best way we found to resume graph states is to use a specific TopicState (ResumeFromSnapshotTopicState) that contains a de-serialized version of the latest preserved StateSnapshot. Each detector that needs to resume its state then has a chance of inspecting the entire snapshot to reconstruct its state.

Initially it was thought that state resuming could be done simply be re-publishing the stored TopicStates but since most Detectors keep state they'd have to subscribe to their own outputs - in a way that the Resume operation would go in reverse order than the normal topological sort of the graph. That was deemed overly complex & contrived and would greatly increase code complexity.

Instead we opted of giving detectors this know-all single chance to restore their state & re-publish any resumed state TopicStates as necessary.

For a full example see Resuming Counter ( Basic Counter Graph with persistent storage. )

Explicit calls to Evaluate()?

Detector's Evaluate() method calls are central to DetectorGraph applications. They are called by the framework in a very specific and coordinated manner (BeginEvaluation()->n*Evaluate()->CompleteEvaluation()).

In some situations it may seem appropriate to explicitly/manually call Evaluate(X) from within another method of a Detector to process a specific (e.g. initial) version of X - that's sometimes an anti-pattern. When a reader encounters an Evaluate(Z) method he/she expects that to only be called when Z has changed - and most of the times that's also what log messages inside Evaluate(Z) will have log readers believe. By manually calling Evaluate() you break that expectation.

More than once this had led debugging along a completely wrong path and makes Detectors program flow less flat and more convoluted. If a single set of operations is necessary for multiple different inputs it is better to implement that as a separate function and have that called from both Evaluate()s.

Publishing Initial States

Detectors do not have a specific mechanism for publishing their initial states/prime outputs.

Detectors are free & encouraged to use their constructors to initialize data members and state but calling any Publish() (FuturePublish(), PublishOnTimeout()) from within the constructor is not supported/allowed (I'd consider it a bug).

This is somewhat intentional - sometimes allowing program flow to diverge a lot between first & not-first instances hurts readability and creates more code for the same functionality. By limiting Publish() calls to Evaluate() calls' bodies the program flow through the graph remains consistent throughout the entire life of DetectorGraphs (i.e. evaluations always follow topological sorts & always happen due to a Topic posting "from the outside").

Instead the suggested pattern going forward is to always rely on ResumeFromSnapshotTopicState and make sure your implementation provides an "empty" ResumeFromSnapshotTopicState when a de-serialized one isn't available.

In the past we have used a specific TopicState (say DetectorGraphInitialized) that detectors can subscribe to to be notified when the system boots and publish any new state they may need to. That solution is redundant and not as canonical as the solution described above so it's not recommended anymore.

For a full example see Resuming Counter ( Basic Counter Graph with persistent storage. )

Usage of Namespaces

All DetectorGraph names are inside the DetectorGraph namespace. Within an application it is suggested to have a single namespace reserved for all your Detectors & TopicStates; that allows you to use short names & appropriate names for those without risk of collision. We have also used separate namespaces for TopicsStates & Detectors but that didn't yield much readability or expressibility. Things to keep in mind when making your decision:

• Detectors' names are used not used often in code.
• Detectors have a dependency on Topics.
• Topics' names are used very often in code.
• Topics do not depend on detectors or on any other code modules.
• Although they can depend on reusable data structures, they should strive to be self-contained.