Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Diagnostics service #1390

Merged
merged 20 commits into from Oct 3, 2017

Conversation

Projects
None yet
3 participants
@sergeuz
Copy link
Member

commented Sep 18, 2017

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)

@sergeuz sergeuz added the enhancement label Sep 18, 2017

@sergeuz sergeuz added this to the 0.8.0 milestone Sep 18, 2017

@sergeuz sergeuz requested a review from m-mcgowan Sep 18, 2017

@@ -1 +1,4 @@
#include "../../../../hal/src/stm32/newlib.cpp"

// Defining this in hal/src/stm32/newlib.cpp causes linker errors
void *__dso_handle = NULL;

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Sep 18, 2017

Contributor

Do you know what has changed to make this necessary?

This comment has been minimized.

Copy link
@sergeuz

sergeuz Sep 18, 2017

Author Member

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

typedef void(*diag_enum_sources_callback)(const diag_source* src, void* data);

typedef struct diag_source {
size_t size; // Size of this structure

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Sep 18, 2017

Contributor

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

This comment has been minimized.

Copy link
@sergeuz

sergeuz Sep 18, 2017

Author Member

👍

class Diagnostics {
public:
Diagnostics() :
enabled_(0) { // The service is disabled initially

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Sep 18, 2017

Contributor

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?

This comment has been minimized.

Copy link
@sergeuz

sergeuz Sep 18, 2017

Author Member

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

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Sep 18, 2017

Contributor

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.

This comment has been minimized.

Copy link
@sergeuz

sergeuz Sep 18, 2017

Author Member

👍

operator IntType() const;

private:
std::atomic<IntType> val_;

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Sep 18, 2017

Contributor

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.

This comment has been minimized.

Copy link
@sergeuz

sergeuz Sep 18, 2017

Author Member

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.


template<typename LockingPolicyT>
inline particle::AbstractIntegerDiagnosticData::IntType particle::IntegerDiagnosticData<LockingPolicyT>::operator++() {
LockingPolicyT::lock();

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Sep 18, 2017

Contributor

RAII opportunity here?

This comment has been minimized.

Copy link
@sergeuz

sergeuz Sep 19, 2017

Author Member

👍

@sergeuz sergeuz referenced this pull request Sep 22, 2017

Merged

"sys.uptime" diagnostic data #1393

3 of 6 tasks complete

@sergeuz sergeuz referenced this pull request Sep 26, 2017

Merged

"net.rssi" diagnostic data #1397

3 of 6 tasks complete

@m-mcgowan m-mcgowan merged commit 7925fda into develop Oct 3, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.