Diagnostics service

[sergeuz]

Problem

This PR implements a simple service allowing the system and applications to register diagnostic data sources.

Steps to Test

Run unit tests.

Example App

#include "application.h"

enum DiagnosticId {
    UPTIME = DIAG_ID_USER,
    COUNTER,
    STATE
};

enum State {
    CONNECTING = 1,
    CONNECTED = 2,
    DISCONNECTED = 3
};

class UptimeDiagnosticData: public AbstractIntegerDiagnosticData {
public:
    UptimeDiagnosticData() :
            AbstractIntegerDiagnosticData(UPTIME, "app.uptime") {
    }

protected:
    virtual int get(int32_t& val) override {
        val = millis();
        return 0; // OK
    }
};

UptimeDiagnosticData uptime;
AtomicIntegerDiagnosticData counter(COUNTER, "app.counter");
AtomicEnumDiagnosticData<State> state(STATE, "app.state", DISCONNECTED);

void setup() {
    ++counter;
    state = CONNECTING;
}

void loop() {
}

References

• [ch7575]

---

Completeness

• User is totes amazing for contributing!
• Contributor has signed CLA (Info here)
• Problem and Solution clearly stated
• Run unit/integration/application tests on device
• Added documentation
• Added to CHANGELOG.md after merging (add links to docs and issues)

[m-mcgowan]

Do you know what has changed to make this necessary?

[sergeuz]

part1 now has a global variable (Diagnostics) with a non-trivial destructor

[m-mcgowan]

should we make size a 16 bit quantity so it packs nicely into memory better? Oh I see it already does. Maybe add another uint16_t reserved so we can make use of those 16-bits and keep the padding alignment

[sergeuz]

👍

[m-mcgowan]

what's the rationale for having enable/disable for the service? If it's to save expensive computation then perhaps we should move the enable flag to the diag_source?

[sergeuz]

The rationale is to avoid a race condition, which is possible when the system receives a request or a call querying diagnostic data but data sources are still being registered. The system explicitly enables the diagnostic service after invoking all global constructors in all system parts here

[m-mcgowan]

Thanks, that makes sense. The name enabled is slightly counter-intuitive and that you cannot add sources once it is enabled. On first reading, I was sure I understood it as the opposite - the error is raised when adding sources and the service is not enabled.

Perhaps we should call it running, or started? Alternatively, you could add a comment to explain, since normally things are not usable while disabled, while here it's the opposite.

[sergeuz]

👍

[m-mcgowan]

What's the overhead of using std:atomic here with SimpleIntegerDiagnosticData? The majority of cases the diagnostic data doesn't need to be thread safe, only a few cases need it, so the non-atomic version should be the more efficient case ideally.

[sergeuz]

SimpleIntegerDiagnosticData uses a dummy NoLockingPolicy, so there shouldn't be any overhead. The class in question is a specialization of IntegerDiagnosticData for AtomicLockingPolicy, i.e. it's always up to a developer to decide if a particular diagnostic data needs to be updated atomically.

[m-mcgowan]

RAII opportunity here?

[sergeuz]

👍